Fixes #29445 - restrict STI deletion to admin #7694
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When obsolete STI records deletion was introduced, we allowed even non-admin users to delete that. This patch makes it possible only for admins to delete records from the database. Non-admin users only see a short message explaining what happened, that they should contact administrator and request ID that can help administrator to figure out what happened. It was also necessary to add explicit require_login call, otherwise User.current was nil in case the stack failed before user was loaded. This also made the page looks much nicer, becase the menu can be properly rendered.
|
Issues: #29445 |
tbrisker
previously approved these changes
May 25, 2020
tbrisker
reviewed
May 27, 2020
| <br><br> | ||
| <%= link_to _('Delete'), '?confirm_data_deletion=yes', class: 'btn btn-danger', data: { confirm: _('The data deletion can not be undone, are you sure you want to proceed?') } %> | ||
| <% else %> | ||
| <%= (_('Please contact your administrator, the request id for finding the logs: "%s"') % request.uuid) %> |
There was a problem hiding this comment.
Tested with a non-admin user. looks like since the error is captured, there is actually nothing in the log.
Also, this prints a full uuid, but at least for file-based logging we only log the first 8 characters of it.
There was a problem hiding this comment.
resolved, see the new commit for the solution, I only display that for non-admin users
tbrisker
approved these changes
Jun 24, 2020
tbrisker
pushed a commit
that referenced
this pull request
Jun 24, 2020
When obsolete STI records deletion was introduced, we allowed even non-admin users to delete that. This patch makes it possible only for admins to delete records from the database. Non-admin users only see a short message explaining what happened, that they should contact administrator and request ID that can help administrator to figure out what happened. It was also necessary to add explicit require_login call, otherwise User.current was nil in case the stack failed before user was loaded. This also made the page looks much nicer, becase the menu can be properly rendered. (cherry picked from commit f44a335)
|
2.1 - 693f8f7 |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When obsolete STI records deletion was introduced, we allowed even
non-admin users to delete that. This patch makes it possible only for
admins to delete records from the database. Non-admin users only see a
short message explaining what happened, that they should contact
administrator and request ID that can help administrator to figure out
what happened.
It was also necessary to add explicit require_login call, otherwise
User.current was nil in case the stack failed before user was loaded.
This also made the page looks much nicer, becase the menu can be
properly rendered.
For the reviewer - under admin account
non-admin account
to reproduce, install a plugin, e.g. discovery, change some discovery setting and then comment it in Gemfile. After server restart, enter setting page under non-admin user. You shouldn't be able to delete records even if you know the magic parameter. Pinging @tbrisker as he was who pointed this weakness out.