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: return to oauth flow after switching from login to other flows #3212

Merged
merged 14 commits into from May 3, 2023

Conversation

Benehiko
Copy link
Contributor

@Benehiko Benehiko commented Apr 4, 2023

Related issue(s)

ory/network#264

With oauth2_provider.return_to_enabled = true the following is possible:

  1. Login flow switch to Registration flow, return to OAuth2 provider
  2. Login flow switch to Recovery flow to Settings flow, return to OAuth2 provider

An end-user following a login flow from a Single Sign On portal to the OAuth2 provider (Hydra) will always be redirected back to the provider (Hydra) after performing a more complex flow, such as switching from login to registration or recovery flow in Kratos. The OAuth2 provider can then complete the flow and redirect the user accordingly.

Note: The provider (Hydra) URL will also need to be added to Kratos's selfservice.allowed_return_urls configuration. Any return_to parameters supplied before the login request is created will be overwritten in Kratos by the OAuth2 provider URL.

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 Apr 4, 2023

Codecov Report

Merging #3212 (c406dbb) into master (342edec) will increase coverage by 0.03%.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##           master    #3212      +/-   ##
==========================================
+ Coverage   78.11%   78.15%   +0.03%     
==========================================
  Files         324      324              
  Lines       20754    20762       +8     
==========================================
+ Hits        16213    16227      +14     
+ Misses       3333     3330       -3     
+ Partials     1208     1205       -3     
Impacted Files Coverage Δ
driver/config/config.go 82.94% <100.00%> (+0.04%) ⬆️
hydra/fake.go 44.44% <100.00%> (+6.94%) ⬆️
selfservice/flow/login/handler.go 79.20% <100.00%> (+0.37%) ⬆️

... and 2 files with indirect coverage changes

@Benehiko Benehiko marked this pull request as ready for review April 26, 2023 13:39
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.

Is the login flow always the entry point, or do we need to add this logic also to registration? I guess not, but I wanted to bring it up anyway.

Follow up:

  • consider always adding the OAuth2 server URL to the allow list for return_to URLs
  • if I understand correctly, the UI can be simplified now because it does not have to handle the return to the OAuth2 server anymore, we probably need to adjust that somewhere in the docs

embedx/config.schema.json Outdated Show resolved Hide resolved
embedx/config.schema.json Outdated Show resolved Hide resolved
@Benehiko
Copy link
Contributor Author

This PR essentially makes it possible for more complex flows, such as recovery, to not break the existing OAuth2 flow by forcing the return_to to be the initial OAuth request URL. So once the recovery flow is complete through a credential update, the user will just continue the OAuth request as intended by signing in and performing the consent flow.

OAuth2 flows are always directed to the login page - at least in the normal case. The user might configure Hydra to point to a registration page which would require Kratos to also have this functionality on the registration endpoint. But I think we don't need to worry about it for now.

Once the OAuth2 server redirects to the login page, the UI could of course at this point set the OAuth request URL to the return_to, but I thought it might make sense to rather have this inside Kratos since we can then configure it. I would assume not everyone wants this behaviour, so it is an opt in rather than opt out feature.

The drawbacks of adding it to the Account Experience is due to it not having any access to significant configuration values and it tries to lessen any API calls to reduce latency, so adding another API endpoint for just this key wouldn't be that great.

Since Kratos is already being called by the Account Experience, it made sense to have Kratos dictate where the UI should end up at. Another bonus from this integration is that Custom UIs will also benefit from this and not just the Account Experience. The UI must, however, still handle the return_to query parameter between flows, e.g. login -> registration should still carry over the return_to on the UI side.

jonas-jonas
jonas-jonas previously approved these changes May 2, 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.

Code wise this LGTM, but I am not 100% sure I fully understand all consequences of this. Would like for someone else to confirm.

embedx/config.schema.json Outdated Show resolved Hide resolved
@Benehiko Benehiko merged commit a1fea6c into master May 3, 2023
30 checks passed
@Benehiko Benehiko deleted the feat-oauth-redirect branch May 3, 2023 15:16
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

3 participants