Allow subadmins to read app config values #40961

jvillafanez · 2023-08-31T08:12:17Z

Description

Create a new AppConfigController aiming to deprecate the old "core/ajax/apppconfig.php" file. The functions in the "core/js/config.js" file will use the new controller.

Some additional notes:

Parameters for the GET requests ("getApps", "getKeys", "getValue" and "hasKey") will be sent as part of the url, so they're be visible to anyone
Parameters for PUT and DELETE requests ("setValue", "deleteKey" and "deleteApp") can be sent both in the url or in the body
For now, only the parameters of the "setValue" will be sent in the body

In addition, the "getApps", "getKeys" and "getValue" will be available to subadmins ("hasKey" will rely on the "getValue" method in the server, so it will also be available)

Related Issue

https://github.com/owncloud/enterprise/issues/5981

Motivation and Context

Some of the functionality wasn't working for subadmins because they couldn't read the app config.

How Has This Been Tested?

test environment:
test case 1:
test case 2:
...

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:

update-docs · 2023-08-31T08:12:21Z

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

steelcuts · 2023-08-31T09:07:43Z

Which apps are utilizing this functionality, or how can I obtain this information?

jvillafanez · 2023-08-31T09:08:09Z

@phil-davis the errors in the acceptance tests in https://drone.owncloud.com/owncloud/core/39004/136/18 are expected, aren't they? We need to adjust the tests. I mean, the code is fine but the tests aren't valid any longer with the new code.

jvillafanez · 2023-08-31T09:10:35Z

Which apps are utilizing this functionality, or how can I obtain this information?

Grepping by OC.AppConfig might be the easiest option. Any matching js file might need to be checked.

jnweiger · 2023-08-31T09:27:26Z

I don't want to include this in 10.13.1 -- branching the release branch now, before this gets merged.
(If we should decide differently, then redirect to release-10.13.1)

branched from master, - taking 5 small well understood easily testable fixes (for php8 forward compatibility) - but before #40961 gets merged to master.

phil-davis · 2023-08-31T10:13:17Z

@phil-davis the errors in the acceptance tests in https://drone.owncloud.com/owncloud/core/39004/136/18 are expected, aren't they? We need to adjust the tests. I mean, the code is fine but the tests aren't valid any longer with the new code.

Yes, if the requirements have changed (or are now understood differently) then we need to adjust the tests.
I can sort that out tomorrow, if that is OK. Or you can change it.

jvillafanez · 2023-08-31T10:46:30Z

I'll leave it up to you. Feel free to add your commits here.

phil-davis · 2023-09-04T13:33:29Z

acceptance tests have passed
SonarCloud is complaining - but probably there is not much that can be done for unit test coverage of the code here?

* prepare 10.13.1 branched from master, - taking 5 small well understood easily testable fixes (for php8 forward compatibility) - but before #40961 gets merged to master. * Improve images (#40963) * Revamped the onlyoffice.png in gimp: cleaner and smaller PNG, ugly checkerboard background removed. (It was actually a 200k jpeg) Replaced wopi.png with the logo found at github.com/OfficeDev/PnP-WOPI (it was a copy of the onlyoffic jpeg) * added transparent round corners to the new icons. drawio.png and richdocuments.png already have transparency. * fix: disallow pre-signed url access if the signing key is not initialized * bump Changelog * bump version.php for final * prepare merge back. * prepare merge back, using 10.13.2 prealpha ... --------- Co-authored-by: Juergen Weigert <jnweiger@gmail.com> Co-authored-by: Thomas Müller <1005065+DeepDiver1975@users.noreply.github.com>

voroyam · 2023-09-15T09:36:47Z

what is the status of this?

pako81 · 2023-09-19T15:44:08Z

@IljaN @steelcuts @mrow4a can you please review this?

IljaN

LGTM. Please add changelog.

jvillafanez · 2023-09-25T08:51:16Z

Changelog added

phil-davis · 2023-09-25T09:03:33Z

settings/Controller/AppConfigController.php

+	}
+
+	/**
+	 * @NoAdminRequired


Note: there is also a NoSubadminRequired annotation which allows a method to be used even if the user is not a subadmin.

So with just the NoAdminRequired annotation, this method can be used by admins and subadmins, which is what we want. Ordinary users, and "the public" won't be able to use the method.

pako81 · 2023-09-25T09:36:49Z

SonarCloud is complaining. CI is passing. Should we merge this?

phil-davis · 2023-09-25T10:11:25Z

https://github.com/owncloud/core/blob/master/settings/Controller/AppSettingsController.php has tests in https://github.com/owncloud/core/blob/master/tests/Settings/Controller/AppSettingsControllerTest.php

The new AppConfigController.php could have a similar AppConfigControllerTest.php to cover that.

jvillafanez · 2023-09-25T11:15:47Z

Unit tests added

phil-davis

LGTM if tests pass.

sonarcloud · 2023-09-25T11:41:30Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

60.4% Coverage
0.0% Duplication

pako81 · 2023-09-25T11:54:33Z

CI is green..merging.

jvillafanez added the 3 - To Review label Aug 31, 2023

jvillafanez self-assigned this Aug 31, 2023

Allow subadmins to read app config values

81e1044

jvillafanez force-pushed the fix_subadmin_appconfig branch from c56b006 to 81e1044 Compare August 31, 2023 08:19

jnweiger requested review from IljaN and steelcuts August 31, 2023 08:55

jnweiger added a commit that referenced this pull request Aug 31, 2023

prepare 10.13.1

039e855

branched from master, - taking 5 small well understood easily testable fixes (for php8 forward compatibility) - but before #40961 gets merged to master.

phil-davis added 2 commits September 4, 2023 16:43

Adjust test for Users page settings for subadmin

9d9654f

Adjust test step grammar

9f5ceda

pako81 requested a review from mrow4a September 19, 2023 15:43

IljaN approved these changes Sep 20, 2023

View reviewed changes

Add changelog entry

4f181ac

phil-davis approved these changes Sep 25, 2023

View reviewed changes

Add unit tests

940c0dc

phil-davis approved these changes Sep 25, 2023

View reviewed changes

pako81 merged commit 500c505 into master Sep 25, 2023
3 checks passed

delete-merged-branch bot deleted the fix_subadmin_appconfig branch September 25, 2023 11:54

nabim777 mentioned this pull request Sep 27, 2023

[tests-only][full-ci] skip tag add on recent release ocV10 versions #40998

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow subadmins to read app config values #40961

Allow subadmins to read app config values #40961

jvillafanez commented Aug 31, 2023

update-docs bot commented Aug 31, 2023

steelcuts commented Aug 31, 2023

jvillafanez commented Aug 31, 2023

jvillafanez commented Aug 31, 2023

jnweiger commented Aug 31, 2023 •

edited

phil-davis commented Aug 31, 2023

jvillafanez commented Aug 31, 2023

phil-davis commented Sep 4, 2023

voroyam commented Sep 15, 2023

pako81 commented Sep 19, 2023 •

edited

IljaN left a comment

jvillafanez commented Sep 25, 2023

phil-davis Sep 25, 2023

pako81 commented Sep 25, 2023

phil-davis commented Sep 25, 2023

jvillafanez commented Sep 25, 2023

phil-davis left a comment

sonarcloud bot commented Sep 25, 2023

pako81 commented Sep 25, 2023

Allow subadmins to read app config values #40961

Allow subadmins to read app config values #40961

Conversation

jvillafanez commented Aug 31, 2023

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

update-docs bot commented Aug 31, 2023

steelcuts commented Aug 31, 2023

jvillafanez commented Aug 31, 2023

jvillafanez commented Aug 31, 2023

jnweiger commented Aug 31, 2023 • edited

phil-davis commented Aug 31, 2023

jvillafanez commented Aug 31, 2023

phil-davis commented Sep 4, 2023

voroyam commented Sep 15, 2023

pako81 commented Sep 19, 2023 • edited

IljaN left a comment

Choose a reason for hiding this comment

jvillafanez commented Sep 25, 2023

phil-davis Sep 25, 2023

Choose a reason for hiding this comment

pako81 commented Sep 25, 2023

phil-davis commented Sep 25, 2023

jvillafanez commented Sep 25, 2023

phil-davis left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 25, 2023

pako81 commented Sep 25, 2023

jnweiger commented Aug 31, 2023 •

edited

pako81 commented Sep 19, 2023 •

edited