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

feat: add prompt=registration parameter #3636

Merged
merged 1 commit into from Sep 29, 2023
Merged

Conversation

hperl
Copy link
Contributor

@hperl hperl commented Sep 28, 2023

Ory 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).

Related issue(s)

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

@hperl hperl requested a review from aeneasr as a code owner September 28, 2023 11:03
@hperl hperl self-assigned this Sep 28, 2023
@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

Merging #3636 (1134e04) into master (5c8e792) will increase coverage by 0.09%.
Report is 2 commits behind head on master.
The diff coverage is 90.00%.

❗ Current head 1134e04 differs from pull request most recent head df55db8. Consider uploading reports for the commit df55db8 to get more accurate results

@@            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     
Files Coverage Δ
consent/strategy_default.go 68.90% <100.00%> (+0.22%) ⬆️
driver/config/provider.go 79.33% <100.00%> (+0.13%) ⬆️
fositex/config.go 86.11% <86.36%> (ø)

... and 2 files with indirect coverage changes

kmherrmann
kmherrmann previously approved these changes Sep 28, 2023
Copy link
Contributor

@kmherrmann kmherrmann left a 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 :)

Copy link
Member

@zepatrik zepatrik left a 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") {
Copy link
Member

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?

Copy link
Contributor Author

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,
Copy link
Member

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.

Copy link
Contributor Author

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.

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`).
@aeneasr aeneasr merged commit 19857d2 into master Sep 29, 2023
29 checks passed
@aeneasr aeneasr deleted the hperl/prompt-registration branch September 29, 2023 08:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants