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

Add autoSave/touchSession for rolling session expiry management #1116

Merged
merged 5 commits into from
Mar 27, 2023

Conversation

aovens-quantifi
Copy link
Contributor

@aovens-quantifi aovens-quantifi commented Mar 16, 2023

  • All new/changed/fixed functionality is covered by tests (or N/A)
  • I have added documentation for all new/changed functionality (or N/A)

📋 Changes

The library does not currently allow getting a session without touching that session (assuming rolling is enabled). In order to properly implement a session expiry modal, there needs to be an ability to get the session expiry time without causing the expiry to be updated. This PR adds a config setting (autoSave) that allows disabling automatically updating the session when getting a session. To restore rolling capabilities, a touchSession function has been added that will allow control over when the session expiry is updated

📎 References

🎯 Testing

Unit tests should fully cover this change

@aovens-quantifi aovens-quantifi requested a review from a team as a code owner March 16, 2023 18:37
@vercel
Copy link

vercel bot commented Mar 16, 2023

@aovens-quantifi is attempting to deploy a commit to the Auth0 Team on Vercel.

A member of the Team first needs to authorize it.

@adamjmcgrath
Copy link
Contributor

adamjmcgrath commented Mar 17, 2023

Thanks for raising this @aovens-quantifi - it's a nice idea

Your option will not have any effect if getSession(req, res, false) is not the first time the session is accessed, which would make it confusing to use.

eg.

withApiAuthRequired(function(req, res) { // cookie already updated
  const session = await getSession(req, res, false);
});
// or
function(req, res) {
  const at = await getAccessToken(req, res); // cookie already updated
  await getSession(req, res, false);
};

I think it would potentially be a good feature, so I'm happy to work on a solution with you. Just so I know the background, what problem are you trying to solve that you can't just use rolling=false? Presumably you want the session duration to be based on the user's last activity but don't want every request to update the cookie expiry? What is your criteria for requests that should update the cookie expiry and requests that should't?

@aovens-quantifi
Copy link
Contributor Author

Hey @adamjmcgrath, thanks for the reply. Good catch on the request caching. You're right that every session access for a patlrticular route would need this flag set to false.

Ultimately I only need a single route to have this set to false. I need to be able to retreive the current session expiry without causing it to roll forward.

We're using a SPA written in next, and so any async chatter to the server or activity in another tab will not update my tab's session expiry value. Therefore, before I show the session expiry modal, I need to figure out if it is actually about to expire. To do this I need to get the current value without affecting the current value

@aovens-quantifi
Copy link
Contributor Author

I think allowing this via a request header might makef the most sense. Another use case would be if a page has infinite polling. I would argue the polling should not count as "user activity" for the purposes of session timeout

@adamjmcgrath
Copy link
Contributor

Hi @aovens-quantifi - thanks for sharing that. Yep, definitely agree this would be a useful feature.

express-session and next-session have configuration options that disable/enable the auto saving session feature that we do when rolling=true (https://expressjs.com/en/resources/middleware/session.html#resave and autoCommit).

They then both expose a touchSession type method (https://expressjs.com/en/resources/middleware/session.html#sessiontouch and https://www.npmjs.com/package/next-session#user-content-sessiontouch) and if resave/autoCommit is false - it's up to the operator to call touchSession to bump the updated-at time of the session.

I would be happy to go for something like that. e.g.

// autoSave default is true (and should only be set to `false` when rolling=true)
const sessionConfig: SessionConfig = { rolling: true, autoSave: false };

// if autoSave is `false` you need to call `touchSession` to update the session
import { touchSession } from '@auth0/nextjs-auth0';

export default api = (req, res) => {
  await touchSession(req, res);
  return { ... };
}

@aovens-quantifi
Copy link
Contributor Author

I like it! I'll implement :)

@adamjmcgrath
Copy link
Contributor

Great! Thanks @aovens-quantifi! Feel free to reach out on this thread if you have any questions

@aovens-quantifi
Copy link
Contributor Author

@adamjmcgrath This is ready to go :)

@adamjmcgrath
Copy link
Contributor

That was quick - and it looks great! Thanks @aovens-quantifi! I wont be able to get to this today, but I'll look at it first thing next week (also I'm travelling next week, so it may be a little slow - apologies). In the meantime, could you remove the /docs changes - I'll generate them when I do a release. Cheers 👍

Copy link
Contributor

@adamjmcgrath adamjmcgrath left a comment

Choose a reason for hiding this comment

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

@adamjmcgrath adamjmcgrath merged commit e49e8be into auth0:main Mar 27, 2023
@adamjmcgrath adamjmcgrath changed the title Allow getting a session without updating the session Add autoSave/touchSession for manual rolling session expiry management Mar 27, 2023
@adamjmcgrath adamjmcgrath changed the title Add autoSave/touchSession for manual rolling session expiry management Add autoSave/touchSession for rolling session expiry management Mar 27, 2023
@adamjmcgrath adamjmcgrath mentioned this pull request Mar 27, 2023
@adamjmcgrath
Copy link
Contributor

Thanks for your work on this @aovens-quantifi - it got released in https://github.com/auth0/nextjs-auth0/releases/tag/v2.4.0

@clyncha
Copy link

clyncha commented Jan 12, 2024

@adamjmcgrath @aovens-quantifi This fixes an issue I am having with adding an auth0 token onto a graphql call from a server component. But I was wondering if this will impact refresh tokens.

@aovens-quantifi
Copy link
Contributor Author

Can you explain your use case a bit more? This feature shouldn't have much effect since the default (when using rolling sessions) is to "touch" the session every time it is accessed. This feature inverts that by only "touching" the session when touchSession is explicitly called

@clyncha
Copy link

clyncha commented Jan 12, 2024

Yeah no problem.

I'm having the same issue but with using getAccessToken() in apolloClient.ts. I am setting up an auth link and using getAccessToken() to get the token to set on the header.

 const authenticationLink = setContext(async (_, { headers }) => {
   const { accessToken } = await getAccessToken();
    return {
      headers: {
        ...headers,
        Authorization: `Bearer ${accessToken}`,
      },
    };
 });

I'm fetching an GraphQL query in one of my server components.

I came across the PR from this stackoverflow
https://stackoverflow.com/questions/76813923/how-to-avoid-warning-message-when-getting-user-information-on-next-js-13-server/77015385#77015385

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.

3 participants