Remove password from credentials object #4661
Merged
Conversation
This file contains 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
|
@tariqajyusuf is attempting to deploy a commit to the Ethyca Team on Vercel. A member of the Team first needs to authorize it. |
|
Tested successfully on this PR branch: |
|
There are some CI test failures, however, as the tests expect the password to be present e.g. https://github.com/ethyca/fides/blob/main/tests/ctl/core/test_user.py |
|
@tariqajyusuf thank you for the updates here! to clear up the CI failure, i think you can just remove the password references in that test file. namely, removing L23 and L28 of let me know if you have any difficulties doing this or if you don't think it's the right approach! i can also make the changes for you if you'd prefer, just a bit lazy at the moment to check out your fork 😆 but happy to do it if that's better! |
|
No worries, fixed @adamsachs @daveqnet |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4661 +/- ##
==========================================
- Coverage 86.67% 86.67% -0.01%
==========================================
Files 336 336
Lines 20090 20089 -1
Branches 2581 2581
==========================================
- Hits 17414 17413 -1
Misses 2203 2203
Partials 473 473 ☔ View full report in Codecov by Sentry. |
|
Much appreciated @tariqajyusuf! Just one final request: can you add the following in the top Unreleased section of I'd do it myself, but I'd have to request permissions on your fork or raise a separate PR - thanks! |
|
Done! |
daveqnet
reviewed
Mar 5, 2024
CHANGELOG.md
Outdated
| @@ -30,6 +30,7 @@ The types of changes are: | |||
|
|
|||
| ### Changed | |||
| - Update when GPP API reports signal status: ready [#4635](https://github.com/ethyca/fides/pull/4635) | |||
| - Modify `fides user login` to not store plaintext password in `~/.fides-credentials` [#4661](https://github.com/ethyca/fides/pull/4661) | |||
There was a problem hiding this comment.
Sorry Tariq, I should have been clearer, this changelog entry should go up further in the [Unreleased] section, not here in the [2.31.0] section.
Because of the timing, this particular change will be included in a later release than 2.31.0.
|
Whoops that's my bad, was caught multitasking and put it in the wrong section |
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.
Fixes issue introduced in #2153
Description Of Changes
fides user loginstores the plaintext password to the~/.fides-credentialsfile. This removes the password since it's not actively used anywhere.Code Changes
Steps to Confirm
fides user loginand login with credentials.~/.fides-credentialsand ensure password is not storedPre-Merge Checklist
CHANGELOG.md