-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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: add prompt=registration parameter #3636
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3636 +/- ##
==========================================
+ Coverage 76.00% 76.10% +0.09%
==========================================
Files 132 132
Lines 10015 10076 +61
==========================================
+ Hits 7612 7668 +56
- Misses 1879 1881 +2
- Partials 524 527 +3
|
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.
LGTM but I don't have golang readability :)
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.
Nice, just some minor docs/test suggestions 👍
@@ -293,7 +293,14 @@ func (s *DefaultStrategy) forwardAuthenticationRequest(ctx context.Context, w ht | |||
return err | |||
} | |||
|
|||
http.Redirect(w, r, urlx.SetQuery(s.c.LoginURL(ctx), url.Values{"login_challenge": {encodedFlow}}).String(), http.StatusFound) | |||
var baseURL *url.URL | |||
if stringslice.Has(prompt, "registration") { |
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.
Do we have to add this to the API spec somewhere, so the value is allowed in the SDK?
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 authorize endpoint is not part of the API spec, so nothing to change here.
https://www.ory.sh/docs/reference/api#tag/oAuth2/operation/oAuth2Authorize
acceptLoginHandler(t, c, subject, nil), | ||
acceptConsentHandler(t, c, subject, nil)) | ||
|
||
code, _ := getAuthorizeCode(t, conf, nil, |
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.
Here is not really a check whether we correctly redirected. To not mess with the whole auth code flow, you could wrap above handler and set some bool var when it is executed. That way we know the right UI was used.
Maybe also set the login URI to some random value, so we make sure it is not used instead.
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.
Good point! I set the login handler to nil
, which will throw an error. Thus the registration handler ist called.
c15d0d3
to
4b9adb0
Compare
Hydra now supports a `registration` value for the `prompt` parameter of the authorization request. When specifying `prompt=registration`, Hydra will redirect the user to the URL found under `urls.registration` (instead of `urls.login`).
4b9adb0
to
df55db8
Compare
Ory Hydra now supports a
registration
value for theprompt
parameter of the authorization request. When specifyingprompt=registration
, Hydra will redirect the user to the URL found underurls.registration
(instead ofurls.login
).Related issue(s)
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further Comments