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 upstream parameters to oidc provider #3138

Merged
merged 16 commits into from Mar 9, 2023
Merged

Conversation

Benehiko
Copy link
Contributor

@Benehiko Benehiko commented Mar 1, 2023

This PR introduces the upstream OIDC query parameters login_hint and hd. More can be added at a later stage in the link.schema.json and settings.schema.json file.

"upstream_parameters": {
"type": "object",
"$comment": "Only the defined parameters are allowed. This is to prevent users from sending arbitrary parameters or craft URLs that cause unexpected behavior.",
"properties": {
"login_hint": {
"description": "The login hint could be an email address or identifier in the upstream provider to pre-fill the upstream provider login form",
"type": "string"
},
"hd": {
"description": "The hd (hosted domain) parameter streamlines the login process for G Suite hosted accounts. By including the domain of the G Suite user (for example, mycollege.edu), you can indicate that the account selection UI should be optimized for accounts at that domain.",
"type": "string"
},

https://github.com/ory/kratos/blob/4362423c3cddd98e46745493288fec69f9c66766/selfservice/strategy/oidc/.schema/settings.schema.json

To send additional upstream parameters the form can post this on a login, registration or settings link submit.
For example the form below does an OIDC flow to Google. We can now add additional parameters such as login_hint and hd to the upstream request to Google login with a pre-filled email email@example.com:

<form action="https://kratos/self-service/login?flow=">
  <input type="submit" name="provider" value="google" />
  <input type="hidden" name="upstream_parameters.login_hint" value="email@example.com" />
  <input type="hidden" name="upstream_parameters.hd" value="example.com" />
</form>

Related issue(s)

#3127
#2069

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

@codecov
Copy link

codecov bot commented Mar 3, 2023

Codecov Report

Merging #3138 (af8ccac) into master (dba3803) will decrease coverage by 0.03%.
The diff coverage is 72.72%.

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

@@            Coverage Diff             @@
##           master    #3138      +/-   ##
==========================================
- Coverage   77.58%   77.55%   -0.03%     
==========================================
  Files         316      316              
  Lines       19906    19924      +18     
==========================================
+ Hits        15444    15452       +8     
- Misses       3273     3279       +6     
- Partials     1189     1193       +4     
Impacted Files Coverage Δ
selfservice/strategy/oidc/strategy_login.go 66.25% <50.00%> (-1.29%) ⬇️
selfservice/strategy/oidc/strategy_registration.go 64.81% <50.00%> (-0.60%) ⬇️
selfservice/strategy/oidc/strategy_settings.go 63.77% <50.00%> (-0.35%) ⬇️
selfservice/flow/login/handler.go 78.77% <100.00%> (ø)
selfservice/strategy/oidc/provider.go 100.00% <100.00%> (ø)
session/test/persistence.go 99.05% <0.00%> (-0.95%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Benehiko
Copy link
Contributor Author

Benehiko commented Mar 6, 2023

For the E2E tests to pass here we need to merge ory/kratos-selfservice-ui-react-nextjs#54 first.

Locally e2e tests pass.
image

@Benehiko Benehiko marked this pull request as ready for review March 6, 2023 16:29
@CaptainStandby CaptainStandby self-requested a review March 7, 2023 10:53
jonas-jonas
jonas-jonas previously approved these changes Mar 7, 2023
Copy link
Contributor

@jonas-jonas jonas-jonas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

selfservice/strategy/oidc/strategy_login.go Outdated Show resolved Hide resolved
selfservice/strategy/oidc/strategy_registration.go Outdated Show resolved Hide resolved
CaptainStandby
CaptainStandby previously approved these changes Mar 7, 2023
Copy link
Contributor

@CaptainStandby CaptainStandby left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work. Only some minor things.

selfservice/strategy/oidc/provider.go Outdated Show resolved Hide resolved
selfservice/strategy/oidc/strategy_login.go Outdated Show resolved Hide resolved
selfservice/strategy/oidc/strategy_registration.go Outdated Show resolved Hide resolved
@Benehiko Benehiko dismissed stale reviews from CaptainStandby and jonas-jonas via 1895fd1 March 7, 2023 16:11
CaptainStandby
CaptainStandby previously approved these changes Mar 7, 2023
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done, I have a some comments :)

selfservice/strategy/oidc/strategy_login.go Outdated Show resolved Hide resolved
selfservice/strategy/oidc/strategy_login.go Outdated Show resolved Hide resolved
Comment on lines 70 to 71
// validation of sent providers are already handled in the `oidc/.schema/link.schema.json` file.
// `upstreamParameters` will always only contain allowed parameters based on the configuration.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer this to be more explicit, because the input source probably is validated against the form payload, but it might not be in a certain case (e.g. the flow is resumed or something). By putting the validation in the central place where the code is executed, the input source no longer matters because the validation is "deeply embedded" into the flow, and not on the "surface".

The tests cover this nicely already, but for example they don't cover the account settings flow. So here we would have uncertainty whether this validation actually is implemented / working.

Generally speaking, security validation should always be very very explicit and treated like "do you actually have this permission and do I have 100% certainty?".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should now be addressed. I have added the upstream parameters to Settings flow as well. I am also double checking the parameters inside the UpstreamParameters function.

@aeneasr
Copy link
Member

aeneasr commented Mar 9, 2023

@Benehiko can you please add this to the google social sign in documentation?

@aeneasr aeneasr merged commit b6b1679 into master Mar 9, 2023
@aeneasr aeneasr deleted the feat-oidc-parameters branch March 9, 2023 10:04
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