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

Recovery self service flow does not pass on redirect_to URL #914

Closed
zepatrik opened this issue Dec 16, 2020 · 8 comments
Closed

Recovery self service flow does not pass on redirect_to URL #914

zepatrik opened this issue Dec 16, 2020 · 8 comments
Labels
bug Something is not working. good first issue A good issue to tackle when being a novice to the project. help wanted We are looking for help on this one.

Comments

@zepatrik
Copy link
Member

The recovery flow should pass on the return_to or ...recovery.after.default_browser_return_url to the following settings flow. This way the user ends up at the correct page after it completed all steps required for recovery.

Start here:

http.Redirect(w, r, req.AppendTo(s.c.SelfServiceFlowRecoveryUI()).String(), http.StatusFound)

@zepatrik zepatrik added bug Something is not working. help wanted We are looking for help on this one. good first issue A good issue to tackle when being a novice to the project. labels Dec 16, 2020
@Beetix
Copy link
Contributor

Beetix commented Dec 16, 2020

Seems like the redirect happens here:

http.Redirect(w, r, sf.AppendTo(s.c.SelfServiceFlowSettingsUI()).String(), http.StatusFound)

@Beetix
Copy link
Contributor

Beetix commented Dec 16, 2020

I took some time to study the code.

When the recovery flow is completed, a new settings flow is created and the user is redirected the settings UI with a flow token:

sf, err := s.d.SettingsHandler().NewFlow(w, r, sess.Identity, flow.TypeBrowser)
if err != nil {
s.d.SelfServiceErrorManager().Forward(r.Context(), w, r, err)
return
}
sf.Messages.Set(text.NewRecoverySuccessful(time.Now().Add(s.c.SelfServiceFlowSettingsPrivilegedSessionMaxAge())))
if err := s.d.SettingsFlowPersister().UpdateSettingsFlow(r.Context(), sf); err != nil {
s.d.SelfServiceErrorManager().Forward(r.Context(), w, r, err)
return
}
http.Redirect(w, r, sf.AppendTo(s.c.SelfServiceFlowSettingsUI()).String(), http.StatusFound)

This is sensible since the user is bound to change his password. Unfortunately, this is currently not compatible with the return URL mechanism.

I see 2 options:

  • Not creating a settings flow if a return URL is specified
  • Calling the return URL with the settings flow token

The later seems more relevant since as mentioned before the user is bound to change his password.

What's your take on this?

@aeneasr
Copy link
Member

aeneasr commented Dec 21, 2020

Thank you for thinking about this @Beetix ! I don't think the second option is valid, it can easily be abused by an attacker to present you a phony "update your password ;) ;)" UI which is actually a phishing site. What we could do though is calling the return_to URL once the settings flow was completed!

@Beetix
Copy link
Contributor

Beetix commented Dec 21, 2020

Thank you for thinking about this @Beetix ! I don't think the second option is valid, it can easily be abused by an attacker to present you a phony "update your password ;) ;)" UI which is actually a phishing site. What we could do though is calling the return_to URL once the settings flow was completed!

Yes, calling the return_to URL after the settings flow seems better. I have proposed these ideas while trying to figure out a way to focus the user on changing his password. The issue with the settings flow is that a user can update his profile instead of his password (which is what really matters in a recovery).

@zepatrik
Copy link
Member Author

If you want to prevent that you can omit the other fields in the UI. It will probably change with #929 so that traits and password are separated anyway.

@Beetix
Copy link
Contributor

Beetix commented Dec 21, 2020

If you want to prevent that you can omit the other fields in the UI. It will probably change with #929 so that traits and password are separated anyway.

Yes, resolving #929 seems to be the answer to my problem. Having a single URL for both is problematic. If I hide the fields in the UI it means that I'll never be able to change these settings.

@zepatrik
Copy link
Member Author

zepatrik commented Dec 22, 2020

You can hide them conditionally, e.g. depending on the referrer or some query parameter.

@dan2kx
Copy link

dan2kx commented Oct 27, 2021

Has this been implemented yet? I'm also looking for a solution to get back to a third party app (which uses hydra) after a user requests recovery or when creating a user in kratos and sending out an invite email with the recovery url

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working. good first issue A good issue to tackle when being a novice to the project. help wanted We are looking for help on this one.
Projects
None yet
Development

No branches or pull requests

4 participants