Add universal required email configuration for authentication #3047
Conversation
| profile.email.endsWith(`@${process.env.TOOLPAD_GOOGLE_AUTH_DOMAIN}`)), | ||
| (requiredEmails.length === 0 || | ||
| requiredEmails.some( | ||
| (requiredEmail) => new RegExp(requiredEmail).test(profile.email!) ?? false, |
There was a problem hiding this comment.
I'm not a huge fan of using regexes for this. Seems a bit like a footgun to me. e.g. users forgetting to add $ on the end, or forgetting to escape .. e.g. someone naively configures .*@foo.com and then me coming in and buying the domain "fooxcom123.com" and getting access to your server.
Wouldn't >95% of the usecases be covered by just restricting the email domain? Can you benchmark other products on how configurable this is?
There was a problem hiding this comment.
Ok I'll give a look into how other tools do it, but last case scenario we can just check if the user email ends with the specified domain preceded by @, instead of regex.
| @@ -27,6 +31,42 @@ export function createAuthHandler(base: string): Router { | |||
| GithubProvider({ | |||
There was a problem hiding this comment.
Does this provider have to be recreated on every request? Would it make sense to put it in its own module?
There was a problem hiding this comment.
I was just following some examples and didn't consider that, it should be better to move it outside, will do!
There was a problem hiding this comment.
Things didn't work when I moved the provider to a separate variable, I think it should be fine to do it like this as that's how the examples I have seen work too.
There was a problem hiding this comment.
Things didn't work when I moved the provider to a separate variable
Any idea why? This seems like a strange thing. Can you investigate?
There was a problem hiding this comment.
Sure I'm taking a look at how it works internally, this library has some weird things sometimes though, it seems to be a bit of a work in progress and most things aren't documented at all.
There was a problem hiding this comment.
Isn't the environment set with this.envManager.start()? This runs after the auth.ts file is imported, that's why the variables that need to be loaded aren't there for a global variable in that file.
There was a problem hiding this comment.
Maybe we can load the Toolpad app's environment before any imports when the server starts if you think it's worth it. Sounds like an unrelated thing though, I don't see what's wrong with initializing the providers in createAuthHandler.
There was a problem hiding this comment.
I can also load the providers in a global function if that's better.
There was a problem hiding this comment.
Maybe we can load the Toolpad app's environment before any imports when the server starts if you think it's worth it
Just
import 'dotenv/config'before anything else
There was a problem hiding this comment.
Ok, it works if I add that line to toolpadAppServer.ts, as that file seems to run before index.ts - it might be because the worker imports that file first, I can look deeper into it and try in another file if this one doesn't seem ideal for any reason.
07015a0
|
I've added the change from regex patterns to using domains only, and kept the rest, let me know if this is good to merge! |
|
Renaming the property to |
| @@ -0,0 +1,5 @@ | |||
| export declare module '@auth/core/types' { | |||
| interface Profile { | |||
| verifiedEmails?: string[]; | |||
There was a problem hiding this comment.
If this isn't public API, should we really rely on it?
There was a problem hiding this comment.
With your suggestion verifiedEmails shouldn't be needed anymore so I'll remove it, it was only for verifying the restricted domains with the Github provider.
|
|
||
| if (githubRes.ok) { | ||
| const emails: GitHubEmail[] = await githubRes.json(); | ||
| profile.email = (emails.find((e) => e.primary) ?? emails[0]).email; |
There was a problem hiding this comment.
Can't we just look for the first verified email that satisfies our restrictedDomains setting and use that as the email? Then set the email_verified boolean to true?
There was a problem hiding this comment.
That was the default logic but it would be an improvement, i'll change it and fallback to primary email.
There was a problem hiding this comment.
I've added the changes here fd18d7c, let me know if it's good now!
apedroferreira
force-pushed
the
auth-required-email
branch
from
67b6692
to
fd18d7c
Compare
Make it possible to add multiple email address patterns to be matched by authenticated users, either for the Github or Google authentication providers.
Also takes in consideration all verified emails in Github.
Also makes the authentication/authorization dialog be tabbed, brought this from #3002.
Untitled.mov