fix: #1251 - fix return_to for expired flows

[dibyajyotibehera]

Related issue(s)

Checklist

• I have read the contributing guidelines.
• 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
  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]

Codecov Report

Merging #1697 (79c34d1) into master (64c9b76) will increase coverage by 0.10%.
The diff coverage is 72.22%.

@@            Coverage Diff             @@
##           master    #1697      +/-   ##
==========================================
+ Coverage   73.92%   74.03%   +0.10%     
==========================================
  Files         260      260              
  Lines       12662    12692      +30     
==========================================
+ Hits         9360     9396      +36     
+ Misses       2679     2672       -7     
- Partials      623      624       +1     

Impacted Files	Coverage Δ
selfservice/flow/login/handler.go	68.88% <66.66%> (-0.11%)	⬇️
selfservice/flow/recovery/flow.go	90.69% <66.66%> (-3.90%)	⬇️
selfservice/flow/registration/handler.go	71.92% <66.66%> (-0.30%)	⬇️
selfservice/flow/settings/handler.go	63.43% <66.66%> (+0.15%)	⬆️
selfservice/flow/verification/flow.go	85.71% <66.66%> (-2.29%)	⬇️
selfservice/flow/login/error.go	77.41% <100.00%> (ø)
selfservice/flow/recovery/error.go	73.84% <100.00%> (ø)
selfservice/flow/registration/error.go	77.41% <100.00%> (ø)
selfservice/flow/settings/error.go	75.60% <100.00%> (ø)
selfservice/flow/verification/error.go	81.66% <100.00%> (ø)
... and 2 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 64c9b76...79c34d1. Read the comment docs.

[aeneasr]

Thank you! This is looking pretty good! I think we should still verify if this works end-to-end - so checking that you end up at the correct URL when you try to submit an expired flow. To do that, you could add a helper like this one

kratos/test/e2e/cypress/support/commands.js

Line 108 in 1cf61cd

Cypress.Commands.add('longRecoveryLifespan', ({} = {}) => {

kratos/test/e2e/cypress/support/commands.js

Line 124 in 1cf61cd

Cypress.Commands.add('shortRecoveryLifespan', ({} = {}) => {

and add a test to login

kratos/test/e2e/cypress/integration/profiles/email/login/success.spec.js

Lines 36 to 49 in 835fb31

	it('should sign in with case insensitive identifier', () => {
	cy.get('input[name="password_identifier"]').type(email.toUpperCase())
	cy.get('input[name="password"]').type(password)
	cy.get('button[type="submit"]').click()
	cy.session().should((session) => {
	const { identity } = session
	expect(identity.id).to.not.be.empty
	expect(identity.schema_id).to.equal('default')
	expect(identity.schema_url).to.equal(`${APP_URL}/schemas/default`)
	expect(identity.traits.website).to.equal(website)
	expect(identity.traits.email).to.equal(email)
	})
	})

(and the other flows) which has a beforeEach that reset the lifespan like here:

kratos/test/e2e/cypress/integration/profiles/recovery/recovery/errors.spec.js

Line 17 in 835fb31

cy.longRecoveryLifespan()

and has one test with a short expiry time

kratos/test/e2e/cypress/integration/profiles/recovery/recovery/errors.spec.js

Line 57 in 835fb31

cy.shortLinkLifespan()

In pseudo-code it might look something like this:

    it('should end up at the correct return to if flow is expired', () => {
      cy.visit(APP_URL + '/auth/login?return_to=....')
      cy.shortLoginLifespan()
      cy.get('input[name="password_identifier"]').type(email.toUpperCase())
      cy.get('input[name="password"]').type(password)
      cy.get('button[type="submit"]').click()

      // Try again
      cy.longLoginLifespan()
      cy.get('input[name="password_identifier"]').type(email.toUpperCase())
      cy.get('input[name="password"]').type(password)
      cy.get('button[type="submit"]').click()

      cy.session()

      cy.location().should('contain', '...')
    })

You can run e2e test quickly using:

./test/e2e/run.sh --dev sqlite

[aeneasr]

Sorry, forgot to submit this comment

[aeneasr]

Just for a sanity check maybe add this here:

Suggested change

	assert.Equal(t, f.RequestURL, oldReqUrl)
	assert.Equal(t, f.RequestURL, oldReqUrl)
	assert.Contains(t, f.RequestURL, "?return_to=https://www.ory.sh")

(and to the other tests as well)

[dibyajyotibehera]

thanks . I made the changes added ui tests for login and registration flows . I believe other flows stay on the settings ui instead of redirecting . Please check.

[aeneasr]

Let me know when you want this reviewed again :)

[dibyajyotibehera]

Let me know when you want this reviewed again :)

Please review it :) again .
I had a put a comment above actually to get it reviewed :)

thanks . I made the changes added ui tests for login and registration flows . I believe other flows stay on the settings ui instead of redirecting . Please check.

[aeneasr]

Oh sorry, I missed that!

[aeneasr]

Awesome work!

[aeneasr]

The CI failure is unrelated to your changes!