Do not allow white space at the start or end of a group name#39540
Do not allow white space at the start or end of a group name#39540phil-davis wants to merge 3 commits intomasterfrom
Conversation
|
I have set this to "Draft" because we do not want to merge stuff to master right now. But I would appreciate a review of this. There is currently edge-case differences between oC10 (with MySQL/MariaDb) and oCIS, particularly for the case of groups "finance" and "finance " oC10 effectively treats those names as "the same thing". oCIS treats them as separate groups. (And I think oC10 with pgsql treats them as separate groups) This PR defines the behavior for this crud. So please review and give your feedback. If we agree with what I have done here, then I can cherry-pick this into #39514 (where I am accumulating stuff that is waiting to go in |
|
Kudos, SonarCloud Quality Gate passed! |
JammingBen
left a comment
JammingBen
left a comment
There was a problem hiding this comment.
LGTM, I think it's a good thing as well.
|
LGTM too. Let's make group name rules consistent with user name rules, where exavtly that is already forbidden (Correctly targeted at master. The 10.9.0 train has left.) |
|
Merged to master in PR #39612 |
4 participants
Description
Do not allow a group to be created with a name that has white space:
This is on top of PR #39530 which demonstrates the current behavior for groups with white space at the beginning or end of the group name.
The Provisioning API acts as if "finance" and "finance " are the same group, because of:
https://stackoverflow.com/questions/17876478/why-the-sql-server-ignore-the-empty-space-at-the-end-automatically
https://support.microsoft.com/en-us/topic/inf-how-sql-server-compares-strings-with-trailing-spaces-b62b1a2d-27d3-4260-216d-a605719003b0
If I create group "finance" first, then try to create group "finance ", the groupExists checks find that "finance " exists already, and refuses to create it.
If I create group "finance " first, then try to create group "finance", the groupExists checks find that "finance" exists already, and refuses to create it.
Item (1) seems a reasonable "accident" of the code - it prevents creating an extra group name with a space on the end.
Item (2) is a bit odd - if we accidentally create group "finance " then we are stuck with "finance " in the database. And when we do an API request like:
We get elements with that space at the end of the name like:
And to query the group directly we have to:
Also: https://jira.atlassian.com/browse/CWD-4290
That claims that pgsql will behave differently than mySQL/MariaDb, and likely happily add separate table rows for "finance" and "finance " groups, so they will be treated as different groups in an oC10 installation using pgsql.
So that is another reason to refuse to create groups (or any string data) with trailing spaces - different databases, and implementations not using an SQL database, might behave differently.
Therefore this PR prevents creation of a group with a name that ends in white space, including a group name that is only white space.
It seems silly to allow group names that start with white space, like " finance", that would be strangely inconsistent with banning white space at the end of the name. So this PR also prevents white space at the start of a group name.
As well as unit test cases, I have added API acceptance test scenarios, because we want oCIS to behave the same. IMO oCIS currently allows spaces at the beginning and end, and considers all combinations like " finance", "finance" and "finance " to be separate valid groups.
Related Issue
https://github.com/owncloud/enterprise/issues/4890
#39533 - sorts out the requirements/rules for that issue.
How Has This Been Tested?
CI
Types of changes
Checklist: