Sessions don't refresh group membership information without logout/login cycle #81
Assignees
Labels
bug
Something isn't working
Comments
|
@jameshadfield The current behaviour is a temporary one, which arose from necessary expediency in the original manifestation the INRB group. As I mentioned earlier, we can improve this lag/re-login issue in the future by refreshing user details from Cognito on ~every nextstrain.org request, ideally with a short-lived TTL cache in between to avoid multiple hits to Cognito across a rapid series of nextstrain.org requests. |
|
Recent thinking on two approaches that combined will address session staleness:
|
tsibley
added a commit
that referenced
this issue
Mar 18, 2022
This will make potential future user object updates in deserializeUser() much easier because we won't have to figure out which old user properties contain the "unparsed" or "original" array of Cognito groups to re-parse. Ultimately, it will still be better to move to storing the original id and refresh tokens in the session instead of the user object. The deserializeUser() step would then reparse the id token (after potentially refreshing it) every time and not have to worry about upgrading a derived, persistent object. I'm not sure when we'll get around to that, however, so for now this bit of low-effort future-proofing seems likely to be helpful. Related to <#81>.
tsibley
added a commit
that referenced
this issue
Apr 7, 2022
This will make potential future user object updates in deserializeUser() much easier because we won't have to figure out which old user properties contain the "unparsed" or "original" array of Cognito groups to re-parse. Ultimately, it will still be better to move to storing the original id and refresh tokens in the session instead of the user object. The deserializeUser() step would then reparse the id token (after potentially refreshing it) every time and not have to worry about upgrading a derived, persistent object. I'm not sure when we'll get around to that, however, so for now this bit of low-effort future-proofing seems likely to be helpful. Related to <#81>.
tsibley
added a commit
that referenced
this issue
Apr 7, 2022
This will make potential future user object updates in deserializeUser() much easier because we won't have to figure out which old user properties contain the "unparsed" or "original" array of Cognito groups to re-parse. Ultimately, it will still be better to move to storing the original id and refresh tokens in the session instead of the user object. The deserializeUser() step would then reparse the id token (after potentially refreshing it) every time and not have to worry about upgrading a derived, persistent object. I'm not sure when we'll get around to that, however, so for now this bit of low-effort future-proofing seems likely to be helpful. Related to <#81>.
tsibley
added a commit
that referenced
this issue
May 10, 2022
…object Validates the stored tokens and constructs a user object on each request, with automatic behind-the-scenes token renewal (and thus user info refresh) as necessary. This is identical to the Nextstrain CLI token handling. We can control how often the server refreshes info from Cognito by tuning the token lifetimes in the Cognito client config for the server. This change also lets us implement "stale before" timestamps for users when we knowingly change their info in Cognito and automatically refresh based on those so that we can keep the cached data fresh. Existing sessions are left as-is for now. They can't be upgraded automatically because the tokens used to establish them at initial login are long gone. We'll need to decide what to do with them (e.g. force all existing sessions to login again). Partially resolves <#81>.
tsibley
added a commit
that referenced
this issue
May 10, 2022
This is a basic cache-invalidation system applied to our authn tokens (and future snapshots of user info).¹ When the app knowingly makes changes to user data in Cognito, such as modifying a user's group memberships, it can mark user data as "stale before" that time. For sessions, this triggers automatic server-side token renewal and thus refresh of user data (e.g. the new group memberships) from Cognito. For API clients (like Nextstrain CLI), this triggers a 401 Unauthorized error prompting the client to retry after renewal. Timestamps are stored in Redis under a per-user key, if Redis is available, else they're stored in a transient in-process Map (useful only for dev). Currently no code sets these timestamps. I expect the first usage to be with the addition of group membership management endpoints to the RESTful API. Partially resolves <#81>. ¹ Using JWTs means you're dealing with a cache (e.g. of the data in the token), even if you don't normally think about it that way. While it might seem we could use the JWT to just get the username and always fetch the latest user info from Cognito's admin API, this isn't feasible because of the rate limits imposed by Cognito and the additional latency it would introduce.
tsibley
added a commit
that referenced
this issue
May 10, 2022
…object Validates the stored tokens and constructs a user object on each request, with automatic behind-the-scenes token renewal (and thus user info refresh) as necessary. This is identical to the Nextstrain CLI token handling. We can control how often the server refreshes info from Cognito by tuning the token lifetimes in the Cognito client config for the server. This change also lets us implement "stale before" timestamps for users when we knowingly change their info in Cognito and automatically refresh based on those so that we can keep the cached data fresh. Existing sessions are left as-is for now. They can't be upgraded automatically because the tokens used to establish them at initial login are long gone. We'll need to decide what to do with them (e.g. force all existing sessions to login again). Partially resolves <#81>.
tsibley
added a commit
that referenced
this issue
May 10, 2022
This is a basic cache-invalidation system applied to our authn tokens (and future snapshots of user info).¹ When the app knowingly makes changes to user data in Cognito, such as modifying a user's group memberships, it can mark user data as "stale before" that time. For sessions, this triggers automatic server-side token renewal and thus refresh of user data (e.g. the new group memberships) from Cognito. For API clients (like Nextstrain CLI), this triggers a 401 Unauthorized error prompting the client to retry after renewal. Timestamps are stored in Redis under a per-user key, if Redis is available, else they're stored in a transient in-process Map (useful only for dev). Currently no code sets these timestamps. I expect the first usage to be with the addition of group membership management endpoints to the RESTful API. Partially resolves <#81>. ¹ Using JWTs means you're dealing with a cache (e.g. of the data in the token), even if you don't normally think about it that way. While it might seem we could use the JWT to just get the username and always fetch the latest user info from Cognito's admin API, this isn't feasible because of the rate limits imposed by Cognito and the additional latency it would introduce.
tsibley
added a commit
that referenced
this issue
May 16, 2022
…object Validates the stored tokens and constructs a user object on each request, with automatic behind-the-scenes token renewal (and thus user info refresh) as necessary. This is identical to the Nextstrain CLI token handling. We can control how often the server refreshes info from Cognito by tuning the token lifetimes in the Cognito client config for the server. This change also lets us implement "stale before" timestamps for users when we knowingly change their info in Cognito and automatically refresh based on those so that we can keep the cached data fresh. Existing sessions are left as-is for now. They can't be upgraded automatically because the tokens used to establish them at initial login are long gone. We'll need to decide what to do with them (e.g. force all existing sessions to login again). Partially resolves <#81>.
tsibley
added a commit
that referenced
this issue
May 16, 2022
This is a basic cache-invalidation system applied to our authn tokens (and future snapshots of user info).¹ When the app knowingly makes changes to user data in Cognito, such as modifying a user's group memberships, it can mark user data as "stale before" that time. For sessions, this triggers automatic server-side token renewal and thus refresh of user data (e.g. the new group memberships) from Cognito. For API clients (like Nextstrain CLI), this triggers a 401 Unauthorized error prompting the client to retry after renewal. Timestamps are stored in Redis under a per-user key, if Redis is available, else they're stored in a transient in-process Map (useful only for dev). Currently no code sets these timestamps. I expect the first usage to be with the addition of group membership management endpoints to the RESTful API. Partially resolves <#81>. ¹ Using JWTs means you're dealing with a cache (e.g. of the data in the token), even if you don't normally think about it that way. While it might seem we could use the JWT to just get the username and always fetch the latest user info from Cognito's admin API, this isn't feasible because of the rate limits imposed by Cognito and the additional latency it would introduce.
Repository owner
moved this from Prioritized
to Done
in Nextstrain planning (archived)
Jun 16, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Right now the code (as I understand it) fetches and stores cognito groups upon login (https://github.com/nextstrain/nextstrain.org/blob/master/authn.js#L76). This can lead to situations where (i) the code doesn't see "new" cognito groups until the user re-logs-in and (ii) the code sees groups which have been removed until the user logs in again.
@tsibley is this the general way things are done (this is not my area of expertise)? Could we poll the Cogntio API every ~24 hours to update without requiring the user to re-login?
The text was updated successfully, but these errors were encountered: