fix: accept recovery link from authenticated users (#1077) #2195

kszafran · 2022-02-05T09:42:27Z

When a recovery link is opened while the user already has a session cookie (possibly for another account), the endpoint will now correctly complete the recovery process and issue new cookies.

BREAKING CHANGES: Calling /self-service/recovery without flow ID or with an invalid flow ID while authenticated will now respond with an error instead of redirecting to the default page.

Related issue(s)

Closes https://github.com/ory-corp/cloud/issues/2173

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

I don't like my solution too much, it feels like a hack. Since I'm not too familiar with the codebase, I didn't want to make too many changes. I'm open to suggestions. I was thinking that perhaps instead of doing this in submitFlow (pseudocode):

fetch flow by ID

for each strategy:
    strategy.TryToHandle(...)

we could do:

for each strategy:
    selected = strategy.TryToAccept(...)

if some specific error:
    redirect authenticated

fetch flow by ID

selected.Handle(...)

This would also help avoid that minor breaking change.

CLAassistant · 2022-02-05T09:42:31Z

All committers have signed the CLA.

selfservice/strategy/link/strategy_recovery.go

kszafran · 2022-02-05T15:02:03Z

The e2e tests failed apparently due to some random timeout. I'll ignore that for now, since I'll probably need to push some changes and rerun tests after code review.

kszafran · 2022-02-07T10:50:24Z

And now it failed on make test-coverage with:

=== RUN   TestError
    e2e_server.go:83: Starting migrations...
Successfully applied SQL migrations!
    e2e_server.go:85: Migration done
    e2e_server.go:87: Starting server...
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x15afdc6]

goroutine 4418 [running]:
github.com/ory/x/prometheusx.(*MetricsManager).RegisterRouter(...)
	/home/circleci/go/pkg/mod/github.com/ory/x@v0.0.334/prometheusx/middleware.go:34
github.com/ory/kratos/cmd/daemon.ServeAdmin(0x1ec6098, 0xc0004d0d80, 0xc0019d2460, 0xc0000f6780, 0xc00195adb0, 0x0, 0x3, 0x0, 0x0, 0x0)
	/home/circleci/project/cmd/daemon/serve.go:170 +0x7e6
created by github.com/ory/kratos/cmd/daemon.ServeAll.func1
	/home/circleci/project/cmd/daemon/serve.go:283 +0x199
FAIL	github.com/ory/kratos/examples/go/selfservice/error	1.903s
FAIL

It does not seem like it's related to my changes 🤔.

aeneasr · 2022-02-07T17:40:19Z

Re-running CI now :)

aeneasr

Thank you! This looks great! Could you please also add an end-to-end test, for example to this file ( https://github.com/ory/kratos/blob/master/test/e2e/cypress/integration/profiles/recovery/recovery/success.spec.ts#L72 ) to ensure these changes work as expected end-to-end? :)

aeneasr · 2022-02-07T17:54:19Z

selfservice/strategy/link/strategy_recovery.go

@@ -221,6 +221,11 @@ func (s *Strategy) Recover(w http.ResponseWriter, r *http.Request, f *recovery.F
 		return s.recoveryUseToken(w, r, body)
 	}

+	if _, err := s.d.SessionManager().FetchFromRequest(r.Context(), r); err == nil {
+		session.RedirectOnAuthenticated(s.d)(w, r, nil)


Where does this redirect to?

I took this straight from

kratos/selfservice/flow/recovery/handler.go

Line 68 in 2a55f3f

redirect := session.RedirectOnAuthenticated(h.d)

. I think it redirects to a default page?

Ok, that works - is there a test that ensures we are really redirecting to there? :)

Check the test in handler_test.go. It initiates a new recovery flow, navigates to ui.action, and ends up redirected.

codecov · 2022-02-07T18:07:31Z

Codecov Report

Merging #2195 (0d6b06f) into master (b075a5b) will increase coverage by 0.03%.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##           master    #2195      +/-   ##
==========================================
+ Coverage   76.58%   76.61%   +0.03%     
==========================================
  Files         318      318              
  Lines       17156    17189      +33     
==========================================
+ Hits        13139    13170      +31     
- Misses       3086     3087       +1     
- Partials      931      932       +1

Impacted Files	Coverage Δ
internal/testhelpers/handler_mock.go	`90.69% <100.00%> (+1.22%)`	⬆️
internal/testhelpers/server.go	`90.69% <100.00%> (+0.22%)`	⬆️
internal/testhelpers/session.go	`100.00% <100.00%> (ø)`
selfservice/flow/recovery/handler.go	`61.81% <100.00%> (ø)`
selfservice/strategy/link/strategy_recovery.go	`64.08% <100.00%> (+0.90%)`	⬆️
persistence/sql/persister_courier.go	`85.00% <0.00%> (-3.34%)`	⬇️

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 61998f9...c03c635. Read the comment docs.

kszafran · 2022-02-07T18:22:13Z

Thank you! This looks great! Could you please also add an end-to-end test, for example to this file ( https://github.com/ory/kratos/blob/master/test/e2e/cypress/integration/profiles/recovery/recovery/success.spec.ts#L72 ) to ensure these changes work as expected end-to-end? :)

I'll try, I didn't yet manage to run e2e tests locally. Is it possible to run a single e2e test?

kszafran · 2022-02-07T18:31:52Z

Are you OK with this approach in general, or would you prefer that I redo it like described in the "Further Comments" section (or solve this some other way)?

aeneasr · 2022-02-08T13:32:47Z

Yes, the approach is good! :)

kszafran · 2022-02-08T13:38:29Z

I added an e2e test.

I aldo added a simple test for GetCSRFToken to get the coverage back up (seems that the recovery flow tests were going through a branch that verification tests don't). I'm not a fan of such mocked, fine-grained tests personally. Let me know if you'd prefer some other test, or just ignore that minimal drop in coverage and delete that test.

aeneasr · 2022-02-14T10:52:34Z

Hey, I need more time to review this. Somehow in my head it does not make sense that the introduced change has the effect it should, so I need to go through the changed code line-by-line, but I don't really have time for it this week :/

kszafran · 2022-02-14T11:32:09Z

Hey, I need more time to review this. Somehow in my head it does not make sense that the introduced change has the effect it should, so I need to go through the changed code line-by-line, but I don't really have time for it this week :/

No worries. I described the problem here: #1077 (comment). The TL;DR is that if you're authenticated, then /self-service/recovery always redirects to the default page, whether you wanted to submit a recovery form or you clicked on a recovery link (it's the same endpoint). The fix is to not redirect immediately on the route level (in recovery.RegisterPublicRoutes), but do it later once we know that what we're handling is not a recovery link.

kszafran · 2022-03-01T08:57:28Z

@aeneasr Any chance you'll look into this PR this week?

piotrmsc · 2022-03-18T13:35:04Z

@kszafran can you please take a look at CI failures?

kszafran · 2022-03-18T14:12:22Z

@kszafran can you please take a look at CI failures?

This issue is supposedly fixed in master already: #1077 I haven't closed this PR yet, because had trouble testing the solution in master locally (see my latest comment in the linked issue). I'm hoping I can test it without issues once v0.8.3 is released.

kszafran · 2022-03-23T10:01:30Z

@piotrmsc I tested revoke_active_sessions locally today and it doesn't solve the issue. I will try to rebase this PR onto current master.

aeneasr · 2022-03-23T11:22:07Z

Thank you, I looked into the PR and resolved a couple of issues I found

kszafran · 2022-03-23T11:53:55Z

@aeneasr In this case, is there still anything you need from me?

aeneasr · 2022-03-23T13:09:28Z

I think we're good :)

vinckr · 2022-03-24T11:48:01Z

Hello @kszafran
Congrats on merging your first PR in Ory 🎉 !
Your contribution will soon be helping secure millions of identities around the globe 🌏.
As a small token of appreciation we send all our first time contributors a gift package to welcome them to the community.
Please drop me an email and I will forward you the form to claim your Ory swag!

When a recovery link is opened while the user already has a session cookie (possibly for another account), the endpoint will now correctly complete the recovery process and issue new cookies. BREAKING CHANGES: Calling /self-service/recovery without flow ID or with an invalid flow ID while authenticated will now respond with an error instead of redirecting to the default page. Closes https://github.com/ory-corp/cloud/issues/2173 Co-authored-by: aeneasr <3372410+aeneasr@users.noreply.github.com>

kszafran requested review from aeneasr and zepatrik as code owners February 5, 2022 09:42

kszafran commented Feb 5, 2022

View reviewed changes

selfservice/strategy/link/strategy_recovery.go Show resolved Hide resolved

aeneasr reviewed Feb 7, 2022

View reviewed changes

fix: accept recovery link from authenticated users (#1077)

12edf98

aeneasr approved these changes Mar 23, 2022

View reviewed changes

aeneasr self-assigned this Mar 23, 2022

chore: code review

c03c635

aeneasr merged commit 0fa64dd into ory:master Mar 23, 2022

kszafran deleted the fix-recovery-link branch March 24, 2022 11:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: accept recovery link from authenticated users (#1077) #2195

fix: accept recovery link from authenticated users (#1077) #2195

kszafran commented Feb 5, 2022 •

edited by aeneasr

Loading

CLAassistant commented Feb 5, 2022 •

edited

Loading

kszafran commented Feb 5, 2022

kszafran commented Feb 7, 2022 •

edited

Loading

aeneasr commented Feb 7, 2022

aeneasr left a comment

aeneasr Feb 7, 2022

kszafran Feb 7, 2022

aeneasr Feb 8, 2022

kszafran Feb 8, 2022

codecov bot commented Feb 7, 2022 •

edited

Loading

kszafran commented Feb 7, 2022

kszafran commented Feb 7, 2022

aeneasr commented Feb 8, 2022

kszafran commented Feb 8, 2022

aeneasr commented Feb 14, 2022

kszafran commented Feb 14, 2022 •

edited

Loading

kszafran commented Mar 1, 2022

piotrmsc commented Mar 18, 2022 •

edited

Loading

kszafran commented Mar 18, 2022

kszafran commented Mar 23, 2022

aeneasr commented Mar 23, 2022

kszafran commented Mar 23, 2022

aeneasr commented Mar 23, 2022

vinckr commented Mar 24, 2022

fix: accept recovery link from authenticated users (#1077) #2195

fix: accept recovery link from authenticated users (#1077) #2195

Conversation

kszafran commented Feb 5, 2022 • edited by aeneasr Loading

Related issue(s)

Checklist

Further Comments

CLAassistant commented Feb 5, 2022 • edited Loading

kszafran commented Feb 5, 2022

kszafran commented Feb 7, 2022 • edited Loading

aeneasr commented Feb 7, 2022

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr Feb 7, 2022

Choose a reason for hiding this comment

kszafran Feb 7, 2022

Choose a reason for hiding this comment

aeneasr Feb 8, 2022

Choose a reason for hiding this comment

kszafran Feb 8, 2022

Choose a reason for hiding this comment

codecov bot commented Feb 7, 2022 • edited Loading

Codecov Report

kszafran commented Feb 7, 2022

kszafran commented Feb 7, 2022

aeneasr commented Feb 8, 2022

kszafran commented Feb 8, 2022

aeneasr commented Feb 14, 2022

kszafran commented Feb 14, 2022 • edited Loading

kszafran commented Mar 1, 2022

piotrmsc commented Mar 18, 2022 • edited Loading

kszafran commented Mar 18, 2022

kszafran commented Mar 23, 2022

aeneasr commented Mar 23, 2022

kszafran commented Mar 23, 2022

aeneasr commented Mar 23, 2022

vinckr commented Mar 24, 2022

kszafran commented Feb 5, 2022 •

edited by aeneasr

Loading

CLAassistant commented Feb 5, 2022 •

edited

Loading

kszafran commented Feb 7, 2022 •

edited

Loading

codecov bot commented Feb 7, 2022 •

edited

Loading

kszafran commented Feb 14, 2022 •

edited

Loading

piotrmsc commented Mar 18, 2022 •

edited

Loading