Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

verify id token validity [On Hold] #49

Closed
wants to merge 5 commits into from
Closed

Conversation

idg-sam
Copy link
Contributor

@idg-sam idg-sam commented Oct 20, 2020

No description provided.

Copy link
Contributor

@rayluo rayluo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, you move fast, Sam! Love your speed!

app.py Outdated
@@ -19,9 +20,10 @@

@app.route("/")
def index():
if not session.get("user"):
if is_authenticated():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implementation would only work when an end user visits this web app from its root page, which typically happens only when that end user is typing the domain name in browser (i.e. start to use this web app), but it won't work when the end user already signs in and navigating among different links of this web app.

Perhaps we can try to adjust the session's life time - something like this - when the ID token was returned.

Copy link
Contributor Author

@idg-sam idg-sam Oct 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. That link also has a handy session.permanent = True which we might use to remove the sample's dependency on flask-session? It looks like we use flask-session due to oversized cookie (due to token_cache?) rather than permanence.

Are there situations in which the user might want a different session TTL than the AAD ID token exp? e.g. they might have some other session data that they'd like to keep for longer, e.g. site display settings. If so, we should change the session.clear() to session.pop('user', None); session.pop('token_cache', None)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we choose not to kill the whole session based on ID token exp, we can wrap auth-required endpoints with a check:

@app.route('/a_protected_route')
@auth_required
def a_protected_route():
   return render_template('protected.html')

which can be defined as something like:

from functools import wraps
def auth_required(f):
        @wraps(f)
        def is_authenticated(*args, **kwargs):
            try:
                # does the id token exist in session? does exp claim exist in the id token? and is exp still valid?
                assert(session["user"]["exp"] > time.time())
                return f(*args, **kwargs)
            except:
                return redirect(url_for("login"))
        return is_authenticated

Copy link
Contributor

@rayluo rayluo Nov 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Sam for all the meaningful questions above!

It looks like we use flask-session due to oversized cookie (due to token_cache?) rather than permanence.

Not exactly. We can have one of the two ways to store the tokens: either server-side session or (encrypted) client-side session. Both have their pros and cons. Some cons might be mitigated by implementing some more workarounds. But then, the SNR (Signal-to-Noise Ratio) comes into play. As a sample for introducing MSAL, we do not want it to become a "crash course: everything you need to know to develop web app" which overload/distract our customer. The flask-session was chosen, largely because it happened to allow a very concise usage (only 1 line, besides its import line), so that it won't get in the way of our customer's learning curve.

Are there situations in which the user might want a different session TTL than the AAD ID token exp?

That is the million-dollar question. The OIDC specs says "Expiration time on or after which the ID Token MUST NOT be accepted for processing", and some interpretation is "The ID token has to be un-expired at this point of use (which it should be, since it has just been issued). But after this it is not used again, so it does not matter if it expires while the user still has an active session. The Client has the authentication information it needs, and in turn can choose its own policy for how long the session lasts before the user has to log in again." On the other hand, there is also guidance on (emphasis mine): "USUALLY, a web application matches a user’s session lifetime in the application to the lifetime of the ID token issued for the user. You can adjust the lifetime of an ID token to control how often the web application expires the application session, and how often it requires the user to be reauthenticated with Microsoft identity platform (either silently or interactively)."

e.g. they might have some other session data that they'd like to keep for longer, e.g. site display settings. If so, we should change the session.clear() to session.pop('user', None); session.pop('token_cache', None)

In practice, we tried that session.pop('user', None); session.pop('token_cache', None) in our early prototype, but the resulting behavior seemed potentially confusing: part of the app would still be accessible with partial information, and another part of the app would trigger a re-authentication, app developer might not realize such a seemingly-inconsistent behavior were "a feature not a bug (TM)". :-) So, our sample philosophy prevailed again, "when in doubt, leave it out".

If we choose not to kill the whole session based on ID token exp, we can wrap auth-required endpoints with a check:
...

This can still be a worthy idea to pursue, if we are (and we will) going to build a more polished web app sample(s) down the road. But, for the current minimalist web app sample, perhaps adding a one-liner app.permanent_session_lifetime = timedelta(minutes=...) right after the successful sign-in, would be good-enough, although I have not actually tested it, yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will implement this and and get back to you.
Do you think there is any merit in keeping the app synced with the SSO session (keep getting ID tokens until AAD decides to kick them out)?
I think an individual ID token TTL is configurable to be 10min-24hrs but the AAD SSO session can be configured from 10min all the way to "until revoked" (doc is confusing - it means either 365days or forever) depending on policy.

Copy link
Contributor

@rayluo rayluo Nov 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think there is any merit in keeping the app synced with the SSO session (keep getting ID tokens until AAD decides to kick them out)?

Based on the guidance (which we quoted above), that is indeed the expectation to every web app. The actual responsibility goes to each web app's developer thus out of our control, but here in our web app sample(s), we should demonstrate the recommended behavior. Oh, and the merit is, by triggering re-auth frequently, the web app would honor a potential revocation of the user consent.

@idg-sam
Copy link
Contributor Author

idg-sam commented Oct 21, 2020

new commit implements:

  • auth_required decorator that checks for id token + exp validation
  • auth_required on index '/' route
  • auth_required on graphcall '/graphcall' route
  • don't destroy session based on id token or logout, only remove 'user' and 'token_cache' on logout.
  • session TTL not tied to ID token exp claim

@idg-sam
Copy link
Contributor Author

idg-sam commented Oct 22, 2020

In this PR, once the user's token expires:

  1. The user is redirected to our-app/login. The user then clicks sign in to be redirected to AAD/authorize.
  2. At the AAD/authorize endpoint, one of two things happens:
    a. If the user has only have one signed-in account with AAD, it passes through silently
    b. If the user has multiple signed-in accounts on AAD, AAD will ask the user to to choose one
  • Is 1. the behaviour we want? If not, we can skip this step if the user is signed-in but token has expired (e.g., send them directly to AAD/authorize)
  • Is 2.b. the behaviour we want? If not, we can pass in a login_hint when sending them to AAD/authorize

I'll check in a test that makes the ID token refresh seamless. I'll use the preferred_username claim as the login_hint.

@idg-sam idg-sam requested a review from rayluo November 7, 2020 02:49
@rayluo rayluo changed the title verify id token validity verify id token validity [On Hold] Mar 4, 2021
@rayluo
Copy link
Contributor

rayluo commented Feb 8, 2023

Thanks @idg-sam for the prototyping. We ended up choosing to move most of this kind of helper logic into a separate helper library, instead of adding more and more helpers into this sample. Closing this PR now.

@rayluo rayluo closed this Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants