Skip to content

Conversation

@pako81
Copy link

@pako81 pako81 commented Jan 31, 2024

Description

Replace the isAdmin implementation

Related Issue

Motivation and Context

Currently, the isAdmin method is indirectly calling the getUserIdGroups which - in case an LDAP connection is configured - would require to hit the LDAP server. For this particular check, getting the admin group and listing the members should be preferable and would not have the disadvantages related to an unstable LDAP connection (it was i.e. observed that under certain circumstances the metrics app got disabled in case the LDAP server could not be reached)

How Has This Been Tested?

Issue with the metrics app was not reproducible when intentionally causing an LDAP BindFailedException

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

@pako81 pako81 added this to the development milestone Jan 31, 2024
@pako81 pako81 self-assigned this Jan 31, 2024
@pako81 pako81 changed the title refactor: replace isAdmin implementation refactor: replace isAdmin implementation Jan 31, 2024
@phil-davis
Copy link
Contributor

Various unit tests failed.
Acceptance tests were failing with output like:

  Scenario: When in a group that is excluded from sharing, can_share is off                                        # /drone/src/tests/acceptance/features/apiCapabilities/capabilities.feature:5509
    Given parameter "shareapi_exclude_groups" of app "core" has been set to "yes"                                  # AppConfigurationContext::serverParameterHasBeenSetTo()
    And user "Alice" has been created with default attributes and without skeleton files                           # FeatureContext::userHasBeenCreatedWithDefaultAttributesAndWithoutSkeletonFiles()
      Unexpected failure when editing a user: HTTP status 401 HTTP reason Unauthorized (Exception)

$adminGroup->inGroup needs to be passed a user object, not a uid string. I adjusted the code and some unit tests.
There might still be a unit test failing in Test\User\UserTest::testCanChangeMailAddress
Let's see what CI thinks.

@owncloud owncloud deleted a comment from update-docs bot Feb 1, 2024
@phil-davis
Copy link
Contributor

Acceptance tests are all passing now, so external behavior is the same as before - good.
https://drone.owncloud.com/owncloud/core/39694/9/7

There was 1 error:
1) Test\User\UserTest::testCanChangeMailAddress with data set #1 (false, false)
Error: Call to a member function getUID() on null
/drone/src/lib/private/Group/Group.php:134
/drone/src/lib/private/Group/Manager.php:350
/drone/src/lib/private/User/User.php:482
/drone/src/tests/lib/User/UserTest.php:402
phpvfscomposer:///drone/src/lib/composer/phpunit/phpunit/phpunit:106

The unit test just needs some adjustment - I will have a look.

Copy link
Member

@jvillafanez jvillafanez left a comment

Choose a reason for hiding this comment

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

Tiny optional change.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 1, 2024

@phil-davis phil-davis merged commit 81bb8d0 into master Feb 1, 2024
@delete-merged-branch delete-merged-branch bot deleted the replace_isAdmin_implementation branch February 1, 2024 11:34
@pako81
Copy link
Author

pako81 commented Feb 1, 2024

thanks for your help! @phil-davis @jvillafanez

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.

5 participants