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

Allow "+" character in usernames #36613

Merged
merged 4 commits into from
Dec 20, 2019
Merged

Allow "+" character in usernames #36613

merged 4 commits into from
Dec 20, 2019

Conversation

pako81
Copy link

@pako81 pako81 commented Dec 18, 2019

Description

Allow + character when creating users.

Related Issue

Motivation and Context

This PR allows the + character in the username when creating new users. This is also required when sharing with guest users having i.e. the following format name+2@test.com.

How Has This Been Tested?

Manually by creating new users having the + sign in their usernames over the Users management page as well by using the user:add occ command.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised
  • Changelog item

@codecov
Copy link

codecov bot commented Dec 19, 2019

Codecov Report

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

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #36613      +/-   ##
============================================
+ Coverage     64.67%   64.68%   +<.01%     
  Complexity    19099    19099              
============================================
  Files          1268     1268              
  Lines         74695    74695              
  Branches       1320     1320              
============================================
+ Hits          48309    48315       +6     
+ Misses        25998    25992       -6     
  Partials        388      388
Flag Coverage Δ Complexity Δ
#javascript 54.12% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 65.85% <100%> (ø) 19099 <0> (ø) ⬇️
Impacted Files Coverage Δ Complexity Δ
lib/private/User/Manager.php 86.18% <100%> (+3.94%) 53 <0> (ø) ⬇️
core/Command/User/Add.php 55.29% <100%> (ø) 16 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1b63244...80807c2. Read the comment docs.

@phil-davis phil-davis force-pushed the add-character-create-user branch from 28d5e5b to 6a66a22 Compare December 19, 2019 04:18
@phil-davis phil-davis self-requested a review December 19, 2019 04:21
@phil-davis
Copy link
Contributor

@pako81 I adjusted acceptance tests to include some cases with a user with + in the username. And also added a changelog entry.

IMO this works. @micbar this could be merged for 10.4 - is that OK?

@pako81
Copy link
Author

pako81 commented Dec 19, 2019

thanks a lot @phil-davis :)

@pako81
Copy link
Author

pako81 commented Dec 19, 2019

@phil-davis I was now thinking about the user provisioning APIs. In there, the + sign is still not correctly escaped but rather replaced with a space character.

@phil-davis
Copy link
Contributor

@pako81 I added provisioning API tests. They are passing locally for me and I expect that they will pass in drone CI.
Probably the tests are providing the + in the URL as ASCII 43, %2B (I think that is true, without actually digging deep into the test code)
And that is how any client using the Provisioning API would need to specify a username containing a + - and the backend code will receive the %2B turned back into a + and it "just works"

@pako81
Copy link
Author

pako81 commented Dec 19, 2019

@phil-davis correct, I was testing the wrong way. The following seems to work correctly.

curl -X POST http://admin:password@example.com/ocs/v1.php/cloud/users -d userid="name%2B1@test.com" -d password="test"

User name+1@test.com is correctly generated.

@phil-davis phil-davis force-pushed the add-character-create-user branch from f2129d4 to cb91305 Compare December 19, 2019 16:02
@phil-davis
Copy link
Contributor

There have been other major features merged just now. So I rebased this PR to make sure that the CI is all green on top of those changes now in master.

@phil-davis
Copy link
Contributor

@micbar @HanaGemela I have added acceptance test coverage and it passes (adding users with + in ther username via the webUI, cli and provisioning API and doing stuff with them...) So this looks like it works fine.

I some developer can review the code and approve, and you think this is a change/enhancement that we should have, then feel free to merge it for 10.4

@phil-davis phil-davis force-pushed the add-character-create-user branch from cb91305 to 8be87ce Compare December 20, 2019 01:19
Copy link
Contributor

@sharidas sharidas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link
Contributor

@karakayasemi karakayasemi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything seems good. As a last, this should be adjusted also.

'User ID used to login (must only contain a-z, A-Z, 0-9, -, _ and @).'

@pako81
Copy link
Author

pako81 commented Dec 20, 2019

thanks @karakayasemi :) missed that

@karakayasemi karakayasemi force-pushed the add-character-create-user branch from 7c34029 to 80807c2 Compare December 20, 2019 08:09
@karakayasemi karakayasemi self-requested a review December 20, 2019 09:46
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