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

Update for #15534 - Refined gates on user bulk #15563

Merged
merged 5 commits into from
Sep 25, 2024

Conversation

snipe
Copy link
Owner

@snipe snipe commented Sep 25, 2024

Just a slight revision for #15534, where we use a looser gate for starting things off, and then refine the gates based on ones that might need more permissions.

Copy link

what-the-diff bot commented Sep 25, 2024

PR Summary

  • Changed Initial User Edit Request Authorization
    The initial user editing procedure previously required an 'update' authorization. This has now been downgraded to a 'view' authorization, allowing users to view information before an update is required.

  • Added Bulk Edit Authorization Check
    We have introduced additional security for bulk editing of users. Now, an 'update' authorization is mandatory before proceeding, bolstering credentials verification.

  • Introduced Bulk Delete Authorization Requirement
    In our efforts to enhance data safety, we have implemented an authorization check during bulk delete operations. Users are now required to possess a 'delete' authorization to perform this action.

  • Enforced Permissions for Merging Users
    To ensure the security and integrity of user data, we have added an 'update' authorization check when merging user accounts. This ensures that only approved personnel can merge user information.

Copy link
Collaborator

@marcusmoore marcusmoore left a comment

Choose a reason for hiding this comment

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

I like making this more detailed 👍🏾

One thing to note is that we only display the bulk actions dropdown if the user has the ability to delete users. Should we update that so it individually takes into account which actions should be shown?

@can('delete', \App\Models\User::class)

@snipe
Copy link
Owner Author

snipe commented Sep 25, 2024

One thing to note is that we only display the bulk actions dropdown if the user has the ability to delete users.

I don't think that's true?

<div id="userBulkEditToolbar">
{{ Form::open([
'method' => 'POST',
'route' => ['users/bulkedit'],
'class' => 'form-inline',
'id' => 'usersBulkForm']) }}
@if (request('status')!='deleted')
@can('delete', \App\Models\User::class)
<div id="users-toolbar">
<label for="bulk_actions" class="sr-only">{{ trans('general.bulk_actions') }}</label>
<select name="bulk_actions" class="form-control select2" style="min-width:300px;" aria-label="bulk_actions">
<option value="edit">{{ trans('general.bulk_edit') }}</option>
<option value="delete">{!! trans('general.bulk_checkin_delete') !!}</option>
<option value="merge">{!! trans('general.merge_users') !!}</option>
<option value="bulkpasswordreset">{{ trans('button.send_password_link') }}</option>
</select>
<button class="btn btn-primary" id="bulkUserEditButton" disabled>{{ trans('button.go') }}</button>
</div>
@endcan
@endif
{{ Form::close() }}
</div>

@snipe
Copy link
Owner Author

snipe commented Sep 25, 2024

@marcusmoore I just added some gates around the dropdown menu - take a peek

@snipe
Copy link
Owner Author

snipe commented Sep 25, 2024

@marcusmoore Derp - you were right about the delete gate - that's dumb. I removed that and only applied it where it matters.

I moved merged into the delete gate though, since technically anyone who can merge a user is deleting a different user(s).

Copy link
Collaborator

@marcusmoore marcusmoore left a comment

Choose a reason for hiding this comment

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

I dig it 😄

@snipe snipe merged commit ac64117 into develop Sep 25, 2024
8 checks passed
@snipe snipe deleted the refined_gates_on_user_bulk branch September 25, 2024 19:36
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.

2 participants