Add "remember password" functionality with secure storage#63
Merged
Conversation
…ration - Introduce SecretStorageService to manage "remember password" data securely in Obsidian's SecretStorage. - Add auto-unlock capability leveraging saved passwords for profiles and keyring. - Enhance modals and settings with "Remember password" toggles and forget password options. - Update runtime logic to auto-unlock profiles using saved credentials, improving user convenience.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdded a SecretStorageService to persist keyring and per-profile passwords, wired it into the plugin and settings, added "Remember password" UI and logic to unlock/keyring modals, auto-attempted unlocking referenced profiles on file-open when enabled, and cleared stored secrets on deletion or invalid-key errors. Changes
Sequence DiagramsequenceDiagram
participant User
participant Plugin as VaultCryptPlugin
participant FileOpen as File-Open Handler
participant SecretSvc as SecretStorageService
participant KeyringSvc as KeyringService
participant Session as SessionService
User->>Plugin: Open note referencing locked profiles
activate Plugin
FileOpen->>FileOpen: check settings.security.autoUnlock
alt autoUnlock enabled
FileOpen->>SecretSvc: autoUnlockProfiles(settings, lockedIds, Session, KeyringSvc)
SecretSvc->>KeyringSvc: load keyring password & getProfilePasswords()
KeyringSvc-->>SecretSvc: profile passwords / error
loop each locked profile
SecretSvc->>Session: unlockProfile(profileId, password)
Session-->>SecretSvc: success / failure
alt InvalidKey error
SecretSvc->>SecretSvc: forgetKeyringPassword() or forgetProfilePassword(profileId)
end
end
end
FileOpen-->>User: Note opened (profiles unlocked where possible)
deactivate Plugin
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main.ts (1)
333-379: Consider moving the auto-unlock flow out ofsrc/main.ts.This block is feature logic plus credential policy inside the
file-openhandler, which makessrc/main.tsharder to scan and unit test. Extracting it into a dedicated helper/service would also give you one place to own the saved-credential behavior above. As per coding guidelines, "src/main.ts: Keep main.ts minimal and focused on plugin lifecycle (loading, unloading, registering commands). Delegate all feature logic to separate modules."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.ts` around lines 333 - 379, The auto-unlock logic inside the file-open handler (conditioned on settings.security.autoUnlock) should be extracted from src/main.ts into a dedicated AutoUnlockService (or helper) to keep main.ts minimal; move the keyring and per-profile flows—including uses of secretStorageService.loadKeyringPassword, keyringService.getProfilePasswords, secretStorageService.forgetKeyringPassword, secretStorageService.loadProfilePassword, secretStorageService.forgetProfilePassword, and sessionService.unlockProfile—into a single method (e.g., AutoUnlockService.autoUnlockProfiles(settings, lockedProfileIds)) that returns the updated lockedProfileIds (or mutates a passed Set) and handles error cases currently in main.ts, then call that method from main.ts so main only orchestrates lifecycle and the new service owns saved-credential behavior and unit tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.ts`:
- Around line 344-377: The current catches unconditionally call
secretStorageService.forgetKeyringPassword() and forgetProfilePassword() on any
error; change them to only clear stored credentials when the thrown error is
explicitly an “invalid credentials” type (e.g., a distinct
InvalidCredentialsError or an error with a specific code/name returned by
keyringService.getProfilePasswords and sessionService.unlockProfile). Update
keyringService.getProfilePasswords and sessionService.unlockProfile to throw or
annotate errors with a unique identifier for bad-password cases (or map known
error messages to that identifier), then in the catch blocks check for that
identifier and call secretStorageService.forgetKeyringPassword() or
forgetProfilePassword(profileId) only when it matches; for all other errors
rethrow (or let them propagate) so transient I/O/corruption issues don’t erase
stored passwords.
In `@src/modals/keyring-modals.ts`:
- Around line 242-246: After successfully calling changeMasterPassword(...) in
ChangeKeyringPasswordModal.submit(), update the persisted secret to reflect the
new master password: if the keyring password is currently remembered (use the
existing remember flag or query secretStorageService for current state), call
this.plugin.secretStorageService.saveKeyringPassword(this.newPassword or
submittedPassword) to overwrite the stored value; if the user opted not to
remember, call this.plugin.secretStorageService.forgetKeyringPassword() so the
saved secret isn't stale. Ensure these calls occur only after
changeMasterPassword(...) resolves successfully.
In `@src/secret-storage-service.ts`:
- Around line 17-65: The save*/forget* methods (saveProfilePassword,
forgetProfilePassword, saveKeyringPassword, forgetKeyringPassword) currently
swallow all exceptions making writes/clears appear successful; change their
signatures to return a boolean (or throw) to surface failures, catch only
expected SecretStorage-unavailable errors and return false, and rethrow or
return false while logging unexpected errors so callers/UI can show
success/failure; keep loadProfilePassword/loadKeyringPassword behavior (return
null on failure) for parity and use the same KEY_PREFIX + key names when
handling errors.
---
Nitpick comments:
In `@src/main.ts`:
- Around line 333-379: The auto-unlock logic inside the file-open handler
(conditioned on settings.security.autoUnlock) should be extracted from
src/main.ts into a dedicated AutoUnlockService (or helper) to keep main.ts
minimal; move the keyring and per-profile flows—including uses of
secretStorageService.loadKeyringPassword, keyringService.getProfilePasswords,
secretStorageService.forgetKeyringPassword,
secretStorageService.loadProfilePassword,
secretStorageService.forgetProfilePassword, and
sessionService.unlockProfile—into a single method (e.g.,
AutoUnlockService.autoUnlockProfiles(settings, lockedProfileIds)) that returns
the updated lockedProfileIds (or mutates a passed Set) and handles error cases
currently in main.ts, then call that method from main.ts so main only
orchestrates lifecycle and the new service owns saved-credential behavior and
unit tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 96a19242-a320-4cf4-ad60-df91a2606da7
📒 Files selected for processing (5)
src/main.tssrc/modals/keyring-modals.tssrc/modals/unlock-modal.tssrc/secret-storage-service.tssrc/settings.ts
src/modals/keyring-modals.ts
Outdated
Comment on lines
+242
to
+246
| if (remember) { | ||
| this.plugin.secretStorageService.saveKeyringPassword(submittedPassword); | ||
| } else { | ||
| this.plugin.secretStorageService.forgetKeyringPassword(); | ||
| } |
There was a problem hiding this comment.
Re-enroll remembered keyring credentials after a password change.
This PR now persists the keyring master password, but ChangeKeyringPasswordModal.submit() only updates the KDBX file. The saved secret stays stale until the next auto-unlock fails once, which also misses the linked requirement to re-set up saved unlock after a master-password change. After changeMasterPassword(...) succeeds, if a keyring password is currently remembered, either overwrite it with this.newPassword or clear it and prompt the user to save it again.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/modals/keyring-modals.ts` around lines 242 - 246, After successfully
calling changeMasterPassword(...) in ChangeKeyringPasswordModal.submit(), update
the persisted secret to reflect the new master password: if the keyring password
is currently remembered (use the existing remember flag or query
secretStorageService for current state), call
this.plugin.secretStorageService.saveKeyringPassword(this.newPassword or
submittedPassword) to overwrite the stored value; if the user opted not to
remember, call this.plugin.secretStorageService.forgetKeyringPassword() so the
saved secret isn't stale. Ensure these calls occur only after
changeMasterPassword(...) resolves successfully.
…ser feedback - Update save/forget methods to return success/failure booleans for better error handling. - Add notices to alert users when secret storage operations fail. - Extract auto-unlock logic into `autoUnlockProfiles` for maintainability. - Synchronize saved keyring passwords during changes for consistent auto-unlock behavior. - Improve code readability with duck-typing for keyring error detection.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/secret-storage-service.ts`:
- Around line 111-147: autoUnlockProfiles can be invoked concurrently causing
duplicate unlock attempts; make it idempotent by checking
sessionService.isUnlocked(profileId) before calling sessionService.unlockProfile
and skip/delete the profile if already unlocked, and additionally track an
in-flight set (e.g., local Set<string> in autoUnlockProfiles) to avoid starting
multiple unlocks for the same profile while awaiting the first attempt; update
the loop that iterates over passwords and the per-profile unlock at line ~156 to
consult sessionService.isUnlocked(profileId) and the in-flight set before
calling unlockProfile and to remove entries from lockedProfileIds only after a
successful unlock.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f896e7c5-68d1-4a16-9765-8df51c4fa824
📒 Files selected for processing (5)
src/main.tssrc/modals/keyring-modals.tssrc/modals/unlock-modal.tssrc/secret-storage-service.tssrc/settings.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main.ts
- src/modals/keyring-modals.ts
…retStorageService
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of Changes
SecretStorageServiceto securely store and manage "remember password" data.Checklist
Closes #31
Summary by CodeRabbit