feat: Make admin recovery to work without emails #1419 #1750

abador · 2021-09-14T08:30:42Z

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

It's possible now to initiate and finish the whole recovery flow without a valid recovery email

aeneasr

Thank you! This looks much more like what we need! I haven’t vetted the tests yet but the logic looks 👌. The SQL statements need a bit more work but generally it appears to be good to go. Could you please also update the docs a bit that talk about this feature to reflect the new behavior? :)

aeneasr · 2021-09-14T10:42:46Z

persistence/sql/migrations/templates/20210913095309_identity_recovery_tokens.up.fizz

@@ -0,0 +1,2 @@
+add_column("identity_recovery_tokens", "identity_id", "uuid", {"size": 36})


This should have a foreign key :)

Fixed in c6f3d98

persistence/sql/migrations/templates/20210913095309_identity_recovery_tokens.down.fizz

aeneasr · 2021-09-14T10:45:06Z

persistence/sql/persister_recovery.go

@@ -75,12 +74,10 @@ func (p *Persister) UseRecoveryToken(ctx context.Context, token string) (*link.R
 		}

 		var ra identity.RecoveryAddress
-		if err := tx.Where("id = ? AND nid = ?", rt.RecoveryAddressID, nid).First(&ra); err != nil {
-			return sqlcon.HandleError(err)
+		if err := tx.Where("id = ? AND nid = ?", rt.RecoveryAddressID, nid).First(&ra); err == nil {


Any error would be ignored here. Please return them appropriately. You can use errors.Is(sql.ErrNoRows) to detect if the row could not be found

abador · 2021-09-15T07:04:19Z

@aeneasr I pushed the fixes and docs. Still the tests fail on running goimports. I'm not sure why this fails since I've got it configured locally and I see no errors :/

persistence/sql/persister_recovery.go

Co-authored-by: Patrik <[email protected]>

aeneasr

Almost :)

aeneasr · 2021-09-16T12:37:37Z

persistence/sql/migrations/templates/20210913095309_identity_recovery_tokens.up.fizz

+      "on_delete": "CASCADE",
+      "on_update": "RESTRICT",
+  })
+{{ end }}


You can add foreign keys in sqlite like so:

kratos/persistence/sql/migrations/templates/20210410175418_network.up.fizz

Line 4 in 4bec9ef

sql("ALTER TABLE selfservice_login_flows ADD COLUMN nid CHAR(36) NULL REFERENCES networks(id) ON DELETE CASCADE ON UPDATE RESTRICT")

Sorry missed that comment. I'll try to fix it today

@aeneasr if I add a column this way the migrations don't generate properly. The identity_id column is added i one of the first migrations and then is being removed after some temp table is generated and data is copied there. This causes tests to fail since the identity_id column doesn't exist
"sqlite create: table identity_recovery_tokens has no column named identity_id"

Could you share the full up/down migration?

@aeneasr 45ec59b

Did it help? :)

sorry @aeneasr no it didn't. I still get "sqlite create: table identity_recovery_tokens has no column named identity_id" . The only change that wasn't in the pr is in this file: 20210410175418000063_network.sqlite3.down.sql, but it doesn't matter.

Ok, I'll take a look

I just checked out your branch, ran rm -rf persistence/sql/migrations/sql/* && make migrations-render-replace and it worked. Not sure what's going on on your end?

Assuming tests pass, is this PR complete in your opinion?

I just checked out your branch, ran rm -rf persistence/sql/migrations/sql/* && make migrations-render-replace and it worked. Not sure what's going on on your end?

Assuming tests pass, is this PR complete in your opinion?

make migrations-render-replace works, but tests fail after that the same way. After regenerating migrations the only difference between my pr and local changes is the file I mentioned in the previous comment.

As for the PR I would say it's finished.

aeneasr · 2021-10-12T11:25:38Z

I have fixed the issues you faced. However, there are still a few tests failing. Those are related to your changes though. Could you please take a look? Thanks!

abador · 2021-10-14T09:15:07Z

I have fixed the issues you faced. However, there are still a few tests failing. Those are related to your changes though. Could you please take a look? Thanks!

Sorry @aeneasr I'll get back to all upstream tickets on friday or next week

aeneasr · 2021-10-20T12:08:38Z

While the PR is being worked on I will mark it as a draft. That declutters our review backlog :)

Once you're done with your changes and would like someone to review them, mark the PR as ready and request a review from one of the maintainers.

Thank you!

…t-email' into issue-1419-admin-recovery-without-email

abador · 2021-10-27T05:19:48Z

@aeneasr it seems one test failed, but it's unrelated to my changes (I did a squash and git push force yesterday when everything passed except ci/circleci: generate-openapi).

aeneasr · 2021-10-27T12:17:44Z

I tried pushing some changes required for merging the PR to your fork & branch, but it appears that I am not allowed to do so 😕

% git push ...
ERROR: Permission to push denied to aeneasr.
fatal: could not read from the remote repository.

Please make sure that you have the correct access rights
and the repository exists.

But the good news is, giving access is easy! ☺️ All you need to do is enable write access for maintainers. Thank you! 😄

If the repository belongs to an organization, please add me for the project as a collaborator!

codecov · 2021-10-28T10:10:58Z

Codecov Report

Merging #1750 (9bfe61e) into master (833f14f) will increase coverage by 0.00%.
The diff coverage is 92.59%.

@@           Coverage Diff           @@
##           master    #1750   +/-   ##
=======================================
  Coverage   75.13%   75.13%           
=======================================
  Files         292      292           
  Lines       15125    15127    +2     
=======================================
+ Hits        11364    11366    +2     
  Misses       2950     2950           
  Partials      811      811

Impacted Files	Coverage Δ
persistence/sql/persister_recovery.go	`80.00% <0.00%> (+0.45%)`	⬆️
selfservice/strategy/link/strategy_recovery.go	`64.15% <100.00%> (-0.93%)`	⬇️
selfservice/strategy/link/test/persistence.go	`100.00% <100.00%> (ø)`
selfservice/strategy/link/token_recovery.go	`100.00% <100.00%> (ø)`

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 273785a...9bfe61e. Read the comment docs.

aeneasr · 2021-10-29T05:48:12Z

Thank you for your hard work! :)

Make admin recovery to work without emails, ory#1419

a750020

abador requested review from aeneasr and zepatrik as code owners September 14, 2021 08:30

aeneasr requested changes Sep 14, 2021

View reviewed changes

CR fixes, ory#1419

c6f3d98

abador changed the title ~~Make admin recovery to work without emails, #1419~~ Make admin recovery to work without emails #1419 Sep 15, 2021

abador changed the title ~~Make admin recovery to work without emails #1419~~ feat: Make admin recovery to work without emails #1419 Sep 15, 2021

Docs, ory#1419

a6be25c

harnash approved these changes Sep 16, 2021

View reviewed changes

zepatrik reviewed Sep 16, 2021

View reviewed changes

persistence/sql/persister_recovery.go Show resolved Hide resolved

Update persistence/sql/persister_recovery.go

3492ca5

Co-authored-by: Patrik <[email protected]>

aeneasr requested changes Sep 16, 2021

View reviewed changes

abador and others added 4 commits September 22, 2021 12:24

Migration with foreign key for sqlite, ory#1419

45ec59b

Merge branch 'master' into issue-1419-admin-recovery-without-email

e8c943f

Merge branch 'master' into issue-1419-admin-recovery-without-email

142d4dd

chore: code review

db4b63f

aeneasr marked this pull request as draft October 20, 2021 12:08

abador added 9 commits October 21, 2021 13:57

Merge branch 'master' into issue-1419-admin-recovery-without-email

823050a

fix tests

f57f1bb

fix imports

99441b1

fix tests

58f696d

fix tests

5272633

Merge branch 'master' into issue-1419-admin-recovery-without-email

5eaedc4

try to fix tests

1fc3eb4

try to fix tests

48bcb38

try to fix tests

d19eede

abador added 5 commits October 26, 2021 09:08

try to fix tests

08a6d1f

Merge branch 'master' into issue-1419-admin-recovery-without-email

462df89

try to fix tests

6810666

Merge remote-tracking branch 'origin/issue-1419-admin-recovery-withou…

053da6b

…t-email' into issue-1419-admin-recovery-without-email

Cleanup tests

0c72018

abador force-pushed the issue-1419-admin-recovery-without-email branch from 27f317e to 0c72018 Compare October 26, 2021 21:19

abador marked this pull request as ready for review October 27, 2021 05:17

abador added 2 commits October 28, 2021 10:00

Merge branch 'master' into issue-1419-admin-recovery-without-email

b3ff59f

Merge branch 'master' into issue-1419-admin-recovery-without-email

9bfe61e

aeneasr merged commit db00e85 into ory:master Oct 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Make admin recovery to work without emails #1419 #1750

feat: Make admin recovery to work without emails #1419 #1750

abador commented Sep 14, 2021 •

edited

Loading

aeneasr left a comment

aeneasr Sep 14, 2021

abador Sep 15, 2021

aeneasr Sep 14, 2021

abador commented Sep 15, 2021

aeneasr left a comment

aeneasr Sep 16, 2021

abador Sep 21, 2021

abador Sep 22, 2021

aeneasr Sep 22, 2021

abador Sep 22, 2021

aeneasr Sep 23, 2021

abador Sep 28, 2021

aeneasr Sep 29, 2021

aeneasr Sep 29, 2021

abador Sep 30, 2021

aeneasr commented Oct 12, 2021

abador commented Oct 14, 2021

aeneasr commented Oct 20, 2021

abador commented Oct 27, 2021

aeneasr commented Oct 27, 2021

codecov bot commented Oct 28, 2021 •

edited

Loading

aeneasr commented Oct 29, 2021

		@@ -0,0 +1,2 @@
		add_column("identity_recovery_tokens", "identity_id", "uuid", {"size": 36})

feat: Make admin recovery to work without emails #1419 #1750

feat: Make admin recovery to work without emails #1419 #1750

Conversation

abador commented Sep 14, 2021 • edited Loading

Related issue(s)

Checklist

Further Comments

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abador commented Sep 15, 2021

aeneasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeneasr commented Oct 12, 2021

abador commented Oct 14, 2021

aeneasr commented Oct 20, 2021

abador commented Oct 27, 2021

aeneasr commented Oct 27, 2021

codecov bot commented Oct 28, 2021 • edited Loading

Codecov Report

aeneasr commented Oct 29, 2021

abador commented Sep 14, 2021 •

edited

Loading

codecov bot commented Oct 28, 2021 •

edited

Loading