Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show only assignable groups to the subadmin #39752

Merged
merged 4 commits into from
Feb 28, 2022

Conversation

jvillafanez
Copy link
Member

@jvillafanez jvillafanez commented Feb 4, 2022

Description

Subadmins will be shown their own assignable groups, not all the groups available

Related Issue

Reported in https://central.owncloud.org/t/10-9-1-group-admin-can-see-all-groups-even-the-ones-that-he-is-not-admin-of/36209/2

Fixes #39756

Motivation and Context

How Has This Been Tested?

  1. Create some users and groups
  2. Assign a user as subadmin of a couple of group
  3. Login with that user
  4. Go to the user page

The user can assign to the groups he's subadmin. Other groups won't appear.

Screenshots (if appropriate):

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

@jvillafanez jvillafanez force-pushed the subadmin_assignable_groups branch from 5df539e to a1bafab Compare February 4, 2022 13:52
@jvillafanez
Copy link
Member Author

Unless someone has a better idea, I think we'll have to skip the phan check the CI is complaining about

@phil-davis
Copy link
Contributor

Unless someone has a better idea, I think we'll have to skip the phan check the CI is complaining about

Yes, there are plenty of comments in the code already:

	/* @phan-suppress-next-line PhanUndeclaredMethod */

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

WFM manually testing. When logged in as a subadmin:

  • adding a new user and
  • editing an existing user
    the list of groups to choose from now only has the groups which are applicable to the subadmin.

The backend server correctly implements checks already anyway, so this was just a UI issue, not a real subadmin privilege issue.

There are already API test scenarios covering this, like:
"Scenario: subadmin should not be able to create a new user into other group"
"Scenario: a subadmin cannot add users to groups the subadmin is not responsible for"

Do we want to add UI tests that confirm that excess groups do not appear in the dropdown lists in the UI?

@owncloud owncloud deleted a comment from update-docs bot Feb 7, 2022
@jvillafanez
Copy link
Member Author

I'm not sure if this could be considered a security issue since the user might not need to know the full list of groups. If so, I think we should have tests for this.

@phil-davis
Copy link
Contributor

phil-davis commented Feb 7, 2022

I'm not sure if this could be considered a security issue since the user might not need to know the full list of groups. If so, I think we should have tests for this.

The current behavior is:

  Scenario: subadmin gets all the groups
    Given user "subadmin" has been created with default attributes and without skeleton files
    And group "brand-new-group" has been created
    And group "0" has been created
    And group "España" has been created
    And user "subadmin" has been made a subadmin of group "brand-new-group"
    When user "subadmin" gets all the groups using the provisioning API
    Then the OCS status code should be "100"
    And the HTTP status code should be "200"
    And the extra groups returned by the API should be
      | España          |
      | brand-new-group |
      | 0               |

  Scenario: normal user cannot get a list of all the groups
    Given user "Alice" has been created with default attributes and without skeleton files
    And group "brand-new-group" has been created
    And group "0" has been created
    And group "España" has been created
    When user "Alice" gets all the groups using the provisioning API
    Then the OCS status code should be "997"
    And the HTTP status code should be "401"
    And the API should not return any data
  1. When a subadmin requests a list of all groups, the full list is returned.
  2. When an ordinary user requests a list of all groups, a 401 is returned. Ordinary users can only do the "search matching" that required them to enter a few characters when sharing, and matching groups and users are returned. So an ordinary user has to try all the combinations like "aa", "ab", "ac",... in order to discover the users and groups.

It can be a separate issue and fix/change if this behavior is not what is really required.

@pmaier1 Is (1) a bug or a feature?
Maybe when a subadmin requests a list of "all" groups, then the list should be filtered, and only show the groups that they are subadmin of?

@sonarcloud
Copy link

sonarcloud bot commented Feb 7, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@pmaier1
Copy link
Contributor

pmaier1 commented Feb 15, 2022

@pmaier1 Is (1) a bug or a feature?

I regard it as a bug. Group admins should only get the groups they are an admin of. How is the behavior for an ownCloud admin? If I'm not mistaken, the Provisioning API is anyway only usable by (group-)admin users.

@phil-davis
Copy link
Contributor

phil-davis commented Feb 15, 2022

I regard it as a bug. Group admins should only get the groups they are an admin of. How is the behavior for an ownCloud admin? If I'm not mistaken, the Provisioning API is anyway only usable by (group-)admin users.

correct - ordinary users can "see" themselves in the users part of the Provisioning API, but nothing else. They can't do anything in groups part of the Provisioning API.

full admins can do everything.

sub-admins (group admins) can do limited stuff for the members of "their" group(s). So we need to just limit/filter the response to a sub-admin when they request a list of groups.

@jvillafanez
Copy link
Member Author

Linking with https://github.com/owncloud/enterprise/issues/5022.
It seems we have quite a bunch of things to clarify regarding the subadmin... Taking that into account, I'm not sure if we want to merge this first and solve all the API issues in a different PR.

@jvillafanez
Copy link
Member Author

Since we'll have to deal with the provisioning API, let's merge this and fix the provisioning API in a different PR.

@jvillafanez jvillafanez merged commit 6272671 into master Feb 28, 2022
@delete-merged-branch delete-merged-branch bot deleted the subadmin_assignable_groups branch February 28, 2022 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10.9.1: Group admin can see all groups, even the ones that he is not admin of
3 participants