Prevent creation of secondary admin group #9527
Conversation
skjnldsv
added
bug
3. to review
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
skjnldsv
force-pushed
the
prevent-admin-group-creation
branch
from
0deb977 to
f4670e7
Compare
Codecov Report
@@ Coverage Diff @@
## master #9527 +/- ##
============================================
- Coverage 51.7% 51.69% -0.01%
- Complexity 25744 25747 +3
============================================
Files 1643 1643
Lines 96506 96510 +4
Branches 1393 1393
============================================
- Hits 49894 49892 -2
- Misses 46612 46618 +6
|
| @@ -225,6 +225,10 @@ public function addGroup(string $groupid): DataResponse { | |||
| $this->logger->error('Group name not supplied', ['app' => 'provisioning_api']); | |||
| throw new OCSException('Invalid group name', 101); | |||
| } | |||
| // Check if trying to create an admin group | |||
| if (trim(strtolower($groupid)) === 'admin') { | |||
There was a problem hiding this comment.
imho, if this scenario is not caught be the groupExists check in the next if-block, something is wrong with that check.
There was a problem hiding this comment.
@blizzz You're saying we sholdn't allow similar groups with lowercase differencies?
There was a problem hiding this comment.
@skjnldsv iirc – but it may depend on the backend – userids are already case insensitive when it comes to the exists check. perhaps it is just the local one with a different reason, but we should strive for some consistency. also, if possible, i prefer to avoid hard coded names.
There was a problem hiding this comment.
We should probably extend this to the global check!
There was a problem hiding this comment.
@blizzz user backend yes... group backend. nope (yay)
There was a problem hiding this comment.
@skjnldsv @rullzer how is it than an issue currently when we have case sensitive group names? as long as the new group is not exactly "admin" it should not have any effect?
We could make this insensitive in 14, however it can conflict when there are already installations with group ids in mixed cases. Keeping backwards compatibility is possible, but we'd never get rid of this extra annoying overhead.
There was a problem hiding this comment.
Let's add this then and not backport it?
There was a problem hiding this comment.
I canceled it. The new vue users list handle the group properly. There is no need for additional patch! :)
skjnldsv
added
2. developing
Fix #8010