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

[SDK-3332] Constrain session lifecycle to withPageAuthrequired to avoid Next warning #664

Merged
merged 5 commits into from
May 20, 2022

Conversation

adamjmcgrath
Copy link
Contributor

Description

Next.js >=12 have started to issue a warning (You should not access 'res' after getServerSideProps resolves.), and in some cases an error, when you access the response object after getServerSideProps has run.

Because this SDK provides a rolling session by default, it writes to the response at the end of every request. It does this at the end of the request using on-headers, which causes the warning because on-headers executes after getServerSideProps resolves.

We can fix this in withAuthenticationRequired by not using on-headers and writing to the headers just before withAuthenticationRequired completes, eg.

async function withAuthenticationRequired({ getServerSideProps }) {
  session.get(disableOnHeaders=true) // read cookie and disable on-headers
  //  ... existing code ...
  session.save() // write cookie (because on-headers is disabled)
}

If you want to use getSession (and getAccessToken) without requiring auth on your route, you'll still need to use the withAuthenticationRequired wrapper, but you can do this now with the authRequired: false option. eg

export const getServerSideProps = withAuthenticationRequired({
  authRequired: false,
  async getServerSideProps(ctx) {
    const session = getSession(ctx.req, ctx.res);
    if (session) {
      // user is logged in
    } else {
      // user is not logged in
    }
  }
});

I also fixed an issue in withAuthenticationRequired when props is a Promise, eg

function getServerSideProps() {
  return { props: Promise.resolve({}) }
}

References

https://nextjs.org/docs/messages/gssp-no-mutating-res
fixes #524

Testing

Run the basic example app and notice that there is no warning when visiting /protected

Checklist

  • I have added documentation for new/changed functionality in this PR or in auth0.com/docs
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not main

@adamjmcgrath adamjmcgrath added the review:medium Medium review label May 13, 2022
@vercel
Copy link

vercel bot commented May 13, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated
nextjs-auth0 ⬜️ Ignored (Inspect) May 18, 2022 at 3:24PM (UTC)

@adamjmcgrath adamjmcgrath changed the title Constrain session lifecycle to withPageAuthrequired to avoid Next warning [SDK-3332] Constrain session lifecycle to withPageAuthrequired to avoid Next warning May 13, 2022
@adamjmcgrath adamjmcgrath marked this pull request as ready for review May 18, 2022 10:43
@adamjmcgrath adamjmcgrath requested a review from a team as a code owner May 18, 2022 10:43
* @category Server
*/
export type WithPageAuthRequiredOptions<P = any, Q extends ParsedUrlQuery = ParsedUrlQuery> = {
getServerSideProps?: GetServerSideProps<P, Q>;
returnTo?: string;
authRequired?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is an authRequired option on withAuthRequired the best way to go? It kind of defeats the entire purpose of withAuthRequired. Should we go with a different wrapper instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about that (eg withPageAuth or withSessionInitialised etc) - but decided this would be the easiest to grok

Copy link
Contributor

@Widcket Widcket May 18, 2022

Choose a reason for hiding this comment

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

I'm not sure that should determine the design of this feature (documenting it well along with a clear name can make it easy to grok as well). I'll leave it up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me give it another thought, I'll see if I can come up with a good name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went for getServerSidePropsWrapper in e523099 (wait until after your day off to take a look :P)

src/index.ts Show resolved Hide resolved
Widcket
Widcket previously approved these changes May 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review:medium Medium review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

warn - You should not access 'res' after getServerSideProps resolves.
2 participants