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-3554] Next.js Middlware support #815

Merged
merged 14 commits into from
Sep 13, 2022
Merged

[SDK-3554] Next.js Middlware support #815

merged 14 commits into from
Sep 13, 2022

Conversation

adamjmcgrath
Copy link
Contributor

@adamjmcgrath adamjmcgrath commented Sep 9, 2022

  • 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

Added a new withMiddlwareAuthRequired helper, to protect routes using middleware.

To protect all your routes:

// middleware.js
import { withMiddlewareAuthRequired } from '@auth0/nextjs-auth0/middleware';

export default withMiddlewareAuthRequired();

To protect specific routes:

// middleware.js
import { withMiddlewareAuthRequired } from '@auth0/nextjs-auth0/middleware';

export default withMiddlewareAuthRequired();

export const config = {
  matcher: '/about/:path*',
}

For more info see: https://nextjs.org/docs/advanced-features/middleware#matching-paths

To run custom middleware for authenticated users:

// middleware.js
import { withMiddlewareAuthRequired, getSession } from '@auth0/nextjs-auth0/middleware';

export default withMiddlewareAuthRequired(async function middleware(req) {
  const res = NextResponse.next();
  const user = await getSession(req, res);
  res.cookies.set('hl', user.language);
  return res;
});

📎 References

See #525
https://nextjs.org/docs/advanced-features/middleware

🎯 Testing

Run kitchen-sink example app
Visit http://localhost:3000/profile-mw

@adamjmcgrath adamjmcgrath added the review:large Large review label Sep 9, 2022
@adamjmcgrath adamjmcgrath requested a review from a team as a code owner September 9, 2022 15:35
@vercel
Copy link

vercel bot commented Sep 9, 2022

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

1 Ignored Deployment
Name Status Preview Updated
nextjs-auth0 ⬜️ Ignored (Inspect) Sep 13, 2022 at 6:24AM (UTC)

@adamjmcgrath adamjmcgrath changed the title Mw [SDK-3554] Next.js Middlware support Sep 9, 2022
"!<rootDir>/src/handlers/auth.ts",
"!<rootDir>/src/auth0-session/config.ts",
"!<rootDir>/src/auth0-session/index.ts",
"!<rootDir>/src/auth0-session/session-cache.ts"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New coverage instrumenter c8 doesn't like files with only TS definitions. (was needed because istanbul instrumented code doesn't run on edge-runtime)

Joi.binary().min(8),
Joi.array().items(Joi.string().min(8), Joi.binary().min(8))
]).required(),
secret: Joi.alternatives([Joi.string().min(8), Joi.array().items(Joi.string().min(8))]).required(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Joi.binary (Buffer check) doesn't work on Edge MW

}
return session;
};
const idTokenValidator =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whitespace changes from eslint --fix

Copy link
Contributor

@Widcket Widcket left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple minor suggestions.

@@ -11,6 +11,7 @@
"files": [
"dist",
"src",
"middleware.js",
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a lone .js file, doesn't it need type defs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, good point - I didn't realise you had to do that. Will fix

Copy link
Contributor

@Widcket Widcket Sep 12, 2022

Choose a reason for hiding this comment

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

You also have to include the typings file here –otherwise it will not be present in the package because it's not inside src or dist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

d'oh, yep - good point 👍

Widcket
Widcket previously approved these changes Sep 12, 2022
Copy link
Contributor

@Widcket Widcket left a comment

Choose a reason for hiding this comment

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

Sorry, just noticed that the typings file also needs to be included in the files array in the package.json –otherwise it will not be present in the package because it's not inside src or dist.

@adamjmcgrath adamjmcgrath merged commit c07fa26 into vNext Sep 13, 2022
@Jared-Dev
Copy link

Not sure if this is the correct place to put this, but I believe I'm seeing some typing issues with the latest version of Next on the vNext branch.

When using a copy/paste example from the Next docs I'm seeing the below.
image
image

@adamjmcgrath
Copy link
Contributor Author

adamjmcgrath commented Oct 6, 2022

Hi @Jared-Dev - thanks for testing this out!

Looks like you're using npm link and the version of next installed in Sites/packages/nextjs-auth0 is incompatible with the version installed in Sites/ox-portal/ - if you use npm pack to package the SDK (or install from npm when the beta is available - v soon) you will only have one next dependency so shouldn't run into this problem.

if (cookies.length || authCookies.length) {
headers.set('set-cookie', [...authCookies, ...cookies].join(', '));
}
return NextResponse.next({ ...res, headers });
Copy link

@divmgl divmgl Oct 20, 2022

Choose a reason for hiding this comment

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

This is not working as intended for redirects, for some reason. We'll need a test around this as well. See #874.

Suggested change
return NextResponse.next({ ...res, headers });
return NextResponse.next({ ...res, headers, status: res.status });

I really am at a loss as to why this is happening, given that ...res should bringing in the status property already... Here's the transpiled version:

                return [
                  2 /*return*/,
                  server_1.NextResponse.next(
                    tslib_1.__assign(tslib_1.__assign({}, res), {
                      headers: headers,
                      status: res.status
                    })
                  )
                ]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review:large Large review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants