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

Modifications to session after login are overwritten over time #416

Closed
matthewdavidson opened this issue Jun 10, 2021 · 7 comments · Fixed by #506
Closed

Modifications to session after login are overwritten over time #416

matthewdavidson opened this issue Jun 10, 2021 · 7 comments · Fixed by #506
Labels
enhancement New feature or request

Comments

@matthewdavidson
Copy link

Describe the problem

Modifications to session after login are overwritten over time

What was the expected behavior?

If code exists to modify the session after login like so (following the example in the docs):

const afterCallback = (req, res, session, state) => {
  delete session.idToken;
  return session;
};

export handleAuth({
  async callback(req, res) {
    try {
      await handleCallback(req, res, { afterCallback });
    } catch (error) {
      res.status(error.status || 500).end(error.message);
    }
  }
});

Then those modifications should persist indefinitely, irrespective of the session being updated at a later point in time. I'm not exactly sure what is causing the session cookie contents to be updated, it could be either:

  • Because rolling: true in session config
  • Because offline_access has been added to the scope param to benefit from refresh tokens

Either way the code written in afterCallback obviously has no bearing on the code that updates the session after the fact.

Reproduction

  • Enable either rolling sessions or add the offline_access scope in the authorisation params
  • Follow the guide in the docs to modify the session after login - e.g. remove the idToken
  • after a certain amount of time (perhaps when the access token expires?) the session will be updated and now include the idToken

Environment

  • Version of this library used: 1.3.1
  • Which framework are you using, if applicable: Nextjs 10.2.3
@Widcket
Copy link
Contributor

Widcket commented Jun 11, 2021

Hi @matthewdavidson, thanks for raising this. I'm afraid modifying the session is not supported when using refresh tokens - see #323 (comment)

@Widcket Widcket added the question Further information is requested label Jun 11, 2021
@matthewdavidson
Copy link
Author

matthewdavidson commented Jun 11, 2021

@Widcket,

If there is an explicit api available to modify the session (e.g. afterCallback), users will expect it to work in all use cases. Therefore I reckon it's worth updating the documentation to reflect this caveat / stance.

Given that session construction happens in multiple places (i.e. not limited to the callback handler) then perhaps the consumer API to modify sessions (afterCallback) shouldn't be located at the callback level and instead should be lifted up higher. Perhaps at the config level?

@Widcket Widcket added enhancement New feature or request and removed question Further information is requested labels Jun 11, 2021
@Widcket
Copy link
Contributor

Widcket commented Jun 14, 2021

cc @adamjmcgrath

@adamjmcgrath
Copy link
Contributor

Hi @matthewdavidson - thanks for raising this

Either way the code written in afterCallback obviously has no bearing on the code that updates the session after the fact.

When you refresh the Access Token, you get a new ID Token. This is why deleting session.idToken appears to be reverted after the access token expires.
Other edits, like adding a custom property would not be overwritten.

We could have a hook that runs whenever the session is updated, but it would have to be different toafterCallback as this is specfically designed to run before a session is instantiated. A session is already instantited when an Access Token is refreshed for example, so there are some cases where the code in afterCallback wouldn't apply.

For this specific case, deleting the ID Token from the session, you'll need to remove the idToken from the session after calling getAccessToken as well as in afterCallback, for example:

const onSessionUpdated = (session) => {
  delete session.idToken;
};

const afterCallback = (req, res, session, state) => {
  onSessionUpdated(session);
  return session;
};

const myGetAccessToken = async (req, res) => {
  const accessToken = await getAccessToken(req, res);
  onSessionUpdated(getSession(req, res))
  return accessToken;
};

@blazzy
Copy link

blazzy commented Aug 17, 2021

The update to the cookie size FAQ still seems wrong.

const afterCallback = (req, res, session, state) => {
  delete session.user.unusedClaim; // This gets persisted by the SDK
  return session;
};

The user data change doesn't get persisted either. At least not in the long run. Looking at the getAccessToken logic it looks like the entire user part of the session being updated on refresh.

    Object.assign(session, {
      ...newSession,
      refreshToken: newSession.refreshToken || session.refreshToken,
      user: { ...session.user, ...newSession.user }
    });

@RobSchilderr
Copy link

Hi @matthewdavidson, thanks for raising this. I'm afraid modifying the session is not supported when using refresh tokens - see #323 (comment)

has this changed in 2022 or its still like that?

@adamjmcgrath
Copy link
Contributor

adamjmcgrath commented Jan 28, 2022

Hi @RobSchilderr - the quote you've shared is a a little far ranging out of context. But yes, the following is still correct:

When you refresh the Access Token, you get a new ID Token. This is why deleting session.idToken appears to be reverted after the access token expires.
Other edits, like adding a custom property would not be overwritten.

If you want to remove the ID Token from the session, you'd need to do it after refreshing the token set - using the example provided in #416 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants