search

owncloud / ocis

:atom_symbol: ownCloud Infinite Scale Stack
https://doc.owncloud.com/ocis/next/
Apache License 2.0
1.41k stars 184 forks source link

Groups with space at the beginning or end of the name can be created with the Provisioning API #2876

Open phil-davis opened 2 years ago

phil-davis commented 2 years ago

Describe the bug

core PR https://github.com/owncloud/core/pull/39540 is improving/changing the group name validation in core. oCIS should behave the same - there should not be any space allowed at the beginning and/or end of a group name.

Steps to reproduce

Run the related new API tests:

apiProvisioningGroups-v1/addGroup.feature:158
apiProvisioningGroups-v1/addGroup.feature:166
apiProvisioningGroups-v1/addGroup.feature:174
apiProvisioningGroups-v2/addGroup.feature:154
apiProvisioningGroups-v2/addGroup.feature:162
apiProvisioningGroups-v2/addGroup.feature:170

Expected behavior

Groups with space at the start or end of the name should not be allowed to be created: " space-at-start" "space-at-end " " " Those should all be prohibited group names.

Actual behavior

Those names can be use when creating a group with the Provisioning API.

Setup

Current ocis master.

ScharfViktor commented 4 months ago

Groups with space at the start or end of the name should not be allowed to be created: " space-at-start" "space-at-end "

why? I tested it on ocis-6.0.0 using graph/v1.0/groups - it's possible to create with space at the beginning or end of the name

@rhafer should we forbid it or close issue?

phil-davis commented 4 months ago

why?

In oC10 this was discussed and it was decided that "accidents" like having a group called "finance" and another group called "finance " (with a space at the end) caused confusion, and are never intentional in real-life. ocis can, of course, decide if it wants such a limitation or not.

micbar commented 4 months ago

@rhafer What is your opinion?

rhafer commented 4 months ago

I agree that we should not allow trailing or leading spaces in group names. It's just confusing.

dragonchaser commented 3 months ago

Would it be valid to just trim the name when validating the input or should there be a hard error (like 400)?

phil-davis commented 3 months ago

Would it be valid to just trim the name when validating the input or should there be a hard error (like 400)?

I like the idea of auto-trimming. And then if the group already exists, an error about that will be returned. (e.g. "finance" already exists, someone tries to create "finance ", they get an error back saying "finance" already exists.

But for non-human clients - the actual code making API requests - if it sends a request to create group "HR ", gets a success response, then it will think that "HR " now exists. But actually "HR" was created. The client application is going to get confused.