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 support for recovery on native flows #3273

Merged
merged 42 commits into from
Nov 15, 2023

Conversation

jonas-jonas
Copy link
Member

@jonas-jonas jonas-jonas commented May 9, 2023

Adds the ability to complete the recovery flow properly on API flows. This PR also streamlines the behavior for SPA flows to not return 422 errors anymore. To enable this new behavior, set the features.use_continue_with_transitions flag in the config to true.

Fixes #2628

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
    [email protected]) 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

@jonas-jonas jonas-jonas self-assigned this May 9, 2023
@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

Attention: 48 lines in your changes are missing coverage. Please review.

Comparison is base (0fa648d) 78.29% compared to head (d9fee0b) 78.39%.
Report is 3 commits behind head on master.

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

Files Patch % Lines
selfservice/strategy/code/strategy_recovery.go 60.00% 18 Missing and 8 partials ⚠️
...lfservice/strategy/code/strategy_recovery_admin.go 78.87% 10 Missing and 5 partials ⚠️
selfservice/flow/recovery/handler.go 33.33% 2 Missing and 2 partials ⚠️
selfservice/flow/recovery/error.go 92.30% 1 Missing and 1 partial ⚠️
identity/handler.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3273      +/-   ##
==========================================
+ Coverage   78.29%   78.39%   +0.10%     
==========================================
  Files         343      343              
  Lines       23418    23470      +52     
==========================================
+ Hits        18334    18400      +66     
+ Misses       3703     3685      -18     
- Partials     1381     1385       +4     

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

@hperl hperl self-assigned this Aug 22, 2023
@hperl hperl force-pushed the jonas-jonas/nativeRecovery branch from e092315 to 91116de Compare August 22, 2023 11:50
@hperl hperl force-pushed the jonas-jonas/nativeRecovery branch from d88d8cf to 49a2b84 Compare August 24, 2023 09:39
@hperl hperl marked this pull request as ready for review August 24, 2023 09:58
Copy link
Member Author

@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.

This definitely still needs e2e tests (either PW or cypress), for all the cases. Since the react-native PR got merged, that should be quite easy to do.

contrib/quickstart/kratos/email-password/kratos.yml Outdated Show resolved Hide resolved
selfservice/flow/recovery/flow.go Show resolved Hide resolved
selfservice/strategy/code/strategy_recovery.go Outdated Show resolved Hide resolved
@hperl hperl force-pushed the jonas-jonas/nativeRecovery branch from 49a2b84 to 0af37f3 Compare August 25, 2023 11:25
Co-authored-by: Jonas Hungershausen <[email protected]>
@hperl hperl force-pushed the jonas-jonas/nativeRecovery branch from 0af37f3 to 77b7c72 Compare August 25, 2023 11:28
@jonas-jonas jonas-jonas marked this pull request as draft September 6, 2023 07:55
Benehiko
Benehiko previously approved these changes Oct 23, 2023
Copy link
Contributor

@Benehiko Benehiko left a comment

Choose a reason for hiding this comment

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

LGTM :)

hperl
hperl previously approved these changes Oct 31, 2023
Copy link
Contributor

@hperl hperl left a comment

Choose a reason for hiding this comment

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

LGTM! I just have a small suggestion regarding the naming of the feature flag.

Also, IIRC, the DX of the 422 errors is not really great in all SDKs. For Java, it requires explicitly overriding some error factory to extract the error. Is this really the best way? We should probably discuss this elsewhere.

embedx/config.schema.json Outdated Show resolved Hide resolved
@jonas-jonas
Copy link
Member Author

Also, IIRC, the DX of the 422 errors is not really great in all SDKs. For Java, it requires explicitly overriding some error factory to extract the error. Is this really the best way? We should probably discuss this elsewhere.

Yes, you're right, but I am not sure what error code we could use instead. A 200 OK response would still be okay, but it doesn't convey the "urgency" that there is another step required here.

@jonas-jonas jonas-jonas dismissed stale reviews from hperl and Benehiko via 983f813 November 12, 2023 12:45
@aeneasr
Copy link
Member

aeneasr commented Nov 14, 2023

What changes are needed in the docs for this?

@aeneasr aeneasr self-requested a review November 14, 2023 10:04
@jonas-jonas
Copy link
Member Author

What changes are needed in the docs for this?

I'll need to figure out how to document this properly. Since we now use the feature flag, and we'll need a UI in the console for it, I'll first try to get this into the network and then do the docs changes, if that's okay with you. @aeneasr

@aeneasr
Copy link
Member

aeneasr commented Nov 14, 2023

Can you please add an entry for the changelog here?

@jonas-jonas jonas-jonas merged commit e363889 into master Nov 15, 2023
28 checks passed
@jonas-jonas jonas-jonas deleted the jonas-jonas/nativeRecovery branch November 15, 2023 10:11
jonas-jonas added a commit that referenced this pull request Nov 15, 2023
Adds the ability to complete the recovery flow properly on API flows. This PR also streamlines the behavior for SPA flows to not return 422 errors anymore. To enable this new behavior, set the features.use_continue_with_transitions flag in the config to `true`.

See also #3273
aeneasr pushed a commit that referenced this pull request Nov 15, 2023
Adds the ability to complete the recovery flow properly on API flows. This PR also streamlines the behavior for SPA flows to not return 422 errors anymore. To enable this new behavior, set the features.use_continue_with_transitions flag in the config to `true`.

See also #3273
moose115 pushed a commit to moose115/kratos that referenced this pull request Dec 7, 2023
---------

Co-authored-by: Henning Perl <[email protected]>
Co-authored-by: Alano Terblanche <[email protected]>
Co-authored-by: aeneasr <[email protected]>
moose115 pushed a commit to moose115/kratos that referenced this pull request Dec 7, 2023
Adds the ability to complete the recovery flow properly on API flows. This PR also streamlines the behavior for SPA flows to not return 422 errors anymore. To enable this new behavior, set the features.use_continue_with_transitions flag in the config to `true`.

See also ory#3273
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.

I'm unable to reset password on mobile devices
5 participants