Skip to content

[stable9.1] Fix subadmin permission check for addToGroup/removeFromGroup#25797

Merged
PVince81 merged 1 commit into
stable9.1from
stable9.1-provisioning-api-subadmin-addtogroup
Aug 16, 2016
Merged

[stable9.1] Fix subadmin permission check for addToGroup/removeFromGroup#25797
PVince81 merged 1 commit into
stable9.1from
stable9.1-provisioning-api-subadmin-addtogroup

Conversation

@PVince81

Copy link
Copy Markdown
Contributor

Backport of #25788 to stable9.1

Please review @DeepDiver1975 @tomneedham @SergioBertolinSG @VicDeo @butonic

Fixes issue where subadmins could never add users to a group.
Added missing unit tests
@PVince81

Copy link
Copy Markdown
Contributor Author

@cdamken @dercorn

* @param IGroup|null $group group to check or null
*
* @return true if the user can manage the group
*/

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.

Not really needed due to this function is private, but setting $group as null by default would be nicer. Moreover, I find confusing why a group is needed: a comment in the docs would be nice.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the groupis needed for the isSubadminofGroup call below.

Basically this function is just a helper to check permissions with a shortcut in case the user is an admin to avoid additional checks.

@jvillafanez

Copy link
Copy Markdown
Member

👍

@PVince81 PVince81 merged commit c5477ad into stable9.1 Aug 16, 2016
@PVince81 PVince81 deleted the stable9.1-provisioning-api-subadmin-addtogroup branch August 16, 2016 10:01
@lock

lock Bot commented Aug 5, 2019

Copy link
Copy Markdown

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock Bot locked as resolved and limited conversation to collaborators Aug 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants