github-mirror/framework
2
0
Fork 0
mirror of https://github.com/flarum/framework.git synced 2024-11-22 06:33:27 +08:00
Code Issues Actions Packages Projects Releases Wiki Activity

User edit permission tightening (#2620)

Browse Source 
- Split user edit permision into edit attributes, edit credentials, and edit groups
- Only Admins can edit Admin Credentials
- Only Admins can Promote/Demote to/from Admin
This commit is contained in:
Matt Kilgore 2021-03-01 15:52:29 -05:00 committed by GitHub
parent d0adb244da
commit 9627eb73f1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 779 additions and 103 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
js/src/admin/components/PermissionGrid.js
View File

@ -327,9 +327,29 @@ export default class PermissionGrid extends Component {
);
items.add(
'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',
},

2
js/src/common/models/User.js
View File

@ -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,

201
js/src/forum/components/EditUserModal.js
View File

@ -37,9 +37,10 @@ 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>
);
}
@ -47,96 +48,112 @@ 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}
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
);
}
</div>,
30
);
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) => (
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',
@ -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;
@ -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
js/src/forum/utils/UserControls.js
View File

@ -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)}>

5
locale/core.yml
View File

@ -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
@ -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

16
src/Api/Serializer/UserSerializer.php
View File

@ -19,14 +19,14 @@ class UserSerializer extends BasicUserSerializer
{
$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),
'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)) {
@ -35,7 +35,7 @@ class UserSerializer extends BasicUserSerializer
];
}
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

15
src/User/Access/UserPolicy.php
View File

@ -24,4 +24,19 @@ class UserPolicy extends AbstractPolicy
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();
}
}
}

21
src/User/Command/EditUserHandler.php
View File

@ -58,7 +58,6 @@ class EditUserHandler
$user = $this->users->findOrFail($command->userId, $actor);
$canEdit = $actor->can('edit', $user);
$isSelf = $actor->id === $user->id;
$attributes = Arr::get($data, 'attributes', []);
@ -66,7 +65,7 @@ class EditUserHandler
$validate = [];
if (isset($attributes['username'])) {
$actor->assertPermission($canEdit);
$actor->assertCan('editCredentials', $user);
$user->rename($attributes['username']);
}
@ -78,17 +77,18 @@ class EditUserHandler
$validate['email'] = $attributes['email'];
}
} else {
$actor->assertPermission($canEdit);
$actor->assertCan('editCredentials', $user);
$user->changeEmail($attributes['email']);
}
}
if ($actor->isAdmin() && ! empty($attributes['isEmailConfirmed'])) {
if (! empty($attributes['isEmailConfirmed'])) {
$actor->assertAdmin();
$user->activate();
}
if (isset($attributes['password'])) {
$actor->assertPermission($canEdit);
$actor->assertCan('editCredentials', $user);
$user->changePassword($attributes['password']);
$validate['password'] = $attributes['password'];
@ -108,7 +108,10 @@ class EditUserHandler
}
if (isset($relationships['groups']['data']) && is_array($relationships['groups']['data'])) {
$actor->assertPermission($canEdit);
$actor->assertCan('editGroups', $user);
$oldGroups = $user->groups()->get()->all();
$oldGroupIds = Arr::pluck($oldGroups, 'id');
$newGroupIds = [];
foreach ($relationships['groups']['data'] as $group) {
@ -117,8 +120,12 @@ class EditUserHandler
}
}
// Ensure non-admins aren't adding/removing admins
$adminChanged = in_array('1', array_diff($oldGroupIds, $newGroupIds)) || in_array('1', array_diff($newGroupIds, $oldGroupIds));
$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) {

598
tests/integration/api/users/UpdateTest.php
View File

@ -9,6 +9,7 @@
namespace Flarum\Tests\integration\api\users;
use Carbon\Carbon;
use Flarum\Tests\integration\RetrievesAuthorizedUsers;
use Flarum\Tests\integration\TestCase;
@ -26,7 +27,25 @@ class UpdateTest extends TestCase
$this->prepareDatabase([
'users' => [
$this->normalUser(),
]
[
'id' => 3,
'username' => 'normal2',
'password' => '$2y$10$LO59tiT7uggl6Oe23o/O6.utnF6ipngYjvMvaxo1TciKqBttDNKim', // BCrypt hash for "too-obscure"
'email' => 'normal2@machine.local',
'is_email_confirmed' => 1,
]
],
]);
}
protected function giveNormalUsersEditPerms()
{
$this->prepareDatabase([
'group_permission' => [
['permission' => 'user.edit', 'group_id' => 3],
['permission' => 'user.editCredentials', 'group_id' => 3],
['permission' => 'user.editGroups', 'group_id' => 3],
],
]);
}
@ -63,4 +82,581 @@ class UpdateTest extends TestCase
$this->assertEquals(200, $response->getStatusCode());
$this->assertStringNotContainsString('admin@machine.local', (string) $response->getBody());
}
/**
* @test
*
* This tests the generic user.edit permission used for non-credential/group attributes
*/
public function users_can_update_own_avatar()
{
$response = $this->send(
$this->request('DELETE', '/api/users/2/avatar', [
'authenticatedAs' => 2,
])
);
$this->assertEquals(200, $response->getStatusCode());
}
/**
* @test
*/
public function users_cant_update_own_email_if_password_wrong()
{
$response = $this->send(
$this->request('PATCH', '/api/users/2', [
'authenticatedAs' => 2,
'json' => [
'data' => [
'attributes' => [
'email' => 'someOtherEmail@example.com',
],
'meta' => [
'password' => 'notTheRightPassword!'
]
]
],
])
);
$this->assertEquals(401, $response->getStatusCode());
}
/**
* @test
*/
public function users_can_update_own_email()
{
$response = $this->send(
$this->request('PATCH', '/api/users/2', [
'authenticatedAs' => 2,
'json' => [
'data' => [
'attributes' => [
'email' => 'someOtherEmail@example.com',
]
],
'meta' => [
'password' => 'too-obscure'
]
],
])
);
$this->assertEquals(200, $response->getStatusCode());
}
/**
* @test
*/
public function users_cant_update_own_username()
{
$response = $this->send(
$this->request('PATCH', '/api/users/2', [
'authenticatedAs' => 2,
'json' => [
'data' => [
'attributes' => [
'username' => 'iCantChangeThis',
],
]
],
])
);
$this->assertEquals(403, $response->getStatusCode());
}
/**
* @test
*/
public function users_can_update_own_preferences()
{
$response = $this->send(
$this->request('PATCH', '/api/users/2', [
'authenticatedAs' => 2,
'json' => [
'data' => [
'attributes' => [
'preferences' => [
'something' => 'else'
]
],
]
],
])
);
$this->assertEquals(200, $response->getStatusCode());
}
/**
* @test
*/
public function users_cant_update_own_groups()
{
$response = $this->send(
$this->request('PATCH', '/api/users/2', [
'authenticatedAs' => 2,
'json' => [
'data' => [
'relationships' => [
'groups' => [
'data' => [
['id'=> 1, 'type' => 'group']
]
]
],
]
],
])
);
$this->assertEquals(403, $response->getStatusCode());
}
/**
* @test
*/
public function users_can_update_marked_all_as_read()
{
$response = $this->send(
$this->request('PATCH', '/api/users/2', [
'authenticatedAs' => 2,
'json' => [
'data' => [
'attributes' => [
'markedAllAsReadAt' => Carbon::now()
],
]
],
])
);
$this->assertEquals(200, $response->getStatusCode());
}
/**
* @test
*/
public function users_cant_activate_themselves()
{
$response = $this->send(
$this->request('PATCH', '/api/users/2', [
'authenticatedAs' => 2,
'json' => [
'data' => [
'attributes' => [
'isEmailConfirmed' => true
],
]
],
])
);
$this->assertEquals(403, $response->getStatusCode());
}
/**
* @test
*
* This tests the generic user.edit permission used for non-credential/group attributes
*/
public function users_cant_update_others_avatars_without_permission()
{
$response = $this->send(
$this->request('DELETE', '/api/users/2/avatar', [
'authenticatedAs' => 3,
])
);
$this->assertEquals(403, $response->getStatusCode());
}
/**
* @test
*/
public function users_cant_update_others_emails_without_permission()
{
$response = $this->send(
$this->request('PATCH', '/api/users/2', [
'authenticatedAs' => 3,
'json' => [
'data' => [
'attributes' => [
'email' => 'someOtherEmail@example.com',
]
],
'meta' => [
'password' => 'too-obscure'
]
],
])
);
$this->assertEquals(403, $response->getStatusCode());
}
/**
* @test
*/
public function users_cant_update_others_usernames_without_permission()
{
$response = $this->send(
$this->request('PATCH', '/api/users/2', [
'authenticatedAs' => 3,
'json' => [
'data' => [
'attributes' => [
'username' => 'iCantChangeThis',
],
]
],
])
);
$this->assertEquals(403, $response->getStatusCode());
}
/**
* @test
*/
public function users_cant_update_others_groups_without_permission()
{
$response = $this->send(
$this->request('PATCH', '/api/users/2', [
'authenticatedAs' => 3,
'json' => [
'data' => [
'relationships' => [
'groups' => [
'data' => [
['id'=> 1, 'type' => 'group']
]
]
],
]
],
])
);
$this->assertEquals(403, $response->getStatusCode());
}
/**
* @test
*/
public function users_cant_activate_others_without_permission()
{
$response = $this->send(
$this->request('PATCH', '/api/users/2', [
'authenticatedAs' => 2,
'json' => [
'data' => [
'attributes' => [
'isEmailConfirmed' => true
],
]
],
])
);
$this->assertEquals(403, $response->getStatusCode());
}
/**
* @test
*
* This tests the generic user.edit permission used for non-credential/group attributes
*/
public function users_can_update_others_avatars_with_permissions()
{
$this->giveNormalUsersEditPerms();
$response = $this->send(
$this->request('DELETE', '/api/users/2/avatar', [
'authenticatedAs' => 3,
])
);
$this->assertEquals(200, $response->getStatusCode());
}
/**
* @test
*/
public function users_can_update_others_emails_with_permission()
{
$this->giveNormalUsersEditPerms();
$response = $this->send(
$this->request('PATCH', '/api/users/2', [
'authenticatedAs' => 3,
'json' => [
'data' => [
'attributes' => [
'email' => 'someOtherEmail@example.com',
]
]
],
])
);
$this->assertEquals(200, $response->getStatusCode());
}
/**
* @test
*/
public function users_can_update_others_usernames_with_permission()
{
$this->giveNormalUsersEditPerms();
$response = $this->send(
$this->request('PATCH', '/api/users/2', [
'authenticatedAs' => 3,
'json' => [
'data' => [
'attributes' => [
'username' => 'iCanChangeThis',
],
]
],
])
);
$this->assertEquals(200, $response->getStatusCode());
}
/**
* @test
*/
public function users_cant_update_admin_emails_with_permission()
{
$this->giveNormalUsersEditPerms();
$response = $this->send(
$this->request('PATCH', '/api/users/1', [
'authenticatedAs' => 3,
'json' => [
'data' => [
'attributes' => [
'email' => 'someOtherEmail@example.com',
]
]
],
])
);
$this->assertEquals(403, $response->getStatusCode());
}
/**
* @test
*/
public function users_cant_update_admin_usernames_with_permission()
{
$this->giveNormalUsersEditPerms();
$response = $this->send(
$this->request('PATCH', '/api/users/1', [
'authenticatedAs' => 3,
'json' => [
'data' => [
'attributes' => [
'username' => 'iCanChangeThis',
],
]
],
])
);
$this->assertEquals(403, $response->getStatusCode());
}
/**
* @test
*/
public function users_can_update_others_groups_with_permission()
{
$this->giveNormalUsersEditPerms();
$response = $this->send(
$this->request('PATCH', '/api/users/2', [
'authenticatedAs' => 3,
'json' => [
'data' => [
'relationships' => [
'groups' => [
'data' => [
['id'=> 4, 'type' => 'group']
]
]
],
]
],
])
);
$this->assertEquals(200, $response->getStatusCode());
}
/**
* @test
*/
public function regular_users_cant_demote_admins_even_with_permission()
{
$this->giveNormalUsersEditPerms();
$response = $this->send(
$this->request('PATCH', '/api/users/1', [
'authenticatedAs' => 3,
'json' => [
'data' => [
'relationships' => [
'groups' => [
'data' => []
]
],
]
],
])
);
$this->assertEquals(403, $response->getStatusCode());
}
/**
* @test
*/
public function regular_users_cant_promote_others_to_admin_even_with_permission()
{
$this->giveNormalUsersEditPerms();
$response = $this->send(
$this->request('PATCH', '/api/users/2', [
'authenticatedAs' => 3,
'json' => [
'data' => [
'relationships' => [
'groups' => [
'data' => [
['id'=> 1, 'type' => 'group']
]
]
],
]
],
])
);
$this->assertEquals(403, $response->getStatusCode());
}
/**
* @test
*/
public function regular_users_cant_promote_self_to_admin_even_with_permission()
{
$this->giveNormalUsersEditPerms();
$response = $this->send(
$this->request('PATCH', '/api/users/3', [
'authenticatedAs' => 3,
'json' => [
'data' => [
'relationships' => [
'groups' => [
'data' => [
['id'=> 1, 'type' => 'group']
]
]
],
]
],
])
);
$this->assertEquals(403, $response->getStatusCode());
}
/**
* @test
*/
public function users_cant_activate_others_even_with_permissions()
{
$this->giveNormalUsersEditPerms();
$response = $this->send(
$this->request('PATCH', '/api/users/2', [
'authenticatedAs' => 2,
'json' => [
'data' => [
'attributes' => [
'isEmailConfirmed' => true
],
]
],
])
);
$this->assertEquals(403, $response->getStatusCode());
}
/**
* @test
*/
public function admins_cant_update_others_preferences()
{
$response = $this->send(
$this->request('PATCH', '/api/users/2', [
'authenticatedAs' => 1,
'json' => [
'data' => [
'attributes' => [
'preferences' => [
'something' => 'else'
]
],
]
],
])
);
$this->assertEquals(403, $response->getStatusCode());
}
/**
* @test
*/
public function admins_cant_update_marked_all_as_read()
{
$response = $this->send(
$this->request('PATCH', '/api/users/2', [
'authenticatedAs' => 1,
'json' => [
'data' => [
'attributes' => [
'markedAllAsReadAt' => Carbon::now()
],
]
],
])
);
$this->assertEquals(403, $response->getStatusCode());
}
/**
* @test
*/
public function admins_can_activate_others()
{
$response = $this->send(
$this->request('PATCH', '/api/users/2', [
'authenticatedAs' => 1,
'json' => [
'data' => [
'attributes' => [
'isEmailConfirmed' => true
],
]
],
])
);
$this->assertEquals(200, $response->getStatusCode());
}
/**
* @test
*/
public function admins_cant_demote_self()
{
$this->giveNormalUsersEditPerms();
$response = $this->send(
$this->request('PATCH', '/api/users/1', [
'authenticatedAs' => 1,
'json' => [
'data' => [
'relationships' => [
'groups' => [
'data' => [
]
]
],
]
],
])
);
$this->assertEquals(403, $response->getStatusCode());
}
}