-
Notifications
You must be signed in to change notification settings - Fork 36
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
[ IMPR ] hashed passwords #237
Merged
+301
−145
Merged
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
eb1dfb5
base for hashed passwords
Winston-Hsiao 4dd551f
small lint fix
Winston-Hsiao 666b047
docstring for password util file
Winston-Hsiao d6971d0
user and oauth normalized + required crud
Winston-Hsiao e8f453a
added tests for oauth user sign ups
Winston-Hsiao 1b746a2
updated requirements.txt
Winston-Hsiao 6383650
fix module import
Winston-Hsiao 358e3e5
update tests to accommodate new models/crud
Winston-Hsiao b137f21
Merge branch 'master' into 236-hashed-password
Winston-Hsiao 55286d5
password strength/validation and refined auth forms
Winston-Hsiao 8ad770f
resolve PR comments
Winston-Hsiao File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
user and oauth normalized + required crud
commit d6971d0808e788d0ef837dfb76432a203e1bb0ee
There are no files selected for viewing
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
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
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.
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.
is it advisable to keep these fields here? what are we going to use them for? in theory we could look this up from
OAuth
if we need to (although it would be kind of gross), i'm just not sure what we would use these IDs for once we have already retrieved the email. apologies if you'd addressed this already in the docThere 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.
good point,
only case I could see now is if someone wanted to unlink an OAuth provider. We have their user data already loaded (they're a signed in user and want to set a password and unlink the provider without deleting/resetting their account and all their listings/artifacts/data), we can then backtrace and search the OAuthKey based on their linked/associated OAuthProvider (that we have stored on their user object) otherwise we'd have to somehow persist the OAuth token past sign up and in cookies or something. Then we'd do the CRUD to delete the key and set their provider to empty string, also for consistency in the user model if a hashed_password value is not set it is assumed they have a populated
github_id
orgoogle_id
so having the 2 string fields exist could be useful for some validation checks and in terms of db storage space it's pretty light and not much of a concern.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.
Leaving in for now, resolved everything else and need approval for merge.
But can revisit this in the future if it really seems redundant/unnecessary we can remove them to keep the user model more lean.