Repository navigation
Conversation
Remove Credentials reported success but left the password in the OS keyring because of three stacked defects: - The view read a non-existent "type" key and passed it as the server. It now passes the selected server and the credential_type returned by list_stored_credentials. - delete_stored_credentials read the username from the top level of the metadata file, which is keyed by service name, so keyring.delete_password never ran. It now reads metadata[service_name]. - A KeyringError was logged and success still returned, and the view ignored the result. A keyring failure now returns a keyring_error result (metadata is kept so the entry can be retried) and the view shows st.error instead of st.success. Tests now assert the exact delete_password arguments, the view's call arguments, and that keyring failures surface as errors. Closes #342 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…y gone keyring raises PasswordDeleteError when there is nothing to delete. Treat that as success and remove the metadata entry, so a credential removed outside DataSure can still be cleared from the Remove dropdown. Refs #342 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
PasswordDeleteError can still trigger false success and remove retry metadata while the password remains stored.
1 open finding
What changed in this PR
Fixes DataSure’s credential-removal flow for #342, a v1.2.0 release blocker.
Changes:
- Passes the correct server and credential type from the view.
- Fixes the username lookup and reports keyring deletion failures.
- Adds regression tests and a changelog entry.
| File | Description |
|---|---|
| tests/views/test_import_view.py | Tests deletion arguments and success/error messages. |
| tests/utils/test_secure_credentials.py | Tests keyring calls and deletion outcomes. |
| src/datasure/views/import_view.py | Corrects deletion arguments and displays failures. |
| src/datasure/utils/secure_credentials.py | Fixes metadata lookup and keyring error handling. |
| CHANGELOG.md | Documents the credential-removal fix. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…dDeleteError The macOS Keychain and KWallet keyring backends raise PasswordDeleteError for access errors and cancelled unlocks, not only for missing passwords. Treating it as "already deleted" could report success and drop the metadata entry while the password stayed in the keyring. Now the metadata entry is cleared only when keyring.get_password confirms the password is absent; otherwise the deletion is reported as a failure and the entry is kept for retry. Addresses Copilot review on #344. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tials # Conflicts: # CHANGELOG.md
|
This branch has not been deployed
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
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.
Pull Request Summary 🚀
What does this PR do? 📝
Makes Remove Credentials actually delete the SurveyCTO password from the OS keyring, and report failure when it can't.
import_view.py): passes the selected server and credential type, which it used to mix up. It shows a success message only when the deletion succeeded, and an error otherwise.delete_stored_credentials(secure_credentials.py): reads the username from the right place in the metadata file. It used to always getNone, sokeyring.delete_passwordnever ran.Closes #342
Why is this change needed? 🤔
Users were told their credentials were deleted while the password stayed in the keyring, where anyone using the same OS account could retrieve it. Three defects stacked (see #342): wrong arguments from the view, a wrong metadata lookup, and swallowed errors. Release blocker for v1.2.0 (#339).
How was this implemented? 🛠️
selected_serverand thecredential_typekey thatlist_stored_credentialsactually returns.delete_stored_credentialsbuilds the service name the same way saving does and reads the username frommetadata[service_name].PasswordDeleteErrorcounts as success only afterkeyring.get_passwordconfirms the password is absent, so an entry whose password was removed outside DataSure can still be cleared from the dropdown. The macOS Keychain and KWallet backends also raisePasswordDeleteErrorfor access errors and cancelled unlocks, so if the password is still there, or the check itself fails, the result is a failure and the entry is kept.Merge danger: two-way door, easy to revert. Blast radius: narrow, limited to the Remove Credentials button. Saving and retrieving credentials are unchanged.
How to test or reproduce ? 🧪
Before: the existing tests asserted only that
delete_stored_credentialswas called, and asserted success when the keyring delete failed.After:
test_delete_credentials_successasserts the exact service name and username passed tokeyring.delete_password.test_delete_credentials_keyring_error_is_failure: a keyring error returns failure and keeps the entry.test_delete_credentials_already_removed_from_keyring: a password confirmed missing returns success and removes the entry.test_delete_credentials_password_delete_error_not_confirmed: aPasswordDeleteErrorwith the password still stored, or with a failing check, returns failure and keeps the entry.test_delete_credentials_buttonandtest_delete_credentials_failure_shows_error: the view passes the server and type, and shows an error on failure.All failed before the fix.
just test: 3910 passed, 6 skipped. The keyring is mocked throughout.Manual check: save credentials, click Remove, then confirm the
datasure_scto_login_<server>_<project>entry is gone from Windows Credential Manager or macOS Keychain, and from the dropdown.Screenshots (if applicable) 📷
N/A
Checklist ✅
Reviewer Emoji Legend
:code::smiley::+1::100:...and I want the author to know it! This is a way to highlight positive parts of a code review.
:star: :star: :star:And I am providing reasons why it needs to be addressed as well as suggested improvements.
:star: :star:And I am providing suggestions where it could be improved either in this PR or later.
:star:...and consider this a suggestion, not a requirement.
:question:This should be a fully formed question with sufficient information and context that requires a response.
:memo::pick:This does not require any changes and is often better left unsaid. This may include stylistic, formatting, or organization suggestions and should likely be prevented/enforced by linting if they really matter
:recycle:Should include enough context to be actionable and not be considered a nitpick.
🤖 Generated with Claude Code