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.

Sign up for GitHub

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

[FIX] UI was not disabling the actions when users has had no permissions to create channels or add users to rooms #10564

Merged
merged 18 commits into from
May 18, 2018
Merged

[FIX] UI was not disabling the actions when users has had no permissions to create channels or add users to rooms #10564

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
f592669
hide plus icon when user doesn't have both permission for create-c an…
cfunkles Apr 23, 2018
df6e91b
add helper to checkout two permissions set initial value for the room…
cfunkles Apr 23, 2018
badaefe
hide the plus icon in directory if user doesn't have both create-c an…
cfunkles Apr 23, 2018
bda88ac
get permissions for create channels and groups
cfunkles Apr 23, 2018
9e49bc9
check if user can add channel hide and groups, hide button based upon…
cfunkles Apr 23, 2018
dcdb454
prevent add user button from being hidden when user has permission ad…
cfunkles Apr 23, 2018
87999b1
Merge branch 'develop' into fix-permissions-bug-create-channels-add-u…
chuckAtCataworx Apr 25, 2018
f2232ae
Merge branch 'develop' into fix-permissions-bug-create-channels-add-u…
chuckAtCataworx Apr 25, 2018
7e5a44f
removed the if statement and use short hand if else syntax
cfunkles May 11, 2018
74a8d87
Merge branch 'develop' into fix-permissions-bug-create-channels-add-u…
chuckAtCataworx May 11, 2018
31e6e5b
Merge branch 'fix-permissions-bug-create-channels-add-users' of https…
cfunkles May 11, 2018
8853fb8
better code for disabling checkbox in create room feature if user doe…
cfunkles May 11, 2018
aee7a94
add missing simicolon
cfunkles May 11, 2018
975ef05
put canShowAddUsersButton into seperate function call function in eve…
cfunkles May 15, 2018
807b25e
move the canShowAddUsersButton function to define before it's called
cfunkles May 15, 2018
ae193fa
Merge branch 'develop' into fix-permissions-bug-create-channels-add-u…
chuckAtCataworx May 16, 2018
44cda3a
fix bug that prevents the viewing of the keyboard shortcuts button in…
cfunkles May 16, 2018
c9ad6ec
fix permissions
ggazzo May 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions packages/rocketchat-ui-flextab/client/flexTabBar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,15 +188,30 @@ Template.RoomsActionTab.helpers({
if (Template.instance().small.get()) {
return [];
}
let removedButton = false;
Copy link
Member

Choose a reason for hiding this comment

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

you dont need this variable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing this variable creates a potential issue. When the add users button isn't displayed, then files list gets duplicated. It stays in it's original more section and takes the place of the add users button

const canAddToChannel = RocketChat.authz.hasAllPermission('add-user-to-any-c-room');
const canAddToGroup = RocketChat.authz.hasAllPermission('add-user-to-any-p-room');
const canAddToJoinedRoom = RocketChat.authz.hasAllPermission('add-user-to-joined-room');
Copy link
Contributor

@gdelavald gdelavald Apr 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Does this happen on create-channel as well? Need to make sure the room is created before he is able to add-user-to-joined-room.

Copy link
Contributor Author

@chuckAtCataworx chuckAtCataworx Apr 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Yes. I think your comment on the createChannel.js file looks like you saw where that happens. I could take a look at how we use the add-user-to-joined-room permission. In this implementation the user needs the permissions 'add-user-to-any-c-room'('add-user-to-any-p-room' for groups) or 'add-user-to-joined-room' to view the 'add users' button. Perhaps it should only rely on the 'add-user-to-joined-room'? It is as simple as deleting the two const 'canAddToChannel' and 'canAddToGroup' then removing them from the if statement on line 202 and 206.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, checking the permissions page show that users do not have the add-user-to-any-p-room (same for c) by default, and only admins do. Also there could be cases where users don't have the add-user-to-joined-room permission (which would be weird, but could happen) and the former permission should overrule this one.
I'm doing some tests on the PR deployment to check for edge cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a slight mistake in my comment in describing the behavior, I edited the comment to reflect the actual behavior of what is coded. Which is what I think should correctly handle the expected behavior. The mistake was saying you need both permissions to view, but corrected to say one or the other.

const buttons = RocketChat.TabBar.getButtons().filter(button => {
if (!Meteor.userId() && !this.anonymous) {
return false;
}
if (button.groups.indexOf(Template.instance().tabBar.currentGroup()) === -1) {
return false;
}
if (!canAddToJoinedRoom && !canAddToChannel && Template.instance().tabBar.currentGroup() === 'channel' && button.id === 'addUsers') {
removedButton = true;
Copy link
Member

Choose a reason for hiding this comment

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

remove ;)

return false;
}
if (!canAddToJoinedRoom && !canAddToGroup && Template.instance().tabBar.currentGroup() === 'group' && button.id === 'addUsers') {
removedButton = true;
Copy link
Member

Choose a reason for hiding this comment

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

remove ;)

return false;
}
return true;
});
if (removedButton) {
Copy link
Member

Choose a reason for hiding this comment

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

remove ;)

return buttons.length <= 5 ? buttons : buttons.slice(0, 3);
}
return buttons.length <= 5 ? buttons : buttons.slice(0, 4);
},

Expand Down
10 changes: 6 additions & 4 deletions packages/rocketchat-ui/client/views/app/createChannel.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,12 @@ <h1 class="create-channel__title">{{_ "Create_A_New_Channel"}}</h1>
<div class="create-channel__switches">
<div class="rc-switch">
<label class="rc-switch__label" tabindex="-1">
<input type="checkbox" class="rc-switch__input" name="type" value="p" checked>
<span class="rc-switch__button">
<span class="rc-switch__button-inside"></span>
</span>
{{#if canCreateBothTypes}}
<input type="checkbox" class="rc-switch__input" name="type" value="p" checked>
Copy link
Member

Choose a reason for hiding this comment

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

@chuckAtCataworx could you change this code to

<input type="checkbox" class="rc-switch__input" name="type" value="p" checked="{{helperTestingIfTypeEqualtoP}}" disabled="{{cantCreateBothTypes}}"/>

Copy link
Member

Choose a reason for hiding this comment

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

instead of {{#if canCreateBothTypes}} use
<input type="checkbox" class="rc-switch__input" name="type" value="p" checked={{testIfTypeEqualP}} disabled="{{cantCreateBothTypes}}">

<span class="rc-switch__button">
<span class="rc-switch__button-inside"></span>
</span>
{{/if}}
<span class="rc-switch__text">
{{typeLabel}}
</span>
Expand Down
9 changes: 8 additions & 1 deletion packages/rocketchat-ui/client/views/app/createChannel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,9 @@ Template.createChannel.helpers({
readOnlyDescription() {
return t(Template.instance().readOnly.get() ? t('Only_authorized_users_can_write_new_messages') : t('All_users_in_the_channel_can_write_new_messages'));
},
canCreateBothTypes() {
return RocketChat.authz.hasAllPermission(['create-c', 'create-p']);
},
createIsDisabled() {
const instance = Template.instance();
const invalid = instance.invalid.get();
Expand Down Expand Up @@ -258,7 +261,11 @@ Template.createChannel.onCreated(function() {
this.extensions_validations = {};
this.extensions_submits = {};
this.name = new ReactiveVar('');
this.type = new ReactiveVar('p');
if (!RocketChat.authz.hasAllPermission(['create-p'])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Member

@ggazzo ggazzo May 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

use

this.type = new ReactiveVar(RocketChat.authz.hasAllPermission(['create-p']) ? 'p' : 'c')

this.type = new ReactiveVar('c');
} else {
this.type = new ReactiveVar('p');
}
this.readOnly = new ReactiveVar(false);
this.broadcast = new ReactiveVar(false);
this.inUse = new ReactiveVar(undefined);
Expand Down
4 changes: 3 additions & 1 deletion packages/rocketchat-ui/client/views/app/directory.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@
</div>
</label>
</div>
<button class="rc-button rc-button--small rc-button--primary rc-directory-plus">{{> icon icon="plus" }}</button>
{{#if createChannelOrGroup}}
<button class="rc-button rc-button--small rc-button--primary rc-directory-plus">{{> icon icon="plus" }}</button>
{{/if}}
</div>
{{/header}}
<div class="rc-directory-content">
Expand Down
3 changes: 3 additions & 0 deletions packages/rocketchat-ui/client/views/app/directory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ Template.directory.helpers({
} = Template.instance();

return key === searchSortBy.get() && sortDirection.get() !== 'asc' ? 'sort-up' : 'sort-down';
},
createChannelOrGroup() {
return RocketChat.authz.hasAtLeastOnePermission(['create-c', 'create-p']);
}
});

Expand Down