Skip to content

Allow "+" character in usernames - #36613

Merged
micbar merged 4 commits into
masterfrom
add-character-create-user
Dec 20, 2019
Merged

Allow "+" character in usernames#36613
micbar merged 4 commits into
masterfrom
add-character-create-user

Conversation

@pako81

@pako81 pako81 commented Dec 18, 2019

Copy link
Copy Markdown

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

codecov Bot commented Dec 19, 2019

Copy link
Copy Markdown

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
Comment thread lib/private/User/Manager.php
@phil-davis
phil-davis self-requested a review December 19, 2019 04:21
@phil-davis

Copy link
Copy Markdown
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

pako81 commented Dec 19, 2019

Copy link
Copy Markdown
Author

thanks a lot @phil-davis :)

@pako81

pako81 commented Dec 19, 2019

Copy link
Copy Markdown
Author

@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
Copy Markdown
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

pako81 commented Dec 19, 2019

Copy link
Copy Markdown
Author

@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
Copy Markdown
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
Copy Markdown
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

@sharidas sharidas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM 👍

@karakayasemi karakayasemi left a comment

Copy link
Copy Markdown
Contributor

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

pako81 commented Dec 20, 2019

Copy link
Copy Markdown
Author

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

5 participants