Ensuring that Users have a username #1379
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
jpwhite4
reviewed
Jul 16, 2020
| @@ -12,7 +12,7 @@ | |||
| { | |||
| "name": "username", | |||
| "type": "varchar(200)", | |||
| "nullable": true | |||
| "nullable": false | |||
There was a problem hiding this comment.
Should we worry about the situation where an existing install has a null username (for whatever reason) and then the upgrade fails?
There was a problem hiding this comment.
We talked about that exact thing. That is why this was moved to 9.5 for the moment, so we can think about all the ways this could already be broken. I cant see a way for the user to login, so I would say we should just delete the user that have null... as part of the upgrade
plessbd
reviewed
Jul 16, 2020
ryanrath
changed the title
ryanrath
changed the title
plessbd
changed the title
ryanrath
force-pushed
the
make_save_user_great_again
branch
2 times, most recently
from
a4aa427
to
f1edd93
Compare
|
Note: For anybody reviewing this one. SonarCloud has a hissy fit whenever you merge in changes from upstream, rebase some local changes on top of that and then force push it up. The duplicate code has nothing to do with this PR. |
Co-authored-by: Ben Plessinger <[email protected]>
This change applies the same validation that is used when creating a new user via the Internal Dashboard ( `create_user.php` ) to the saveUser function.
These two changes will ensure that upgrades for existing installations that have managed to create a user with a null username will complete sucessfully instead of erroring out when the ETL framework attempts to update the User table. Once these null username Users have are converted to empty string username Users then a SQL script will be run that deactivates the User as it should be possible to create a user with an empty / null username.
Since we can't have Users with empty / null usernames anymore we needed to
replace the method used to generate a username. This method relies on the
uniqueness of `uniqid("", true)` to ensure that we get a unique username the
first time without having to check a second time.
It turns out that things work better ( or at all ) when you have them running in the correct migration.
Since we updated the way our test usernames were generated in the last commit, we need to update the expected exception message when a user is not found.
ryanrath
force-pushed
the
make_save_user_great_again
branch
from
b122cc7
to
bf4233f
Compare
jpwhite4
approved these changes
Mar 31, 2021
jpwhite4
added
Category:General
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.
NOTE: this is up for discussion / inclusion in 9.5
Description
Just something we ran across while testing things for the OpenOnDemand / XDMoD tutorial. Turns out that you can end up with user's without a username. These changes ensure that when
saveUseris called that an exception is thrown if_usernameis empty. We've also updated the username column to not accept null values & to also have a unique index.Motivation and Context
No more null username Users!
Tests performed
Types of changes
Checklist: