Do not allow various special usernames (UIDs)

[phil-davis]

Description

If UIDs are created with these "special" values then "bad things happen" because there are folders in the data directory, or end-points that become ambiguous or...

avatars
files_external
files_encryption
meta

Prevent them from being created as local users.

(will also need some action for user_ldap to sort out what to do if a UID like this exists in LDAP)

Related Issue

#30438 and #32547 and #37267

How Has This Been Tested?

CI and manually try to create these users in the webUI

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, see TEMPLATE

[phil-davis]

Comments please from anybody.
I am proposing that, for the time being, we disallow these special usernames. (Some day we could re-engineer the universe so that this unfortunate stuff does not happen)

Are there others that should be added to the list?

[micbar]

good improvement.
I tested this in the past, was surprised that thes names were allowed.

[codecov]

Codecov Report

Merging #37268 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master   #37268   +/-   ##
=========================================
  Coverage     64.55%   64.55%           
- Complexity    19207    19208    +1     
=========================================
  Files          1267     1267           
  Lines         75065    75068    +3     
  Branches       1331     1331           
=========================================
+ Hits          48458    48461    +3     
  Misses        26215    26215           
  Partials        392      392           

Flag	Coverage Δ	Complexity Δ
#javascript	54.14% <ø> (ø)	0.00 <ø> (ø)
#phpunit	65.71% <100.00%> (+<0.01%)	19208.00 <0.00> (+1.00)

Impacted Files	Coverage Δ	Complexity Δ
lib/private/User/Manager.php	86.41% <100.00%> (+0.25%)	58.00 <0.00> (+1.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 e38bb87...4aa37a8. Read the comment docs.

[cs35-owncloud]

Hey,

how about updater-data I think it's a directory created by the updater.

[phil-davis]

Hey,

how about updater-data I think it's a directory created by the updater.

Anyone know about this? I didn't find the string mentioned in the core repo.

Can anyone think of other special strings that should be prohibited?

[cs35-owncloud]

It's related to this doc

Creates an updater_backup directory under your ownCloud data directory

I don't know why I found updater-data in my case on some ownClouds, but updater_backup seems official, I guess.

[PVince81]

@VicDeo might know about the updater

[jvillafanez]

Is it worthy to provide some kind of "UserUidValidator" to be injected there, instead of using a fixed list of names? We could use a chain of implementations, such as a hardcoded list or a list configurable via config.php file.
Basically, we inject a "ValidatorChain", which would contain one or more validators. If the username passes through all the validators, the name is valid and could be used.

This could be more flexible in case we miss a name, or if the admin wants to forbid a particular name for whatever reason.

[micbar]

@jvillafanez Yes, please go in that direction.

[phil-davis]

@jvillafanez please take over this PR. (Or make a new one)
I raised this initial code just to get things moving.

[VicDeo]

@phil-davis

Anyone know about this? I didn't find the string mentioned in the core repo.

Updater is located in a separate repo and the path to it's data is updater-data indeed
https://github.com/owncloud/updater/blob/master/src/Utils/Locator.php#L183

[jvillafanez]

@micbar (and anyone who wants to say anything), I think we need to decide what happen to the accounts that have been created before making the uid invalid.
Use case could be similar to:

1. Sync one backend
2. Notice that one of the users has a potentially dangerous uid
3. Invalidate that uid

For now, the code doesn't really do anything to that account, but the user won't be able to login (there is a crash I have to check, caused by the invalid uid crash fixed, now it shows a failed login).
We might consider to disable the account, although the deletion would need to be performed by the admin at some point (better let the admin to take the decision).
I haven't checked in depth, but I guess all the interactions with the "dangerous" account are still possible.

The current code won't sync accounts (looking particularly at LDAP, but applicable to any backend) with potentially dangerous uids.

[jvillafanez]

Reverted the changes to the original ones due to overengineering

[phil-davis]

@micbar @DeepDiver1975 @PVince81 @jvillafanez @VicDeo or anyone. Please review and advise if this is OK as-is or if there is more to do or another approach needed.

[VicDeo]

In general I'm fine with this approach.
As for me, making $invalidUids a protected class member is preferable.

[jvillafanez]

assuming we aren't missing any name