-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Auth Hooks #1993
base: main
Are you sure you want to change the base?
Auth Hooks #1993
Conversation
@@ -0,0 +1,30 @@ | |||
import type { |
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.
Example usage of the hooks
@@ -40,7 +40,11 @@ app todoApp { | |||
}, | |||
}, | |||
onAuthFailedRedirectTo: "/login", | |||
onAuthSucceededRedirectTo: "/profile" | |||
onAuthSucceededRedirectTo: "/profile", | |||
onBeforeSignup: import { onBeforeSignup } from "@src/auth/hooks.js", |
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.
Declare the hooks in the auth
dict
const { accessToken } = await github.validateAuthorizationCode(code); | ||
return getGithubProfile(accessToken); | ||
}, | ||
getProviderTokens: ({ code }) => github.validateAuthorizationCode(code), |
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.
Refactor OAuth a bit to split getting tokens and getting user profile into two steps so it's easier to write a hook onAfterOAuthTokenReceived
import type { ProviderId, createUser } from '../../auth/utils.js' | ||
import { prisma } from '../index.js' | ||
|
||
type CommonInput = { |
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.
Each hook will receive hookName
, prisma
and Express's req
object.
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 suggest changing the name to CommonAuthHookParams
.
Also, consider whether this affects how the IDE displays the types and whether it's worth it. The user probably doesn't care what's common and what's not common - they just want to see which params they have.
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.
} & CommonInput | ||
|
||
/* On Before OAuth Redirect Hook */ | ||
export type OnBeforeOAuthRedirectHookFn = ( |
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.
Users have the ability to modify the OAuth redirect URL
const user = await createUser( | ||
providerId, | ||
providerData, | ||
// Using any here because we want to avoid TypeScript errors and | ||
// rely on Prisma to validate the data. | ||
userFields as any, | ||
) | ||
if (onAfterSignupHook) { |
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.
Hook
@@ -34,13 +35,19 @@ export function getSignupRoute({ | |||
}) | |||
|
|||
try { | |||
await createUser( | |||
if (onBeforeSignupHook) { |
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.
Hook
providerId, | ||
providerData, | ||
// Using any here because we want to avoid TypeScript errors and | ||
// rely on Prisma to validate the data. | ||
userFields as any | ||
) | ||
if (onAfterSignupHook) { |
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.
Hook
passwordResetSentAt: null, | ||
}); | ||
try { | ||
if (onBeforeSignupHook) { |
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.
Hook
// rely on Prisma to validate the data. | ||
userFields as any, | ||
) | ||
if (onAfterSignupHook) { |
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.
Hook
72dc06f
to
ff5ce18
Compare
" methods: {", | ||
" google: {}", | ||
" },", | ||
" onAuthFailedRedirectTo: \"/login\"", | ||
" onAuthFailedRedirectTo: \"/login\",", | ||
" onBeforeSignup: import { onBeforeSignup } from \"@src/auth/hooks.js\",", |
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've included hooks in the e2e test
unlines | ||
[ "entity SocialLogin {=psl", |
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've cleaned up the e2e tests not use old auth concepts 👍
2f39592
to
382f643
Compare
@sodic and I jumped on quick call to discuss some of the OAuth hooks since I felt I didn't design them to be that useful. We concluded the following:
|
382f643
to
9bcd660
Compare
@@ -77,13 +77,11 @@ const _waspConfig: ProviderConfig = { | |||
|
|||
return createOAuthProviderRouter({ | |||
provider, | |||
stateTypes: ['state'], | |||
optionalStateTypes: [], |
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.
Since the state
is always sent and we rely on that knowledge in the new hooks, I've refactored this a bit to reflect the fact that the state
will be always present in the oAuthState object.
setOAuthCookieValue(provider, res, 'state', state); | ||
result.state = state; | ||
} | ||
const state = generateState(); |
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.
Always set the state since it's always used and will always be used for each future OAuth provider.
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.
Gotta run now, will finish the review tomorrow morning.
Nice work with this!
|
||
For every auth hook you want to use, you need to declare it in the `auth` dict in your Wasp file: | ||
|
||
```wasp |
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.
You might want to consider adding a TS/JS switch here for consistency. I'm fine either way.
web/docs/auth/auth-hooks.md
Outdated
import { EmailPill, UsernameAndPasswordPill, GithubPill, GooglePill, KeycloakPill } from "./Pills"; | ||
import ImgWithCaption from '@site/blog/components/ImgWithCaption' | ||
|
||
Sometimes you need to do some action _before_ the user signs up or _after_ they sign up. Or you need to access the OAuth redirect URL or the OAuth access token. For these cases, you can use the Auth Hooks. |
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.
Examples would help here.
Also, I have a couple of suggestions for the style. Take the ones you want:
- some action -> something - "some action" sounds weird to me.
- Too many "or"s
- For these cases... -> Auth hooks can help you here.
I also suggesting adding a sentence that explains the term "hook" to avoid the "curse of knowledge". For example: ... Auth hooks can help you here. Use them to "hook into" different parts of Wasp's authentication flow
Also, consider using the Oxford comma where appropriate ("Use the oxford comma" from the style guide)
web/docs/auth/auth-hooks.md
Outdated
|
||
## Available Hooks | ||
|
||
For every auth hook you want to use, you need to declare it in the `auth` dict in your Wasp file: |
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.
For every auth hook you want to use, you need to declare it in the `auth` dict in your Wasp file: | |
You must first declare all desired hooks in your Wasp file: |
A little shorter while conveying the same message.
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'd drop the 'user' rectangle below "User account is created. It makes the graph look cleaner and I don't think we lose anything.
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've simplified the diagrams and dropped all of the "data" rects.
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.
The "Redirect to Google. Get auth token. User singup data is received" part is little confusing. It's the only text that contains several actions, but there's no delimiter.
Can we separate this into three separate actions, or put some commas in there?
Also, why do we need the rectangle with oauth: token
below if we can't use it? Can we drop that?
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've made it into a single action that reads okay.
Also, why do we need the rectangle with oauth: token below if we can't use it? Can we drop that?
I wanted to point out where the data that appears in the hooks comes from. But it was getting crowded so I dropped all the data rects.
type UserHookParamsToInternalHookParams<T> = T extends ( | ||
params: infer P, | ||
) => unknown | ||
? Omit<P, 'prisma' | 'hookName'> |
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.
There's some duplication here. We should query the names of these fields from CommonInput
.
I've changed my mind on what I've written below. While technically still true, the wrapper is too complex to make it worth it. I'm leaving the old text for posterity:
Warning
Outdated opinion
Also, the type is generic (suggesting all three functions/wrappers act in the same way on purpose), while the functions themselves are duplicated (suggesting that there's no knowledge duplication and the structural similarities are a coincidence).
I'm not sure which one it is, but I suggest making it consistent:
- If you want to treat the hooks independently, they shouldn't use a generic type
- If you want them all to conform to the same interface, make a generic wrapper.
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.
That would work we didn't have a param we want to keep from the CommonInput
which is req
.
Common input has hookName
, prisma
and req
, we want to keep out the first two. If I used keyof
to get the keys of the CommonInput
, I'd still need to write down the literal req
which would still be duplication.
Did I understand this comment correctly?
}) | ||
{=/ onBeforeSignupHook.isDefined =} | ||
{=^ onBeforeSignupHook.isDefined =} | ||
export const onBeforeSignupHook = undefined |
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 suggest using null
.
As for the bigger picture...
From the generated code's perspective, this is a little weird. We always have a check if(hook)
before calling a hook. But that check is (again, from the runtime's perspective) always unnecessary.
Once the code is generated, the hook either null
or a function. It feels weird to generate code that will perform this check considering that, at that point, we know already know the check's outcome.
I understand why you did it. You didn't want too much coupling and ugly template logic in the handler
file, but I'd argue that delegating already existing knowledge to the runtime instead of acting on it immediately isn't a huge improvement.
So, I propose a third solution. Instead of setting the internal hook functions to null
and undefined
, set them to a no-op (e.g., () => undefined
). For the one that returns a redirect URL, just return the original URL. Then, always call the hooks from the handler.
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've went with no-op and identity functions 👍 Great suggestion which makes the code cleaner.
|
||
export function setOAuthCookieValue( | ||
provider: ProviderConfig, | ||
res: ExpressResponse, | ||
stateType: StateType, | ||
stateType: RequiredStateType | OptionalStateType, |
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.
Seems like we could make a type StateType = RequiredStateType | OptionalStateType
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've simplified the logic so this no longer applies.
} | ||
|
||
/* On Before Signup Hook */ | ||
export type OnBeforeSignupHookFn = ( |
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.
Let's add it when it proves necessary.
} | ||
|
||
/* On Before Signup Hook */ | ||
export type OnBeforeSignupHookFn = ( |
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 suggest dropping the Fn
part, OnBeforeSignupHook
should be enough.
I know we have SeverSetupFn
and whatnot, but I'm not a fan of those either. I'd change them to SetUpServer
or something.
Of course, changing this know makes our public API names inconsistent. If you think keeping the consistency is more important, I'm all for it.
I'd probably sacrifice consistency in this case, but it's your call.
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.
Ok, finally done with everything 😄
I didn't try the playing around with the feature, I trust it works. Let me know if you still want me to try it out.
All in all, this is looking pretty nicely. I left a lot of comments mostly because of:
- The docs.
- Refactoring suggestions.
- The fact that I'm the only one reviewing this.
Some refactoring comments talk about the previous version of the code. Decide what you want to tackle and what not (I don't think any of them were a big deal).
As for the stuff that must be fixed, it's just one type error (ST
instead of OST
, you'll know when you find the comment). And please figure out why that's not failing anywhere.
aliasedExtImportToImportJson :: | ||
JsImportAlias -> | ||
Path Posix (Rel importLocation) (Dir ServerSrcDir) -> | ||
Maybe EI.ExtImport -> | ||
Aeson.Value | ||
aliasedExtImportToImportJson importAlias pathFromImportLocationToSrcDir maybeExtImport = GJI.jsImportToImportJson aliasedJsImport | ||
where | ||
jsImport = extImportToJsImport pathFromImportLocationToSrcDir <$> maybeExtImport | ||
aliasedJsImport = JI.applyJsImportAlias (Just importAlias) <$> jsImport |
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 got lost with all the import, js import, import json functions and types... but shouldn't this thing exist already?
We are already doing aliased imports for operations, so we should have this. Can you please look into it?
Also, the function's name seems a little off. It suggest it maps an aliased ext import to import json, but it does something different (maybe too many things).
Finally, this file exports some symbols no one else uses, and even has a function or two that aren't used at all. This is not at all related to your PR, but I suggest cleaning it up a little - this JS import stuff has gotten a little out of hand. We should try to tame 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 got lost with all the import, js import, import json functions and types... but shouldn't this thing exist already?
We are already doing aliased imports for operations, so we should have this. Can you please look into it?
The operations aliasing function is really a special case that imports symbols from the wasp/operations/sdk
vs. this function importing from user files. So I couldn't really use it here.
I believe the proper solution is to automate this i.e. #1865
): { [name in ST]: string } { | ||
const result = {} as { [name in StateType]: string } | ||
res: ExpressResponse | ||
): { [name in OST]: string } & { [name in RequiredStateType]: string } { |
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.
Why not use Records here? Did you want to document the name
property?
Also, why use a product type instead of a union in the key type (as you did when asserting result). All in all, this is how I'd write it:
Record<RequiredStateType | OST, string>
|
||
if (stateTypes.includes('codeVerifier' as ST)) { | ||
if (optionalStateTypes.includes('codeVerifier' as ST)) { |
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 ST
here should throw an error somewhere. Can you please check whether wasp start
and wasp build
let it through?
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've adjusted the code, so this is no longer relevant.
const result = {} as { | ||
[name in OptionalStateType]: string; | ||
} & { | ||
[name in RequiredStateType]: string; | ||
} & { | ||
code: string; | ||
}; |
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.
Most of the other two big comments apply to this function as well, I think.
export type RequiredStateType = 'state'; | ||
|
||
export function generateAndStoreOAuthState<ST extends StateType>( | ||
stateTypes: ST[], | ||
export type OptionalStateType = 'codeVerifier'; |
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.
Besides creating a union called State
(mentioned in #1993 (comment)), I also recommend specifying more information with these types and colocating all the knowledge we have on them.
For example, the value for both states is of type string
, but this knowledge is sprinkled throughout the functions. I recommend we list it explicitly:
export type RequiredOAuthState = {
state: string
}
export type OptionalOAuthState = {
codeVerifier: string
}
export type OAuthState = RequiredOAuthState & Partial<OptionalOAuthState>
And then you have both names and values of states defined in the same place. You can get extract just the names, just the values, or both from this type:
type OptionalOAuthStateType = keyof OptionalOAuthState
The types contain all knowledge and everything else just follows. The types functions use become simpler as well:
function generateCodeVerifier(): OAuthState['codeVerifier']
function generateAndStoreOAuthState<OST extends OptionalOAuthStateType>(
optionalStateTypes: OST[],
provider: ProviderConfig,
res: ExpressResponse
): OAuthState
@@ -32,37 +34,43 @@ export function generateAndStoreOAuthState<ST extends StateType>( | |||
return result; |
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.
Github doesn't allow me to select the function's entire body, but the comment applies to all of it.
Recently, when reading Effective TS, I learned a couple of new things we can apply here:
- Create objects all at once
- Know how to iterate over objects
I won't repeat the item's content here, but here's how the code looks like after we apply:
const shouldGenerateCodeVerifier = optionalStateTypes.includes(
'codeVerifier' as OST
)
const result = {
state: generateState(),
...shouldGenerateCodeVerifier && { codeVerifier: generateCodeVerifier() },
}
let key: keyof typeof result
for (key in result) {
setOAuthCookieValue(provider, res, key, result[key])
}
return result
If you implement some of the stuff from the state types comment, maybe it even makes sense to go full retard functional:
const stateGenerators: {
[name in keyof OAuthState]: () => OAuthState[name]
} = {
state: generateState,
codeVerifier: generateCodeVerifier,
}
// ...
const stateTypes: StateType[] = [...optionalStateTypes, 'state']
const result = stateTypes.reduce(
(result, stateType) => ({
...result,
[stateType]: stateGenerators[stateType](),
}),
{} as OAuthState
)
let key: keyof typeof result
for (key in result) {
setOAuthCookieValue(provider, res, key, result[key])
}
return result
This might be pushing it a little tho (although it is open-closed for new optional states). I'm just giving ideas, it's your call. The current version of the code is perfectly fine to me.
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've adjusted the code to be simpler and generate the result
all the once :D
userSignupFields: UserSignupFields | undefined; | ||
req: ExpressRequest; | ||
accessToken: string; | ||
oAuthState: { [name in RequiredStateType]: string }; |
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.
Another place where i'd put a record.
const redirectUri = await finishOAuthFlowAndGetRedirectUri({ | ||
provider, | ||
providerProfile, | ||
providerUserId, | ||
userSignupFields, | ||
req, | ||
accessToken, | ||
oAuthState, | ||
}) |
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 a lot of arguments spanning different levels of abstraction. I see why they're all necessary though.
Do you think it maybe makes sense grouping them into a couple of objects and passing them around like that? This will depend on semantic connection between the arguments (are they always dispatched together, is one batch used for one thing and the other batch for a different thing, etc.).
You'll judge this better than I can.
stateTypes: ST[], | ||
export type OptionalStateType = 'codeVerifier'; | ||
|
||
export function generateAndStoreOAuthState<OST extends OptionalStateType>( |
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 the purpose of the generic, you want to connect the argument's type to the return type.
However, using the generic complicates the rest of the code (for example, when defining callbacks such as getAuthorizationUrl
, you can't just use OAuthState
but must instead query the ReturnType
of a different function).
This might be necessary to prevent user errors that would cause unexpected behaviour. I have a feeling it is necessary (there seems to be a connection over OST while creating provider configs) but am not 100% sure.
But if user's don't get too much from this extra type safety (do they need to know about specific optional state types they're using?), I recommend dropping the generic and just using OptionalStateType
.
As mentioned, I don't know whether which is the case and, if I were to bet, I'd say that it was necessary. I can try it out and see, but you'll know right away.
NOTE: When I say users, I mean "Wasp users and calling code", not only Wasp users.
/* | ||
The function that returns the URL to redirect the user to the | ||
provider's login page. | ||
*/ | ||
getAuthorizationUrl: Parameters<typeof createOAuthLoginHandler<ST>>[2], | ||
getAuthorizationUrl: Parameters<typeof createOAuthLoginHandler<OST>>[2] |
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.
Disclaimer: talking about code again
Ok, I'm guessing this is where the relationship I suspected of in the previous comment shines through.
Another Effective typescript tip: Ideally, we'd want the types and and the user facing code to dictate the types/structure.
User-facing type
--> internal functions
For these fields, that does happen, but with extra steps:
User-facing type (optionalStateTypes)
--> internal functions (createOAuthLoginHandler)
--> User-facing type (getAuthorizationUrl)
Can we somehow flip this to:
User-facing types (all of them) --> internal functions
Signed-off-by: Mihovil Ilakovac <mihovil@ilakovac.com>
Signed-off-by: Mihovil Ilakovac <mihovil@ilakovac.com>
35d3c07
to
bdd7e70
Compare
/* On After Signup Hook */ | ||
export type OnAfterSignupHookFn = ( | ||
params: OnAfterSignupHookFnInput, | ||
) => Promise<void> |
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.
What happens when a user wants the hook to be a non-async 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.
Yep, we need to account for that 👍
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've adjusted the return type to allow for async and sync functions.
46e987f
to
0b93adc
Compare
Signed-off-by: Mihovil Ilakovac <mihovil@ilakovac.com>
Closes #1556
Adding hooks:
onAfterOAuthTokenReceivedLeft to do: