Skip to content

do not create empty userid when attribute does not have allowed chars - #8634

Merged
rullzer merged 3 commits into
masterfrom
ldap-no-empty-names
Mar 5, 2018
Merged

do not create empty userid when attribute does not have allowed chars#8634
rullzer merged 3 commits into
masterfrom
ldap-no-empty-names

Conversation

@blizzz

@blizzzblizzz commented Mar 2, 2018

Copy link
Copy Markdown
Member

If some funny attribute contains only characters that are not allowed for user ids, one empty userid would have been generated. Comes with unit test.

@blizzz
do not create empty userid when attribute does not have allowed chars
8607992
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzzblizzz added this to the Nextcloud 14 milestone Mar 2, 2018
MorrisJobke
MorrisJobke reviewed Mar 5, 2018
View reviewed changes
Comment threadapps/user_ldap/lib/Access.php Outdated
'level' => Util::WARN,
]);
// we don't attempt to set a username here. We can go for
// for an alternativ 4 digit random number as we would append

@MorrisJobkeMorrisJobkeMar 5, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

alternative ;)

MorrisJobke
MorrisJobke reviewed Mar 5, 2018
View reviewed changes
Comment threadapps/user_ldap/lib/Access.php
$name = preg_replace('/[^a-zA-Z0-9_.@-]/u', '', $name);

if($name === '') {
throw new \InvalidArgumentException('provided name template for username does not contain any allowed characters');

@MorrisJobkeMorrisJobkeMar 5, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Missing in PHPDoc

@blizzzblizzzMar 5, 2018

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

mh, PhpStorm not complaining, usually it does… odd. Anyway you're right.

MorrisJobke
MorrisJobke approved these changes Mar 5, 2018
View reviewed changes

@MorrisJobkeMorrisJobke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Beside the little nitpicks: 👍

@MorrisJobke

MorrisJobke commented Mar 5, 2018

Copy link
Copy Markdown
Member

unit tests fail iconv(): Detected an illegal character in input string 🤔

@blizzz
typo + phpdoc
4f8c724
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@codecov

codecovBot commented Mar 5, 2018•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #8634 into master will decrease coverage by 20.49%.
The diff coverage is 0%.

@@ Coverage Diff @@## master #8634 +/- ##
============================================
- Coverage 51.87% 31.37% -20.5% - Complexity 25422 25424 +2 
============================================
Files 1609 1609 Lines 95343 95352 +9 Branches 1378 1378 ============================================
- Hits 49455 29914 -19541 - Misses 45888 65438 +19550
Impacted FilesCoverage ΔComplexity Δ
apps/user_ldap/lib/Access.php0% <0%> (-35.79%)323 <0> (+2)
...rivate/Authentication/Token/DefaultTokenMapper.php0% <0%> (-100%)11% <0%> (ø)
apps/user_ldap/lib/Migration/UUIDFix.php0% <0%> (-100%)5% <0%> (ø)
lib/public/Files/ForbiddenException.php0% <0%> (-100%)2% <0%> (ø)
apps/files_trashbin/lib/AppInfo/Application.php0% <0%> (-100%)2% <0%> (ø)
lib/private/DB/OCSqlitePlatform.php0% <0%> (-100%)5% <0%> (ø)
lib/private/Share20/Hooks.php0% <0%> (-100%)3% <0%> (ø)
lib/private/Files/Cache/Wrapper/JailPropagator.php0% <0%> (-100%)1% <0%> (ø)
...ivate/Files/Cache/Wrapper/CachePermissionsMask.php0% <0%> (-100%)3% <0%> (ø)
apps/user_ldap/lib/BackendUtility.php0% <0%> (-100%)1% <0%> (ø)
... and 374 more

@blizzz
treat iconv issues
47a10bd
Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz

blizzz commented Mar 5, 2018

Copy link
Copy Markdown
MemberAuthor

unit tests fail iconv(): Detected an illegal character in input string thinking

iconv :-/

»Caution

If and how //TRANSLIT works exactly depends on the system's iconv() implementation (cf. ICONV_IMPL). Some implementations are known to ignore //TRANSLIT, so the conversion is likely to fail for characters which are illegal for the out_charset.«
php doc

Yet there are no runtime or any config options available for PHP. So I changed the behaviour slightly: try to transliterate, be silent about issues, otherwise non-ascii characters are removed anyhow.

@rullzer
rullzer merged commit c2320ae into masterMar 5, 2018
@rullzer
rullzer deleted the ldap-no-empty-names branch March 5, 2018 18:37
This was referenced Mar 5, 2018
Merged
Merged
@blizzzblizzz mentioned this pull request Mar 7, 2018
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rullzerrullzerrullzer approved these changes
@MorrisJobkeMorrisJobkeMorrisJobke approved these changes
@GitHubUser4234GitHubUser4234Awaiting requested review from GitHubUser4234

Assignees

No one assigned

Labels

3. to reviewWaiting for reviewsbugfeature: ldap

Projects

None yet

Milestone

Nextcloud 14

Development

Successfully merging this pull request may close these issues.

3 participants

@blizzz@MorrisJobke@rullzer