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

[NEW] Allows admin to list all groups with API #7565

Merged
merged 13 commits into from
Aug 24, 2017
Merged

[NEW] Allows admin to list all groups with API #7565

merged 13 commits into from
Aug 24, 2017

Conversation

mboudet
Copy link
Contributor

@mboudet mboudet commented Jul 25, 2017

@RocketChat/core

Closes #7408

This feature allows someone with "view-room-administration" permission to list all groups with the API call /api/v1/groups.list

Similar modifications should be done in the other groups API calls to ensure users with the proper permissions can do the same thing via API than with the web interface (such as delete & modify groups)

@CLAassistant
Copy link

CLAassistant commented Jul 25, 2017

CLA assistant check
All committers have signed the CLA.

@RocketChat RocketChat deleted a comment Jul 25, 2017
@RocketChat RocketChat deleted a comment Jul 25, 2017
@RocketChat RocketChat deleted a comment Jul 25, 2017
@RocketChat RocketChat deleted a comment Jul 25, 2017
@RocketChat RocketChat deleted a comment Jul 25, 2017
@RocketChat RocketChat deleted a comment Jul 25, 2017
@RocketChat RocketChat deleted a comment Jul 25, 2017
@cmonjeau
Copy link

It's a excellent great idea and I really need that!

Maybe changing the groups.info method also to allow the admin to see the complete infos of the group?

@rodrigok
Copy link
Member

Should we add a new method to list all groups? Like groups.listAll this one is more like groups.listMy and will change the behavior of the API if you have more power, this can break some existent integration

@graywolf336 what do you think?

@mboudet
Copy link
Contributor Author

mboudet commented Jul 26, 2017

It would probably be fine to add another method (though this one works if you consider that an admin should have access to all groups).

However, if we want to mirror the web UI capability for the admin, he should be able to delete, modify, and get the info of all groups.

So in any case, we will need to change the other methods to include an admin check, and bypass the "user is in group" check. (Or write another set of methods, admin only?)

@rodrigok
Copy link
Member

I agree we should improve the methods to allow admins to administrate the rooms via API, just the method to list should have a new one to not change the behavior, methods to get (a single record), change, and delete can be the same.

But we need the opinion from @graywolf336 here

Copy link
Contributor

@graywolf336 graywolf336 left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request, however yes I agree with @rodrigok in that it should be split out into it's own call so that it doesn't provide a breaking change in how this endpoint works.

let rooms = _.pluck(RocketChat.models.Subscriptions.findByTypeAndUserId('p', this.userId).fetch(), '_room');
let rooms;
if (RocketChat.authz.hasPermission(this.userId, 'view-room-administration')) {
rooms = _.pluck(RocketChat.models.Subscriptions.findByType('p').fetch(), '_room');
Copy link
Contributor

Choose a reason for hiding this comment

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

This will have duplicate rooms since the subscriptions are for each user and each room the user is part of. Use the Rooms model and find by the private type.

@mboudet
Copy link
Contributor Author

mboudet commented Jul 31, 2017

Alright, I will revert back and add a .listAll with limited access

@graywolf336
Copy link
Contributor

Sounds good, thanks 👍

@RocketChat RocketChat deleted a comment Aug 3, 2017
@RocketChat RocketChat deleted a comment Aug 3, 2017
@RocketChat RocketChat deleted a comment Aug 3, 2017
@RocketChat RocketChat deleted a comment Aug 3, 2017
@RocketChat RocketChat deleted a comment Aug 3, 2017
get() {
const { offset, count } = this.getPaginationItems();
const { sort, fields } = this.parseJsonQuery();
if (!RocketChat.authz.hasPermission(this.userId, 'view-room-administration')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for making the changes, any chance you can move this permission check to be above the other two items?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sense. Done.

@mboudet
Copy link
Contributor Author

mboudet commented Aug 9, 2017

To mirror the web UI functionalities, the admin should be able to use most of the API methods for groups without being in the groups.
Namely, the methods .archive, .unarchive, .info, and the group properties editing (rename, setDescription, setPurpose, setReadOnly, setTopic and setType) should include an admin check

Should I make a separate pull request for these changes?

@mboudet
Copy link
Contributor Author

mboudet commented Aug 24, 2017

Alright, should be done now.
@graywolf336

@graywolf336 graywolf336 merged commit c73d54e into RocketChat:develop Aug 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants