Skip to content

seedless onboarding password sync #6

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 25 commits into
base: feat/seedless-multi-srp
Choose a base branch
from
Draft

seedless onboarding password sync #6

wants to merge 25 commits into from

Conversation

tuna1207
Copy link
Member

@tuna1207 tuna1207 commented Apr 25, 2025 •
edited
Loading

Explanation

Handle password sync flow

  • Add checkIsPasswordOutdated method (which is cached for 15 mins)
  • Add password check to fetch all SRPs and change password
  • Add recover password method to recover old password using latest global password
  • Add sync latest global password to reset vault to use latest password and persist latest auth pubkey

References

https://docs.google.com/document/d/1r7WwVgrdmBLzhX7yYDrVc-NEs_Q02VPBGXi2-VGQAyc/edit?tab=t.0

Changelog

  • Add checkIsPasswordOutdated method
  • Add password check to fetch all SRPs and change password
  • Add recoverPassword method
  • Add syncLatestGlobalPassword method

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've communicated my changes to consumers by updating changelogs for packages I've changed, highlighting breaking changes as necessary
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

@tuna1207 tuna1207 changed the base branch from main to feat/seedless-onboarding-toprf April 25, 2025 01:43
@lwin-kyaw lwin-kyaw changed the base branch from feat/seedless-onboarding-toprf to feat/seedless-multi-srp April 25, 2025 02:21
@tuna1207 tuna1207 force-pushed the feat/seedless-onboarding-password-sync branch from e6e424b to 60cff9b Compare April 25, 2025 05:00
lwin-kyaw
lwin-kyaw reviewed Apr 25, 2025
View reviewed changes
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts Outdated
@@ -457,6 +477,71 @@ export class SeedlessOnboardingController extends BaseController<
this.#isUnlocked = false;
}

/**
* @description Fetch the password corresponding to the current authPubKey in state (current device password).
Copy link

Choose a reason for hiding this comment

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

Suggested change
* @description Fetch the password corresponding to the current authPubKey in state (current device password).
* @description Fetch the latest password corresponding to the current authPubKey in state (current device password).

Copy link
Member Author

Choose a reason for hiding this comment

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

@lwin-kyaw actually it's not latest, it's the old password (the current password being used for this device) which is already out of sync with the current global password so that user just have to input the latest global password when they are logged out because their device password went out of sync, then we use this recovered old password to unlock the vault and set the password to the new global password

Copy link

Choose a reason for hiding this comment

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

Ah, I'm abit confused by the term global.

@lwin-kyaw
Copy link

lgtm.

himanshuchawla009
himanshuchawla009 requested changes Apr 29, 2025
View reviewed changes
Copy link
Member

@himanshuchawla009 himanshuchawla009 left a comment

Choose a reason for hiding this comment

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

good job, minor comments

tuna1207 added 2 commits April 30, 2025 14:53
@tuna1207
refactor: use noble lib to deserialize pubkey and compare curve point
63a4fa2
@tuna1207
Merge branch 'feat/seedless-multi-srp' into feat/seedless-onboarding-…
eef0ebd 
…password-sync

# Conflicts:
#	packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts
#	packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
himanshuchawla009
himanshuchawla009 approved these changes Apr 30, 2025
View reviewed changes
Copy link
Member

@himanshuchawla009 himanshuchawla009 left a comment

Choose a reason for hiding this comment

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

lgtm

tuna1207 and others added 9 commits May 6, 2025 10:02
@tuna1207 tuna1207 force-pushed the feat/seedless-onboarding-password-sync branch from d7c3553 to 9cf2022 Compare May 14, 2025 14:51
chaitanyapotti
chaitanyapotti approved these changes May 14, 2025
View reviewed changes
Copy link
Member

@chaitanyapotti chaitanyapotti left a comment

Choose a reason for hiding this comment

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

Lgtm

@matthiasgeihs
Copy link

@tuna1207 To satisfy the changelog checker, you need to add an entry to the CHANGELOG.md that references this PR.
See here, for example.

matthiasgeihs
matthiasgeihs reviewed May 16, 2025
View reviewed changes
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
await this.#assertPasswordInSync({
skipCache: true,
});
// NOTE don't include #assertPasswordInSync in #withControllerLock since #assertPasswordInSync already acquires the controller lock
Copy link

Choose a reason for hiding this comment

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

technically, shouldn't this be called within the same lock?
maybe there should be a flag in assertPasswordInSync that allows this to be called from within an existing lock.

packages/seedless-onboarding-controller/src/SeedlessOnboardingController.ts
idTokens: hashedIdTokenHexes,
groupedAuthConnectionParams: {
authConnectionId,
return await this.#withControllerLock(async () => {
Copy link

Choose a reason for hiding this comment

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

It's a bit hard to see which changes are because of adding the controller lock, and which are because of adding password syncing functionality. i think ideally this could have been split into 2 PRs. it's OK now, just mentioning it for next time.

packages/seedless-onboarding-controller/toprf-secure-backup.tgz
Copy link

Choose a reason for hiding this comment

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

this needs to be removed

matthiasgeihs
matthiasgeihs requested changes May 16, 2025
View reviewed changes
packages/seedless-onboarding-controller/src/SeedlessOnboardingController.test.ts
Copy link

Choose a reason for hiding this comment

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

we should have a test that checks for SeedlessOnboardingControllerError.OutdatedPassword when trying to add a backup item using outdated keys, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lwin-kyaw lwin-kyaw lwin-kyaw left review comments

@matthiasgeihs matthiasgeihs matthiasgeihs requested changes

@chaitanyapotti chaitanyapotti chaitanyapotti approved these changes

@himanshuchawla009 himanshuchawla009 himanshuchawla009 approved these changes

@huggingbot huggingbot Awaiting requested review from huggingbot

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tuna1207 @lwin-kyaw @matthiasgeihs @chaitanyapotti @himanshuchawla009