-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat(auth): add authentication endpoints #6265
Conversation
* update datamodel * update scope handlers * add index migration * fix integration tests * fix integration tests
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 some comments, lgtm otherwise!
packages/medusa/src/api-v2/auth/[scope]/[authProvider]/callback/route.ts
Outdated
Show resolved
Hide resolved
|
||
const { success, error, authUser, location } = authResult | ||
if (location) { | ||
res.redirect(location) |
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.
question: shouldn't the client be the one doing the redirect?
Co-authored-by: Riqwan Thamir <[email protected]>
Co-authored-by: Riqwan Thamir <[email protected]>
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.
few questions :D
packages/medusa/src/api-v2/auth/[scope]/[authProvider]/callback/route.ts
Outdated
Show resolved
Hide resolved
const authResult = await service.validateCallback(authProvider, authData) | ||
|
||
const { success, error, authUser, location } = authResult | ||
if (location) { |
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.
if there is a location (the redirect could be configured) then we don't check any success or failure?
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.
yes, we could validate to ensure that success
is required if you think that's appropriate? 😄 just figured that in case of a redirect you would want that no matter what
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.
in general you have a success redirect and a failure redirect that eventually could have custom query parameters, let see that later
What
/auth/[scope]/[provider]
/auth/[scope]/[provider]/callback
note: there's still some remaining work related to jwt auth to be handled, this is mainly focussed on session auth with endpoints