X Tutup
Skip to content

refactor: replace isAdmin implementation#41171

Merged
phil-davis merged 6 commits intomasterfrom
replace_isAdmin_implementation
Feb 1, 2024
Merged

refactor: replace isAdmin implementation#41171
phil-davis merged 6 commits intomasterfrom
replace_isAdmin_implementation

Conversation

@pako81
Copy link

@pako81 pako81 commented Jan 31, 2024 •
edited
Loading

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

phil-davis commented Feb 1, 2024

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

phil-davis commented Feb 1, 2024

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.

jvillafanez
jvillafanez reviewed Feb 1, 2024
View reviewed changes
jvillafanez
jvillafanez reviewed Feb 1, 2024
View reviewed changes
jvillafanez
jvillafanez approved these changes Feb 1, 2024
View reviewed changes
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.

phil-davis
phil-davis approved these changes Feb 1, 2024
View reviewed changes
@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 1, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@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

Reviewers

@jvillafanez jvillafanez jvillafanez approved these changes

@phil-davis phil-davis phil-davis approved these changes

@DeepDiver1975 DeepDiver1975 Awaiting requested review from DeepDiver1975

+1 more reviewer

@IljaN IljaN IljaN approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@pako81 pako81

Labels

3 - To Review

Projects

None yet

Milestone

development

Development

Successfully merging this pull request may close these issues.

4 participants

@pako81 @phil-davis @IljaN @jvillafanez
X Tutup