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

fix: don't inherit flow type in recovery and verification flows #2250

Merged
merged 8 commits into from
Feb 25, 2022

Conversation

Demonsthere
Copy link
Contributor

@Demonsthere Demonsthere commented Feb 23, 2022

  • Set flow type to Browser in recovery and verification flows
  • Add tests for both
  • Add e2e tests for expired validation flow

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 green light (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

@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Merging #2250 (ba34f4a) into master (c5a04b5) will increase coverage by 0.00%.
The diff coverage is 88.88%.

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2250   +/-   ##
=======================================
  Coverage   75.88%   75.89%           
=======================================
  Files         298      298           
  Lines       15917    15923    +6     
=======================================
+ Hits        12079    12085    +6     
  Misses       2979     2979           
  Partials      859      859           
Impacted Files Coverage Δ
courier/message.go 100.00% <ø> (ø)
courier/template/testhelpers/testhelpers.go 95.06% <66.66%> (ø)
selfservice/strategy/link/sender.go 71.27% <66.66%> (ø)
driver/config/config.go 81.89% <82.35%> (ø)
courier/courier.go 69.44% <100.00%> (ø)
courier/template/load_template.go 78.43% <100.00%> (ø)
driver/registry_default.go 86.64% <100.00%> (ø)
selfservice/flow/recovery/error.go 78.43% <100.00%> (ø)
selfservice/flow/recovery/flow.go 93.44% <100.00%> (+0.33%) ⬆️
selfservice/flow/verification/error.go 78.84% <100.00%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5a04b5...ca62281. Read the comment docs.

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.

For web-based flows we have a test that recovers / verifies an account with an expired link here:

it('is unable to recover the email address if the code is expired', () => {
cy.shortLinkLifespan()
const identity = gen.identityWithWebsite()
cy.registerApi(identity)
cy.recoverApi({ email: identity.email })
cy.recoverEmailButExpired({ expect: { email: identity.email } })
cy.get('[data-testid="ui/message/4060005"]').should(
'contain.text',
'The recovery flow expired'
)
cy.noSession()
})

I am under the impression that this test is actually recovering the account using an API flow, and then testing for this bug. So it appears that this bug is actually not happening on account recovery, only on verification?

If you (more or less) copy the test code of recovery to verification as well:

it('is unable to verify the email address if the code is expired', () => {
cy.shortLinkLifespan()
cy.visit(verification)
cy.get('input[name="email"]').type(identity.email)
cy.get('button[value="link"]').click()
cy.get('[data-testid="ui/message/1080001"]').should(
'contain.text',
'An email containing a verification'
)
cy.verifyEmailButExpired({ expect: { email: identity.email } })
})

we would have a full e2e test covering this use case. Could you please add that?

It is a bit mysterious to me though why recovery seems to pass without our changes? But I do remember that I at one point introduced a fix for this...

selfservice/flow/recovery/flow.go Show resolved Hide resolved
selfservice/flow/verification/flow.go Show resolved Hide resolved
@Demonsthere Demonsthere requested a review from aeneasr February 24, 2022 13:37
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.

Looks solid IMO.

test/e2e/cypress/support/commands.ts Show resolved Hide resolved
selfservice/flow/recovery/flow.go Show resolved Hide resolved
selfservice/flow/verification/flow.go Show resolved Hide resolved
@@ -402,6 +402,52 @@ Cypress.Commands.add('recoverApi', ({ email, returnTo }) => {
})
})

Cypress.Commands.add('verificationApiExpired', ({ email, returnTo }) => {
cy.shortVerificationLifespan()
Copy link
Member

Choose a reason for hiding this comment

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

This will affect following tests right? Can you maybe also after creating the expired flow, reset the config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I could, but we do reset it for each test case here

Copy link
Member

Choose a reason for hiding this comment

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

I see, then this is fine 👍

@aeneasr
Copy link
Member

aeneasr commented Feb 25, 2022

Thank you! I wanted to add a little test to recovery as well and while doing so I realized that the bug is still there. You can find my fixes here: 8d191d5

The problem was basically the redirection logic as it was still using the old flow type, hence redirecting to a JSON endpoint!

selfservice/flow/recovery/error.go Outdated Show resolved Hide resolved
selfservice/flow/verification/error.go Outdated Show resolved Hide resolved
@aeneasr aeneasr merged commit c5b444a into ory:master Feb 25, 2022
@vinckr vinckr mentioned this pull request Mar 18, 2022
peturgeorgievv pushed a commit to senteca/kratos-fork that referenced this pull request Jun 30, 2023
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.

Recovery link answers with JSON payload for API flows
3 participants