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

Support Azure AD authentication: Implementation #1311

Merged
merged 28 commits into from
Dec 3, 2020
Merged

Support Azure AD authentication: Implementation #1311

merged 28 commits into from
Dec 3, 2020

Conversation

jeliasson
Copy link
Contributor

@jeliasson jeliasson commented Oct 8, 2020

This PR seeks to implement Azure AD as authentication in the @redwoodjs/auth package.

See #1310

@jeliasson jeliasson mentioned this pull request Oct 8, 2020
9 tasks
@dthyresson dthyresson self-requested a review October 9, 2020 11:53
@dthyresson dthyresson self-assigned this Oct 9, 2020
@jeliasson jeliasson marked this pull request as draft October 12, 2020 09:01
@peterp
Copy link
Contributor

peterp commented Oct 29, 2020

@jeliasson You've done amazing work here, thanks @dthyresson for reviewing this :)

@jeliasson
Copy link
Contributor Author

jeliasson commented Nov 8, 2020

Hey all 🖐

@dthyresson I have implemented token validation (using Auth0 decoder example, thanks for pointing me in that direction!) and a better auth template for Azure Active Directory which picks up the roles nicely. By any chance you can have another review at your convenience? 🙏

@thedavidprice By any chance you can estimate time on the setup command and migration from generate as of #1307 #1309? Also, is there any chance to maybe merge this into a canary release? I'm about to start a new project based of Redwood, and that project will require this authentication method. Would like to target that canary release and do battle tests 😊

Latest documentation can be seen on this preview. Will need some more work tho.

Finally, is there any (long-term) "strategy" what various token claims that should be surfaced to currentUser? In the auth generator I bring in everything from ...decoded and adding email from decoded.prefered_username. Thoughts on this?

/cc @peterp

@jeliasson jeliasson marked this pull request as ready for review November 8, 2020 20:28
@Tobbe
Copy link
Member

Tobbe commented Nov 9, 2020

@jeliasson I'm sure @thedavidprice wouldn't mind some help with #1307 and #1309. If you want to jump in and do some work on those I'd be happy to review/merge to give you a canary to try 🙂

@jeliasson
Copy link
Contributor Author

jeliasson commented Nov 9, 2020

I'm sure @thedavidprice wouldn't mind some help with #1307 and #1309. If you want to jump in and do some work on those I'd be happy to review/merge to give you a canary to try 🙂

@Tobbe Unfortunately I'm swamped with work this month, otherwise I would catch up on those and jump right in. I would like to focus on the authentication, test in a real-life application from a canary, and I'll be happy to contribute to setup in December. 📌

If canary is not possible, is there any nice way of patching something under node_modules? I.e. manually bring in the changes. This would help greatly in the project I'm about to spin up. Thanks!

@Tobbe Tobbe mentioned this pull request Nov 9, 2020
@Tobbe
Copy link
Member

Tobbe commented Nov 9, 2020

If canary is not possible, is there any nice way of patching something under node_modules? I.e. manually bring in the changes. This would help greatly in the project I'm about to spin up. Thanks!

@jeliasson You could voice your support for the issue I just created 🙂 #1458

@github-actions
Copy link

github-actions bot commented Nov 25, 2020

@andrew-hwahin
Copy link
Contributor

Hope this will be released soon😁🙏🙇‍♂️

Copy link
Member

@Tobbe Tobbe left a comment

Choose a reason for hiding this comment

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

Now that I've worked with this code (#1536) I feel I can read this and provide some feedback. Mostly nitpicking comments, but since you'll need to handle merging this with the new Supabase stuff anyway, I figured you could take a look at my minor comments as well.

Another thing you could do, that I forgot when I did the Supabase stuff, is update the comment at the end of the auth README, with the list of auth providers (please add Supabase as well if you do 🙂)

If you fix the comments and rebase your code I'll make sure we get this merged. No need to wait for @thedavidprice's setup stuff. We can move this over to setup later together with every other auth generator.

packages/api/src/auth/decoders/azureActiveDirectory.ts Outdated Show resolved Hide resolved
packages/api/src/auth/decoders/azureActiveDirectory.ts Outdated Show resolved Hide resolved
packages/api/src/auth/decoders/azureActiveDirectory.ts Outdated Show resolved Hide resolved
packages/auth/src/authClients/azureActiveDirectory.ts Outdated Show resolved Hide resolved
packages/auth/src/authClients/azureActiveDirectory.ts Outdated Show resolved Hide resolved
packages/auth/src/authClients/azureActiveDirectory.ts Outdated Show resolved Hide resolved
packages/auth/src/authClients/azureActiveDirectory.ts Outdated Show resolved Hide resolved
packages/auth/src/authClients/azureActiveDirectory.ts Outdated Show resolved Hide resolved
jeliasson and others added 12 commits December 3, 2020 16:51
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
@Tobbe Tobbe merged commit 2986015 into redwoodjs:main Dec 3, 2020
@jeliasson
Copy link
Contributor Author

Thanks for the help @Tobbe and @dthyresson! ❤️

I'll bring in the canary release to redwoodjs/playground-auth#6 for final testing, and have another glance on the documentation in https://github.com/redwoodjs/redwoodjs.com/pull/407.

/ping @andrew-hwahin @AndrewLamYW

@AndrewLamYW
Copy link
Contributor

Million thanks for the effort!🙏❤️ @jeliasson @Tobbe @dthyresson

I think this will definitely drives more RedwoodJS adoption for projects which has the requirement to use AzureAD.

@thedavidprice thedavidprice added this to the Next Release milestone Jan 6, 2021
@jeliasson jeliasson deleted the feat/auth-azure-ad branch January 8, 2021 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants