-
Notifications
You must be signed in to change notification settings - Fork 3
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
Proof-of-concept: Support OAuth and OIDC auth providers #243
base: main
Are you sure you want to change the base?
Conversation
@@ -1,5 +1,6 @@ | |||
/// <reference types="next" /> | |||
/// <reference types="next/image-types/global" /> | |||
/// <reference types="next/navigation-types/compat/navigation" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Not auth related. Added due to Next.js App router being utilized in this branch
app/src/app/layout.tsx
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Added due to Next.js App router being utilized in this branch
app/tsconfig.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Changes due to Next.js App router being utilized in this branch
app/src/app/user/page.tsx
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this could be a component or added to the homepage or wherever, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, any server component that needs to reference session details would just do const session = await auth()
@sawyerh you are a 🧙 |
app/src/app/layout.tsx
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Not auth related. Added due to Next.js App router being utilized in this branch
app/tsconfig.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ Not auth related. Added due to Next.js App router being utilized in this branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great. Pretty much exactly what I would have hoped for.
The main open question I have is how often we'd be able to leverage an OpenID provider. Often the agencies we work with have SSO in their roadmap but it's often not yet ready at the time we're developing the application. Part of me wonders if it's a chicken and egg issue. One thought is that if we can also prototype the "egg" then we might be able to kickstart agencies' efforts to develop SSO.
Two high level paths (not mutually exclusive) for providing agencies with a path to have a reusable identity provider:
- create a custom provider for login.gov, which you already noted below (this doesn't solve the issue directly, but makes it easier to adopt should the agency choose to go with login.gov)
- create a template for an identity provider solution using a white label service like okta. we could create something like a template-identity-provider repo that contains everything needed for it, including the infra and instructions for spinning it up. and so if an agency doesn't yet have SSO, they could spin that up alongside with this application that can then leverage that OpenID. feed two birds with one scone, baby.
/** | ||
* Not super sure why this needs to be called on the client-side, but at the | ||
* moment the Auth.js signOut method has a dependency on `window`, at a minimum. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my guess is they need to clear some cookies, which only get sent to the matching domain by the browser for security reasons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this is a proof of concept, but just sketching out some high level design notes as a reminder for myself if/when we implement it into the template:
We'd want to break up auth.ts into 3 modules, each responsible for one thing:
- a module that wraps/integrates with NextAuth (that's this one)
- a module that serves as an adapter for Cognito (looks like
next-auth/providers/cognito
is exactly this so in this case i don't think there's a need to make a wrapper for this) - an initialization module (or we can abstract it into a config file) that's specific to the current application that initializes the provider (e.g. Cognito) and any necessary auth configuration (e.g. session length or whatever) specific to the app
so basically the line of code that does providers: [CognitoProvider]
should be factored out into it's own file, and that's the only file that project teams should really care about or touch.
* | ||
* @see https://authjs.dev/getting-started/providers/oauth-tutorial | ||
*/ | ||
export { GET, POST } from "src/auth"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks a little different from the snippet in the tutorial, which looks like
export default NextAuth({
providers: [ ... ],
})
just curious for my own understanding the choice to do it this way. my guess is because you wanted to use the new App Router and the NextAuth snippet is using the old method of routing?
second question, also for my edification: since my typescript is a bit rusty but i don't see how the type exported from src/auth matches, it looks like src/auth exports nvm figured this part out{ handlers: { GET, POST } }
, but here we have { GET, POST }
(without handlers
), so how does that work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this branch is using the 5.0 beta version so some of the existing docs are out of date
|
||
export const { | ||
/** API route handlers */ | ||
handlers: { GET, POST }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for these questions, but I can't for the life of me figure out where GET
and POST
are defined. I don't see it anywhere in this file, nor do I see anything like an import *
in python.
Is this a type hint? (if so i find it confusing that type hints involve an equal sign)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh... i get it. the = NextAuth(...)
is populating module.exports with a bunch of attributes from NextAuth.
for my own mental note/remind:, i imagine once we move past proof of concept and clean things up a bit, we'd make it more explicit, like:
const nextAuth = NextAuth({ ... })
export const {
handlers: { ... },
auth,
GET: nextAuth.GET,
POST: nextAuth.POST,
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good feedback – the syntax is definitely funky. I agree we can make it more explicit when cleaning things up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- create a custom provider for login.gov, which you already noted below (this doesn't solve the issue directly, but makes it easier to adopt should the agency choose to go with login.gov)
- create a template for an identity provider solution using a white label service like okta. we could create something like a template-identity-provider repo that contains everything needed for it, including the infra and instructions for spinning it up. and so if an agency doesn't yet have SSO, they could spin that up alongside with this application that can then leverage that OpenID. feed two birds with one scone, baby.
@lorenyu I like both of these ideas! Just to confirm my understanding, these wouldn't be dependencies for this proof-of-concept to move forward right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct, i don't think either of these ideas are blockers. it might just have to be a bit higher touch in the beginning as project teams might need some guidance on what to do with the standalone module and what options they have with it. if they go either the login.gov route or the new IDP route, might even be able to leverage that.
Warning
This is just a proof-of-concept. There are a number of TODO's that would need cleaned up before merging.
Ticket
Resolves #235
Changes
next-auth
with an example Cognito integrationInitial impressions of Auth.js
Open questions
Testing
To test this locally:
app/.env.local
. You can copy the values from 1password, see "Next.js platform env.local (next-auth)"auth-js-poc-demo.mp4