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: extend recovery flow by adding the FlowID #3650

Closed

Conversation

matthiastz
Copy link

@matthiastz matthiastz commented Dec 1, 2023

  • the pw recovery flow has been extended
  • specifically the RecoveryCodeValidModel now also holds the FlowID
  • context: the FlowID is both persisted and associated to the RecoveryCode data model already
  • 🚀 when a Courier sends a new email for the pw recovery flow (valid), the Message.TemplateData now includes all the props as before + the FlowID
    • this was implemented and asserted in code_sender_test.go
    • having this FlowID parameter gives more flexible ways to design the recovery flow with a custom UI: when a user first triggers a pw recovery flow in the UI (a FlowID is already created), the user can close this Browser Tab and later on opens the email with the link that includes both the original FlowID and the recovery code (instead of keeping the Browser tab open and copy over the code manually), which all match to one Flow of the user

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

Copy link

codecov bot commented Dec 1, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7c0e02e) 78.40% compared to head (f7c4575) 78.41%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3650   +/-   ##
=======================================
  Coverage   78.40%   78.41%           
=======================================
  Files         344      344           
  Lines       23496    23497    +1     
=======================================
+ Hits        18422    18424    +2     
+ Misses       3690     3689    -1     
  Partials     1384     1384           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jonas-jonas
Copy link
Member

It was a deliberate choice to not include the link to the recovery form in the email body. The reason is, that the flow is protected against CSRF attacks, and if a violation occurs, the flow is voided. A violation could occur if the user opens the link in a different browser than they started the flow in. This would then just show a CSRF violation error message, and the user would need to re-start the flow. And to not even make it possible to implement these potentially broken flows, we omitted the link from the template data.

Is this the use-case you're trying to cover?

@matthiastz
Copy link
Author

@jonas-jonas thx for providing the context. Yeah thats pretty much the use-case.

So I assume you would deny/remove this PR, right?

@jonas-jonas
Copy link
Member

Yes, I don't think we can accept this PR.

Still, thank you, for your time and contribution! :)

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.

2 participants