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

User edit permission split #2620

Merged
merged 16 commits into from
Mar 1, 2021
Merged

User edit permission split #2620

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
c93436b
Initial user permission split
tankerkiller125 Feb 21, 2021
423ee74
Fix handler
tankerkiller125 Feb 21, 2021
b86fd47
More fixes
tankerkiller125 Feb 21, 2021
9dbedc4
Minor fixes
tankerkiller125 Feb 21, 2021
149c6f3
Another minor fix
tankerkiller125 Feb 21, 2021
a8eb8ed
Add Test
tankerkiller125 Feb 26, 2021
15976de
Only Admins can edit Admins Credentials
tankerkiller125 Feb 26, 2021
e803a70
Only Admins can Promote/Demote to/from Admin
tankerkiller125 Feb 26, 2021
9af11c6
Apply fixes from StyleCI
luceos Feb 26, 2021
9e39119
Add additional test
tankerkiller125 Feb 26, 2021
3c211a8
Group edit user items together
askvortsov1 Mar 1, 2021
3ee005c
Fix permission title
askvortsov1 Mar 1, 2021
c6ff98b
Use consistent naming for permissions
askvortsov1 Mar 1, 2021
34e112a
Enforce permissions in edit user modal UX
askvortsov1 Mar 1, 2021
9e502c8
Show message if no fields available to edit.
askvortsov1 Mar 1, 2021
2ff95b6
Minor cleanup
askvortsov1 Mar 1, 2021
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
22 changes: 21 additions & 1 deletion js/src/admin/components/PermissionGrid.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,9 +327,29 @@ export default class PermissionGrid extends Component {
);

items.add(
tankerkiller125 marked this conversation as resolved.
Show resolved Hide resolved
'userEdit',
'userEditCredentials',
{
icon: 'fas fa-user-cog',
label: app.translator.trans('core.admin.permissions.edit_users_credentials_label'),
permission: 'user.editCredentials',
},
60
);

items.add(
'userEditGroups',
{
icon: 'fas fa-users-cog',
label: app.translator.trans('core.admin.permissions.edit_users_groups_label'),
permission: 'user.editGroups',
},
60
);

items.add(
'userEdit',
{
icon: 'fas fa-address-card',
label: app.translator.trans('core.admin.permissions.edit_users_label'),
permission: 'user.edit',
},
Expand Down
2 changes: 2 additions & 0 deletions js/src/common/models/User.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ Object.assign(User.prototype, {
commentCount: Model.attribute('commentCount'),

canEdit: Model.attribute('canEdit'),
canEditCredentials: Model.attribute('canEditCredentials'),
canEditGroups: Model.attribute('canEditGroups'),
canDelete: Model.attribute('canDelete'),

avatarColor: null,
Expand Down
203 changes: 118 additions & 85 deletions js/src/forum/components/EditUserModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,106 +37,123 @@ export default class EditUserModal extends Modal {
}

content() {
const fields = this.fields().toArray();
return (
<div className="Modal-body">
<div className="Form">{this.fields().toArray()}</div>
{fields.length > 1 ? <div className="Form">{this.fields().toArray()}</div> : app.translator.trans('core.forum.edit_user.nothing_available')}
</div>
);
}

fields() {
const items = new ItemList();

items.add(
'username',
<div className="Form-group">
<label>{app.translator.trans('core.forum.edit_user.username_heading')}</label>
<input className="FormControl" placeholder={extractText(app.translator.trans('core.forum.edit_user.username_label'))} bidi={this.username} />
</div>,
40
);

if (app.session.user !== this.attrs.user) {
if (app.session.user.canEditCredentials()) {
items.add(
'email',
'username',
<div className="Form-group">
<label>{app.translator.trans('core.forum.edit_user.email_heading')}</label>
<div>
<input className="FormControl" placeholder={extractText(app.translator.trans('core.forum.edit_user.email_label'))} bidi={this.email} />
</div>
{!this.isEmailConfirmed() ? (
<div>
{Button.component(
{
className: 'Button Button--block',
loading: this.loading,
onclick: this.activate.bind(this),
},
app.translator.trans('core.forum.edit_user.activate_button')
)}
</div>
) : (
''
)}
<label>{app.translator.trans('core.forum.edit_user.username_heading')}</label>
<input
className="FormControl"
placeholder={extractText(app.translator.trans('core.forum.edit_user.username_label'))}
bidi={this.username}
disabled={this.nonAdminEditingAdmin()}
/>
</div>,
30
40
);

items.add(
'password',
<div className="Form-group">
<label>{app.translator.trans('core.forum.edit_user.password_heading')}</label>
<div>
<label className="checkbox">
<input
type="checkbox"
onchange={(e) => {
this.setPassword(e.target.checked);
m.redraw.sync();
if (e.target.checked) this.$('[name=password]').select();
e.redraw = false;
}}
/>
{app.translator.trans('core.forum.edit_user.set_password_label')}
</label>
{this.setPassword() ? (
if (app.session.user !== this.attrs.user) {
items.add(
'email',
<div className="Form-group">
<label>{app.translator.trans('core.forum.edit_user.email_heading')}</label>
<div>
<input
className="FormControl"
type="password"
name="password"
placeholder={extractText(app.translator.trans('core.forum.edit_user.password_label'))}
bidi={this.password}
placeholder={extractText(app.translator.trans('core.forum.edit_user.email_label'))}
bidi={this.email}
disabled={this.nonAdminEditingAdmin()}
/>
</div>
{!this.isEmailConfirmed() && this.userIsAdmin(app.session.user) ? (
<div>
{Button.component(
{
className: 'Button Button--block',
loading: this.loading,
onclick: this.activate.bind(this),
},
app.translator.trans('core.forum.edit_user.activate_button')
)}
</div>
) : (
''
)}
</div>
</div>,
20
);
}

items.add(
'groups',
<div className="Form-group EditUserModal-groups">
<label>{app.translator.trans('core.forum.edit_user.groups_heading')}</label>
<div>
{Object.keys(this.groups)
.map((id) => app.store.getById('groups', id))
.map((group) => (
</div>,
30
);

items.add(
'password',
<div className="Form-group">
<label>{app.translator.trans('core.forum.edit_user.password_heading')}</label>
<div>
<label className="checkbox">
<input
type="checkbox"
bidi={this.groups[group.id()]}
disabled={this.attrs.user.id() === '1' && group.id() === Group.ADMINISTRATOR_ID}
onchange={(e) => {
this.setPassword(e.target.checked);
m.redraw.sync();
if (e.target.checked) this.$('[name=password]').select();
e.redraw = false;
}}
disabled={this.nonAdminEditingAdmin()}
/>
{GroupBadge.component({ group, label: '' })} {group.nameSingular()}
{app.translator.trans('core.forum.edit_user.set_password_label')}
</label>
))}
</div>
</div>,
10
);
{this.setPassword() ? (
<input
className="FormControl"
type="password"
name="password"
placeholder={extractText(app.translator.trans('core.forum.edit_user.password_label'))}
bidi={this.password}
disabled={this.nonAdminEditingAdmin()}
/>
) : (
''
)}
</div>
</div>,
20
);
}
}

if (app.session.user.canEditGroups()) {
items.add(
'groups',
<div className="Form-group EditUserModal-groups">
<label>{app.translator.trans('core.forum.edit_user.groups_heading')}</label>
<div>
{Object.keys(this.groups)
.map((id) => app.store.getById('groups', id))
.map((group) => (
<label className="checkbox">
<input
type="checkbox"
bidi={this.groups[group.id()]}
disabled={group.id() === Group.ADMINISTRATOR_ID && (this.attrs.user === app.session.user || !this.userIsAdmin(app.session.user))}
/>
{GroupBadge.component({ group, label: '' })} {group.nameSingular()}
</label>
))}
</div>
</div>,
10
);
}

items.add(
'submit',
Expand Down Expand Up @@ -176,21 +193,26 @@ export default class EditUserModal extends Modal {
}

data() {
const groups = Object.keys(this.groups)
.filter((id) => this.groups[id]())
.map((id) => app.store.getById('groups', id));

const data = {
username: this.username(),
relationships: { groups },
relationships: {},
};

if (app.session.user !== this.attrs.user) {
data.email = this.email();
if (this.attrs.user.canEditCredentials() && !this.nonAdminEditingAdmin()) {
data.username = this.username();

if (app.session.user !== this.attrs.user) {
data.email = this.email();
}

if (this.setPassword()) {
data.password = this.password();
}
}

if (this.setPassword()) {
data.password = this.password();
if (this.attrs.user.canEditGroups()) {
data.relationships.groups = Object.keys(this.groups)
.filter((id) => this.groups[id]())
.map((id) => app.store.getById('groups', id));
}

return data;
Expand All @@ -209,4 +231,15 @@ export default class EditUserModal extends Modal {
m.redraw();
});
}

nonAdminEditingAdmin() {
return this.userIsAdmin(this.attrs.user) && !this.userIsAdmin(app.session.user);
}

/**
* @internal @protected
*/
userIsAdmin(user) {
return user.groups().some((g) => g.id() === Group.ADMINISTRATOR_ID);
}
}
2 changes: 1 addition & 1 deletion js/src/forum/utils/UserControls.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export default {
moderationControls(user) {
const items = new ItemList();

if (user.canEdit()) {
if (user.canEdit() || user.canEditCredentials() || user.canEditGroups()) {
items.add(
'edit',
<Button icon="fas fa-pencil-alt" onclick={this.editAction.bind(this, user)}>
Expand Down
5 changes: 4 additions & 1 deletion locale/core.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,9 @@ core:
delete_posts_label: Delete posts
description: Configure who can see and do what.
edit_posts_label: Edit posts
edit_users_label: Edit users
edit_users_label: Edit user attributes
edit_users_credentials_label: Edit user credentials
edit_users_groups_label: Edit user groups
global_heading: Global
moderate_heading: Moderate
new_group_button: New Group
Expand Down Expand Up @@ -291,6 +293,7 @@ core:
email_heading: => core.ref.email
email_label: => core.ref.email
groups_heading: Groups
nothing_available: There is nothing available for you to edit at this time.
password_heading: => core.ref.password
password_label: => core.ref.password
set_password_label: Set new password
Expand Down
16 changes: 8 additions & 8 deletions src/Api/Serializer/UserSerializer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,14 @@ protected function getDefaultAttributes($user)
{
$attributes = parent::getDefaultAttributes($user);

$canEdit = $this->actor->can('edit', $user);

$attributes += [
'joinTime' => $this->formatDate($user->joined_at),
'discussionCount' => (int) $user->discussion_count,
'commentCount' => (int) $user->comment_count,
'canEdit' => $canEdit,
'canDelete' => $this->actor->can('delete', $user),
'joinTime' => $this->formatDate($user->joined_at),
'discussionCount' => (int) $user->discussion_count,
'commentCount' => (int) $user->comment_count,
'canEdit' => $this->actor->can('edit', $user),
Copy link
Member

Choose a reason for hiding this comment

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

Should this be serialized as canEditAttributes? I think that might be more consistent.

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 think I didn't do that to prevent breaking extensions that might use canEdit.

Copy link
Member

Choose a reason for hiding this comment

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

We're not losing the method on the frontend user model, so we'd be preserving this for API consumers. But the meaning of the value has changed, so I'm not sure it's worth it to preserve; renaming would force consumers to adapt to the new system.

Copy link
Member

Choose a reason for hiding this comment

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

we should rename this, it's quiet confusing right now what the difference between canEdit and canEditAttributes is, on the frontend canEdit is true if the user has the ability to edit any of the three new permissions, on the backend (serialization) it just refers to the canEditAttributes ability.

Are we gonna keep canEdit the way it is on the frontend ? (ie any of the three abilities), I'm assuming we're deprecating it, if so, can we comment on the frontend model that it's deprecated please ?

'canEditCredentials' => $this->actor->can('editCredentials', $user),
'canEditGroups' => $this->actor->can('editGroups', $user),
'canDelete' => $this->actor->can('delete', $user),
];

if ($user->getPreference('discloseOnline') || $this->actor->can('viewLastSeenAt', $user)) {
Expand All @@ -35,7 +35,7 @@ protected function getDefaultAttributes($user)
];
}

if ($canEdit || $this->actor->id === $user->id) {
if ($attributes['canEditCredentials'] || $this->actor->id === $user->id) {
$attributes += [
'isEmailConfirmed' => (bool) $user->is_email_confirmed,
'email' => $user->email
Expand Down
15 changes: 15 additions & 0 deletions src/User/Access/UserPolicy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,19 @@ public function can(User $actor, $ability)
return $this->allow();
}
}

/**
* @param User $actor
* @param User $user
*/
public function editCredentials(User $actor, User $user)
{
if ($user->isAdmin() && ! $actor->isAdmin()) {
return $this->deny();
}

if ($actor->hasPermission('user.editCredentials')) {
return $this->allow();
}
}
}
Loading