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 plus in LDAP usernames #490

Merged
merged 4 commits into from
Mar 6, 2020
Merged

Allow plus in LDAP usernames #490

merged 4 commits into from
Mar 6, 2020

Conversation

phil-davis
Copy link
Contributor

@phil-davis phil-davis commented Dec 19, 2019

core PR owncloud/core#36613 proposes allowing + in usernames.

If that PR is accepted, then we should/could also allow + in usernames coming from LDAP.

This PR implements that, and adds unit tests to check the LDAP sanitizeUsername() method. (There were not any tests for that yet)

  • set minimum core version to 10.4 (we only want to allow sync of LDAP usernames containing + if core also supports usernames containing +)

IMO this should only be merged and released after core 10.4 has been released.

@phil-davis
Copy link
Contributor Author

@pako81 this would also be needed if we start allowing + in usernames/UIDs

@codecov
Copy link

codecov bot commented Dec 19, 2019

Codecov Report

Merging #490 into master will increase coverage by 0.16%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #490      +/-   ##
============================================
+ Coverage     36.75%   36.91%   +0.16%     
  Complexity     1305     1305              
============================================
  Files            31       31              
  Lines          3700     3700              
============================================
+ Hits           1360     1366       +6     
+ Misses         2340     2334       -6     
Impacted Files Coverage Δ Complexity Δ
lib/Access.php 7.67% <0.00%> (+0.80%) 281.00% <0.00%> (ø%)

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 195e509...b1677a7. Read the comment docs.

@pako81
Copy link

pako81 commented Dec 19, 2019

@phil-davis makes sense 👍

@phil-davis
Copy link
Contributor Author

CI here is "stuck" - with core min version 10.4 we cannot have test pipelines against latest.
But there is no rush - we can run CI again when core 10.4 is latest.

@mmattel
Copy link
Contributor

mmattel commented Jan 30, 2020

@phil-davis has there anything changed that allows a progress 😅

@phil-davis phil-davis force-pushed the allow-plus-in-ldap-users branch from d32daff to ac9ab45 Compare January 30, 2020 14:41
@phil-davis
Copy link
Contributor Author

I rebased to catch up with current user_ldap.

There is no point releasing this anyway, until core 10.4 is out. So we can easily merge this the day that 10.4 community release happens and release user_ldap soon after.

@phil-davis phil-davis force-pushed the allow-plus-in-ldap-users branch from ac9ab45 to efe21b3 Compare February 11, 2020 12:28
@phil-davis phil-davis force-pushed the allow-plus-in-ldap-users branch from efe21b3 to 2c96ba9 Compare February 21, 2020 12:14
@phil-davis
Copy link
Contributor Author

phil-davis commented Feb 27, 2020

@micbar @HanaGemela I adjusted CI so that it does not test against core latest (which is 10.3.2)

CI should pass. We could merge this PR and start the process for a user_ldap release to go with core 10.4.0

@phil-davis phil-davis force-pushed the allow-plus-in-ldap-users branch from 2c96ba9 to 659a0d0 Compare February 27, 2020 06:00
@micbar
Copy link
Contributor

micbar commented Feb 27, 2020

@phil-davis Does it really have a strickt dependency? what would happen, if I use the current version with 10.4.0?

@phil-davis
Copy link
Contributor Author

If your LDAP does not actually have any usernames containing + then yes, it should still work on a core 10.2 or 10.3 system.

Do you want to try and keep the new version of user_ldap installable on 10.2 and 10.3?

@phil-davis phil-davis force-pushed the allow-plus-in-ldap-users branch from 659a0d0 to b276b07 Compare March 3, 2020 11:12
@phil-davis
Copy link
Contributor Author

Note: if we wait until core 10.4.0 becomes the "latest" community tarball, then I can remove the last commit from this PR.

@HanaGemela HanaGemela mentioned this pull request Mar 3, 2020
31 tasks
@phil-davis phil-davis force-pushed the allow-plus-in-ldap-users branch from b276b07 to b1677a7 Compare March 4, 2020 17:00
@phil-davis
Copy link
Contributor Author

Note: if we wait until core 10.4.0 becomes the "latest" community tarball, then I can remove the last commit from this PR.

10.4.0 is now "latest". I reverted the 3rd commit. Let's see what happens to CI.

@mmattel
Copy link
Contributor

mmattel commented Mar 6, 2020

Green, green 😄

@phil-davis
Copy link
Contributor Author

@micbar please get a review of this.

@micbar micbar merged commit dc2e8a4 into master Mar 6, 2020
@delete-merged-branch delete-merged-branch bot deleted the allow-plus-in-ldap-users branch March 6, 2020 13:42
@HanaGemela HanaGemela added this to the QA milestone Mar 9, 2020
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