-
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
ADR - Use NextAuth #102
base: main
Are you sure you want to change the base?
ADR - Use NextAuth #102
Conversation
|
||
|
||
|
||
## Example Implementation |
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 left an example in here. It's not a true implementation, in that I didn't verify it works as I don't have access to Login.gov API's. Assuming you have the proper secrets and domains in the content below, I don't see why it wouldn't work though. A potential improvement is mapping all of the login.gov attributes to typescript values to use in the return function.
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.
LMK if we should scrap it, keep it, or move it somewhere else.
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.
on the fence / don't feel super strongly about it.
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 think it's a useful reference, but I think we should mention in here that it's pseudo-code/untested.
|
||
## Decision Outcome | ||
|
||
In-progress |
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.
ℹ️ Will update based on feedback and decisions.
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.
@brandon-lent sorry for the late review, and thanks for putting this together!
after reading through and thinking about it more, i feel like i can't really recommend a solution until we have some proof of concepts.
some possible paths forward, we could explicitly call out next steps in the ADR to implement some proof of concepts and merge the ADR as an in progress / proposed state.
and then we can update the ADR in the future once we have more info from the technical spikes.
or alternatively we can put this on the backburner for now and have somebacklog tickets to do some spikes.
|
||
## Context and Problem Statement | ||
|
||
A common requirement for government projects is working with an identity provider (Such as login.gov or AWS Cognito). It isn't uncommon for authentication providers to be changed during the lifecycle of a project. We want to ease the concern of being "locked in" to one provider. [NextAuth](https://next-auth.js.org/) fills this need by allowing us to easily swap identity providers and providing helpful functions for retrieving user data. |
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 sure i agree with the part about
It isn't uncommon for authentication providers to be changed during the lifecycle of a project.
i think the goal is less about making it easy to switch auth providers, and more about eliminating the dependency of the rest of the template on any particular auth provider, so that project teams can use the Next.js application template even if they use a non-default provider by simply implementing a new adapter rather than needing to refactor the parts of the application that rely on auth functionality.
|
||
|
||
|
||
## Example Implementation |
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.
on the fence / don't feel super strongly about it.
also curious for @sawyerh 's opinion on this too |
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 agree with Loren's comment on the problem statement. I think we should probably update that part before merging.
we could explicitly call out next steps in the ADR to implement some proof of concepts and merge the ADR as an in progress / proposed state.
I like this idea too. I feel pretty good about recommending NextAuth, so merging this ADR as "proposed" + (someone) doing a proof-of-concept in a follow-up PR feels like a good next step.
I think you could potentially repurpose your code example at the bottom as a "Next steps" section, mentioning that we'd like to explore a proof of concept and "here's a pseudo example of what we could try"
3. Seamless integration with Next.js: NextAuth is designed to work seamlessly with Next.js applications. It provides a simple way to handle authentication in server-rendered and client-side rendered pages, ensuring a smooth user experience. | ||
|
||
Cons: | ||
1. Dependency on Next.js: NextAuth is tightly coupled with Next.js framework. If you are not using Next.js, integrating NextAuth into your existing stack may require additional effort. |
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 used to be true but they've recently pivoted to "Auth.js" and made it framework-agnostic: https://authjs.dev/ — for this template though, we'd still use the next-auth
package.
|
||
|
||
|
||
## Example Implementation |
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 think it's a useful reference, but I think we should mention in here that it's pseudo-code/untested.
Update: proof-of-concept created #243 |
Ticket
#179
Changes
Created a new ADR that:
Context for reviewers
Most projects at Nava eventually require integration with an authentication provider. Using NextAuth simplifies this for us as it allows us to easily integrate authentication into Next.js applications.
For more context, we have a thread about this in slack here.