-
Notifications
You must be signed in to change notification settings - Fork 144
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
Conversation
There was a problem hiding this 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(): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 It looks like we use flask-session due to oversized cookie (due to token_cache?) rather than permanence.session.permanent = True
which we might use to remove the sample's dependency on flask-session?
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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()
tosession.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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
new commit implements:
|
In this PR, once the user's token expires:
I'll check in a test that makes the ID token refresh seamless. I'll use the |
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. |
No description provided.