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 8 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
18 changes: 17 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 @@ -326,10 +326,26 @@ export default class PermissionGrid extends Component {
60
);

items.add('userEditCredentials', {
icon: 'fas fa-user-cog',
label: app.translator.trans('core.admin.permissions.edit_users_credentials_label'),
permission: 'user.editCredentials',
});

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-user-cog',
icon: 'fas fa-address-card',
label: app.translator.trans('core.admin.permissions.edit_users_label'),
permission: 'user.edit',
},
Expand Down
8 changes: 7 additions & 1 deletion js/src/common/models/User.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,15 @@ Object.assign(User.prototype, {
discussionCount: Model.attribute('discussionCount'),
commentCount: Model.attribute('commentCount'),

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

canEdit: computed('canEditCredentials', 'canEditGroups', 'canEditAttributes', function (canEditAttributes, canEditCredentials, canEditGroups) {
return canEditAttributes || canEditCredentials || canEditGroups;
}),

avatarColor: null,
color: computed('username', 'avatarUrl', 'avatarColor', function (username, avatarUrl, avatarColor) {
// If we've already calculated and cached the dominant color of the user's
Expand Down
162 changes: 85 additions & 77 deletions js/src/forum/components/EditUserModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,96 +47,104 @@ export default class EditUserModal extends Modal {
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}
/>
</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() ? (
<input
className="FormControl"
type="password"
name="password"
placeholder={extractText(app.translator.trans('core.forum.edit_user.password_label'))}
bidi={this.password}
/>
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" 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>
) : (
''
)}
</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;
}}
/>
{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}
/>
) : (
''
)}
</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={this.attrs.user.id() === '1' && group.id() === Group.ADMINISTRATOR_ID}
/>
{GroupBadge.component({ group, label: '' })} {group.nameSingular()}
</label>
))}
</div>
</div>,
10
);
}

items.add(
'submit',
Expand Down
4 changes: 3 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 users groups
askvortsov1 marked this conversation as resolved.
Show resolved Hide resolved
global_heading: Global
moderate_heading: Moderate
new_group_button: New Group
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();
}
}
}
20 changes: 13 additions & 7 deletions src/User/Command/EditUserHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,14 @@ public function handle(EditUser $command)

$user = $this->users->findOrFail($command->userId, $actor);

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

$attributes = Arr::get($data, 'attributes', []);
$relationships = Arr::get($data, 'relationships', []);
$validate = [];

if (isset($attributes['username'])) {
$actor->assertPermission($canEdit);
$actor->assertPermission($actor->can('editCredentials', $user));
$user->rename($attributes['username']);
}

Expand All @@ -78,17 +77,18 @@ public function handle(EditUser $command)
$validate['email'] = $attributes['email'];
}
} else {
$actor->assertPermission($canEdit);
$actor->assertPermission($actor->can('editCredentials', $user));
$user->changeEmail($attributes['email']);
}
}

if ($actor->isAdmin() && ! empty($attributes['isEmailConfirmed'])) {
if (!empty($attributes['isEmailConfirmed'])) {
Copy link
Member

Choose a reason for hiding this comment

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

This change allows us to more cleanly test editing the user

$actor->assertAdmin();
$user->activate();
}

if (isset($attributes['password'])) {
$actor->assertPermission($canEdit);
$actor->assertPermission($actor->can('editCredentials', $user));
$user->changePassword($attributes['password']);
Copy link
Member

Choose a reason for hiding this comment

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

off topic observation: we force users to use the change password button (reset password email) on the frontend but not on the backend, so they could just send a payload and directly edit it.

I don't know if this was done on purpose.

Copy link
Member

@askvortsov1 askvortsov1 Mar 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't disagree, but this PR already contains a lot so I'll leave this alone for now

Copy link
Member

Choose a reason for hiding this comment

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

Yea, I wouldn't want that changed here anyway :P was an observation as it looked odd.


$validate['password'] = $attributes['password'];
Expand All @@ -108,7 +108,10 @@ public function handle(EditUser $command)
}

if (isset($relationships['groups']['data']) && is_array($relationships['groups']['data'])) {
$actor->assertPermission($canEdit);
$actor->assertPermission($actor->can('editGroups', $user));

$oldGroups = $user->groups()->get()->all();
$oldGroupIds = Arr::pluck($oldGroups, 'id');

$newGroupIds = [];
foreach ($relationships['groups']['data'] as $group) {
Expand All @@ -117,8 +120,11 @@ public function handle(EditUser $command)
}
}

$adminChanged = in_array('1', array_diff($oldGroupIds, $newGroupIds)) || in_array('1', array_diff($newGroupIds, $oldGroupIds));
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 moved out to a guard (like SelfDemotionGuard)? We decided to leave it here since that felt simpler. Alternatively, we could move SelfDemotionGuard here for simplicity?

Copy link
Member

Choose a reason for hiding this comment

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

I don't feel strongly about this, but I wouldn't remove the SelfDemotionGuard either, maybe just leave things as is ?

$actor->assertPermission(!$adminChanged || $actor->isAdmin());

$user->raise(
new GroupsChanged($user, $user->groups()->get()->all())
new GroupsChanged($user, $oldGroups)
);

$user->afterSave(function (User $user) use ($newGroupIds) {
Expand Down
Loading