Skip to content
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

Check proper case for user UID #9633

Closed
wants to merge 1 commit into from
Closed

Conversation

skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented May 28, 2018

Fix #9532

Testing:
$.get('/ocs/v2.php/cloud/users/admin') 200
$.get('/ocs/v2.php/cloud/users/Admin') 404

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@rullzer
Copy link
Member

rullzer commented May 28, 2018

Nope!

This is not the way to go. A lot of things will do 💥. (like people that use case insentivie login from the desktop or android client).

The loginname is case insensitive on our database backend.

@skjnldsv
Copy link
Member Author

The loginname is case insensitive on our database backend.

I never knew! :o

Well, I don't see any other proper way to do it. Adding a check on every userExists function in the provisioning api seems like a terrible way to implement such verification. :/

@rullzer
Copy link
Member

rullzer commented May 28, 2018

Well then the provisioningAPI will just return non sensible data if you don't case the userid correctly. Better than the alternative now ;)

@skjnldsv
Copy link
Member Author

skjnldsv commented May 28, 2018

@rullzer our current backend return the uid we request, so there is no way to check if getUID === requestedUID since it will always be true.
Our cache is populated that way :/

@codecov
Copy link

codecov bot commented May 28, 2018

Codecov Report

Merging #9633 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##             master   #9633      +/-   ##
===========================================
+ Coverage      51.7%   51.7%   +<.01%     
- Complexity    25706   25707       +1     
===========================================
  Files          1635    1635              
  Lines         95957   95960       +3     
  Branches       1384    1384              
===========================================
+ Hits          49611   49613       +2     
- Misses        46346   46347       +1
Impacted Files Coverage Δ Complexity Δ
lib/private/User/Database.php 79.22% <100%> (-0.25%) 44 <0> (+1)

blizzz
blizzz previously approved these changes May 28, 2018
@rullzer
Copy link
Member

rullzer commented May 28, 2018

@skjnldsv k let me think if there is a solutoion. But this is not it ;)

@rullzer rullzer closed this May 28, 2018
@rullzer rullzer deleted the case-sensitive-users-api branch May 28, 2018 13:44
@blizzz
Copy link
Member

blizzz commented May 28, 2018

loadUser is called from userExists and getDisplayName. Both expect the current uid. Auth goes via checkpassword, which is and stays case insensitive.

@rullzer
Copy link
Member

rullzer commented May 28, 2018

@blizzz mmmm that might be true... lets see.

@rullzer rullzer restored the case-sensitive-users-api branch May 28, 2018 13:47
@rullzer rullzer reopened this May 28, 2018
@rullzer
Copy link
Member

rullzer commented May 28, 2018

Ok we should do some extensive testing here to make sure it keeps working.

@skjnldsv
Copy link
Member Author

Well, we already know that with this patch, we can create 'test0' and Test0' users.
So we need an extensive testing here as well :)

Maybe keep the non sensitive case check on the userExists check?

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels May 28, 2018
@blizzz
Copy link
Member

blizzz commented May 28, 2018

one valid case that @rullzer described: the local backend uses it when creating users, it would still need to make sure that the user id is not taken. what createuser does not do however: asking the User Managers userExists to check against other user backends.

Suggestion: we could phase out userExists, point towards getUser('uid') as a test for existence, and introduce a isUserIDReserved or something like that for user backends to see whether they can give away that one.

@skjnldsv
Copy link
Member Author

skjnldsv commented Jun 7, 2018

@blizzz @rullzer don't forget about this please :D

@nextcloud-bot nextcloud-bot added the stale Ticket or PR with no recent activity label Jun 29, 2018
@MorrisJobke
Copy link
Member

@rullzer @nickvergessen Mind to review?

@nextcloud-bot nextcloud-bot removed the stale Ticket or PR with no recent activity label Jul 11, 2018
@MorrisJobke MorrisJobke mentioned this pull request Jul 24, 2018
21 tasks
@MorrisJobke
Copy link
Member

Most likely nothing for 14 -> moving to 15.

@MorrisJobke
Copy link
Member

I guess we need to move this to 16 😢

@MorrisJobke MorrisJobke modified the milestones: Nextcloud 15, Nextcloud 16 Nov 7, 2018
@skjnldsv
Copy link
Member Author

skjnldsv commented Nov 7, 2018

I'm guessing we can close @MorrisJobke

@skjnldsv skjnldsv closed this Nov 7, 2018
@MorrisJobke MorrisJobke deleted the case-sensitive-users-api branch November 7, 2018 23:38
@MorrisJobke MorrisJobke removed this from the Nextcloud 16 milestone Nov 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants