-
-
Notifications
You must be signed in to change notification settings - Fork 801
Hash application client secrets using Django password hashing #1093
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.
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
Conversation
@pkarman - Thanks for the idea regarding client secret hashing. I've simplified by turning this into a breaking change (migration not reversible) that will always hash the client secret. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1093 +/- ##
==========================================
+ Coverage 96.58% 96.61% +0.02%
==========================================
Files 32 32
Lines 1787 1801 +14
==========================================
+ Hits 1726 1740 +14
Misses 61 61 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, it LGTM!
…jazzband#1020) Co-authored-by: Alan Crosswell <[email protected]>
Apply suggestions from code review Co-authored-by: Andrew Chen Wang <[email protected]>
1cf4259
to
b5f6674
Compare
…nd#1093) * Add ClientSecretField field to use Django password hashing algorithms (jazzband#1020) Co-authored-by: Andrew Chen Wang <[email protected]> Co-authored-by: Peter Karman <[email protected]> Co-authored-by: Andrew Chen Wang <[email protected]>
If feels wrong to see "WIP" in the title of a merged PR / to see WIP in a breaking change in the changelog. |
Fixes #729
Description of the Change
Pick up where #1020 left off and simplify to always hash the client_secret using the Django default hasher.
The migration is not reversible as it hashes cleartext application client secrets.
Checklist
CHANGELOG.md
updated (only for user relevant changes)AUTHORS