Uh oh!
There was an error while loading. Please reload this page.
feat(settings): let group admins add any existing account to a managed group - #53114
feat(settings): let group admins add any existing account to a managed group#53114cushdog wants to merge 2 commits into
Conversation
| ['root' => '/cloud', 'name' => 'Groups#deleteGroup', 'url' => '/groups/{groupId}', 'verb' => 'DELETE', 'requirements' => ['groupId' => '.+']], | ||
| // Users | ||
| ['root' => '/cloud', 'name' => 'Users#getUsers', 'url' => '/users', 'verb' => 'GET'], |
There was a problem hiding this comment.
Can you run composer run cs:fix locally?
There was a problem hiding this comment.
Hmpf the reformating is still there
Uh oh!
There was an error while loading. Please reload this page.
| * 200: Users returned | ||
| */ | ||
| #[NoAdminRequired] | ||
| public function searchAllUsers(string $search = '', ?int $limit = null, int $offset = 0): DataResponse { |
There was a problem hiding this comment.
Is this not exactly what getUsers does? It also has a search param
There was a problem hiding this comment.
Yeah there is no need for a new endpoint, the frontend should use the existing ones unless there’s a really good reason.
I think getUsersDetails is the one to use as displaynames are needed, I suppose this is what the rest of the frontend uses (I did not check).
There was a problem hiding this comment.
Sorry about this! Changed this and did a much simpler version of what I had before, let me know what you think now!
| </template> | ||
| <script> | ||
| import { translate as t } from '@nextcloud/l10n' |
There was a problem hiding this comment.
| import { translate as t } from '@nextcloud/l10n' | |
| import { t } from '@nextcloud/l10n' |
| </NcDialog> | ||
| </template> | ||
| <script> |
There was a problem hiding this comment.
I would prefer new files to be Typescript.
| <script> | |
| <script lang="ts"> |
(and then use defineComponent below)
| <NcIconSvgWrapper :path="mdiCog" /> | ||
| </template> | ||
| {{ t('settings', 'Account management settings') }} | ||
| {{ t("settings", "Account management settings") }} |
There was a problem hiding this comment.
please use npm run lint:fix locally. This contains a lot of fixable issues like usage of semicolon or double quotes.
sorbaugh
commented
May 26, 2025
4e16a51 to
66091a4Comparecushdog
commented
May 27, 2025
Note: just wanted to quickly apologize for the change in commit history, ran into a bit of git trouble that I was able to work out with a force commit! |
| $isAdmin = $this->groupManager->isAdmin($uid); | ||
| $isDelegatedAdmin = $this->groupManager->isDelegatedAdmin($uid); | ||
| if ($isAdmin || $isDelegatedAdmin) { | ||
| if ($isAdmin || $isDelegatedAdmin || $subAdminManager->isSubAdmin($user)) { |
There was a problem hiding this comment.
This allows group admins to see all users.
@sorbaugh What is the design decision on this? It will be enabled by default? It is a big change.
The linked issue talks about limiting to displayname and a few things, which is not done here, but not sure it would be enough. We might want to safe guard behind an opt-in config flag, at least for upgrades.
There was a problem hiding this comment.
User iteration is not in our threat model, thus not a problem.
What is a problem is that more information as uid and displayname might be available.
Then again this is not a problem, because if we accept this feature, then every group admin can assign users to their group gaining full admin access over them.
So this means a group admin then has the same permissions on user management as admins, as there are not restrictions anymore (they can just add a user to their group and then adjust the user as they like).
For this its good to have approval of @nickvergessen for security point of view.
There was a problem hiding this comment.
So, the API already allows some extend of it.
Using http requests I as able, as a group admin, to add some users to my groups.
I am not sure why we need to change the API 🤔
There was a problem hiding this comment.
We need to change the API so that group admins can list all users, to be able to chose which ones they want to add to their groups
There was a problem hiding this comment.
Is the consensus that I would need to change the API then? I couldn't see any way to go about listing all users without adding in some new endpoint that only group-admins could access, which is why my original commit included a change to the API routes.
There was a problem hiding this comment.
We need to change the API so that group admins can list all users, to be able to chose which ones they want to add to their group
Right, so only the listing API needs a change. Other than that it should be working already.
There was a problem hiding this comment.
My thoughts exactly, I'll revert back to the API-change approach I committed originally!
There was a problem hiding this comment.
@cushdog You reverted to the added-route version it seems.
I prefer to not add a new route but only fix the existing one, like you did in the version I commented.
Maybe wait for consensus before switching again, but I do not see the point of having 2 routes for listing users? Unless the subadmin one gives access to less things? But as stated by others, as subadmins can add users to their group to gain access to them, they are all user admins in a way.
There was a problem hiding this comment.
Thanks! My intention with the new route was for it to act as a "limited access" endpoint. I did explore alternatives that avoided adding a new route, but they would have required changing some fairly fundamental permission or design structures, which I assumed we’d want to avoid.
I realize this is a significant change, and I’ve been trying to stay aligned with the project’s design principles. That said, would it be possible to set up a quick chat (even async) with someone familiar with the architectural constraints here? It would really help to clarify what parts of the existing structure are flexible and what are more set in stone. I think that might be smoother than going back and forth through multiple commits and PR comments.
Totally understand if that's not feasible, just thought I'd ask!
EDIT: I gave this some further thought and compiled a list of implementation ideas that might meet the requirements while aligning better with project direction. Hopefully, this helps us move forward more efficiently (I apologize for all the back-and-forward, I should've asked for clarification earlier and that is on me, and I appreciate your patience tremendously!):
Expand
getUsersfor subadmins- Modify
apps/provisioning_api/lib/Controller/UsersController.phpingetUsers()to allow subadmins to useuserManager->search()when no group filter is provided. - This reuses the existing
/ocs/v2.php/cloud/usersendpoint while returning only user IDs (no new route).
- Modify
Expand
getUsersDetailswith limited data- Adjust
getUsersDetails()so subadmins may search globally. - Optionally filter the returned data to
{ id, displayname }when the caller is a subadmin, preserving privacy.
- Adjust
Add a query parameter to existing route
- Keep
/ocs/v2.php/cloud/usersbut introduce a parameter like?scope=allor?searchAll=true. - The controller checks this parameter and, if the requester is a subadmin, lists all users.
- Keep
Introduce a configuration flag
- Provide a config option (e.g.,
allow_group_admin_global_search) that admins can enable. - When enabled,
getUsers()(and possiblygetUsersDetails()) will return all users to subadmins.
- Provide a config option (e.g.,
Reuse the sharees search API
- Instead of modifying the provisioning API, call the existing
/ocs/v2.php/apps/files_sharing/api/v1/shareesendpoint from the frontend (used by the sharing dialog). - It already returns user IDs and display names without requiring admin rights.
- Instead of modifying the provisioning API, call the existing
Central user-search service
- Implement a new internal service class (e.g.,
UserSearchService) that encapsulates permission checks for admins, delegated admins, and subadmins. getUsers()andgetUsersDetails()call into this service.- Allows future adjustments without new routes.
- Implement a new internal service class (e.g.,
Keep a dedicated
/users/searchendpoint (current PR approach)- Provide
/ocs/v2.php/cloud/users/searchreturningid → displaynamefor all users. - Restrict access to admins, delegated admins, and subadmins.
- Frontend uses this endpoint when the current user is only a group admin.
- Provide
Leverage group endpoints with empty filter
- Update
GroupsController::getGroupUsersDetails()so that when called with an empty group ID by a subadmin, it performs a global search. - The frontend then queries this endpoint for user suggestions.
- Update
Modify
SubAdmin::isUserAccessible- Broaden the logic of
lib/private/SubAdmin.phpso subadmins are considered “accessible” to any user for search purposes. - After adding a user to their group, the usual permission checks (e.g., editing) continue to rely on actual group membership.
- Broaden the logic of
| #### DO NOT CHANGE ANYTHING ABOVE THIS LINE #### | ||
| ErrorDocument 403 /index.php/error/403 | ||
| ErrorDocument 404 /index.php/error/404 |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
cushdog
commented
May 29, 2025
Apologies, working right now on unstaging the huge amount of extra files that got added in for some reason |
c26dd3b to
64731feCompare64731fe to
1f8e6c6Comparecushdog
commented
May 29, 2025
Ok I think I figured this out, apologies I had to do quite a few |
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
aparcar
commented
Jan 7, 2026
Is there a simple way to test this within an AIO installation? Like can I edit the local files manually and it "just works"? Looking at the changes, this doesn't do any changes to any databases etc |
miaulalala
commented
Jan 19, 2026
Since you need to recompile the frontend it won't be that easy unfortuntately. If you're familiar with Docker you can use https://juliusknorr.github.io/nextcloud-docker-dev/basics/getting-started/ Add your fork as an origin and pull from the origin, then check out your branch, run |
joshtrichards
commented
Jul 31, 2026
I think we should avoid both changing the default behavior of the existing user-listing endpoints for subadmins and adding a generic Either of these seems reasonable to me:
The trade-off is mainly a smaller API surface versus a more self-describing route. I do not feel strongly about it, although I slightly lean toward extending Whichever route we choose:
For the product/privacy side, I think this should be behind an instance-level setting that is visible in the UI and off by default for both new and upgraded instances. That makes the expanded discovery behavior intentional rather than silently broadening what existing subadmins can see, while giving all instances the same default and avoiding upgrade-specific behavior. I would also expect the search to match both user IDs and display names, omit users already in the target group, and omit disabled users (assuming comparable user-selection flows also omit them; I did not check). |
Summary
This PR adds two related features:
Frontend
AddExistingUserDialog.vue) that lets group-admins search for existing users and assign them to the currently selected group.showAddExistingUserForm) persisted alongside the “New account” form flag to open/close the dialog.Backend
userId => displayNamefor all Nextcloud users matching the search string.Screenshots
TODO
AddExistingUserDialog.vue(rendering, search behavior, submit).UsersController::searchAllUsers()(success and 403 cases)./users/searchendpoint./users/search.Checklist
Note
I just wanted to quickly note that I didn't update the documentation yet, since I wasn't sure if that should only be updated after this pull-request is potentially accepted!