Admin users should know site report permission allows for more than viewing

Consider this a feature req. and not a bug. Moving to D8.x

I'll take this on.

Looks good to me. I'd mark as rtbc, but I suspect there are many people who would recommend that this include tests.

Edit: Disregard that. The bot is correct, this needs work. Specifically the existing test needs to be updated for the new permission. The bot currently fails in part because it doesn't have permission to clear the log.

I added the permission to the test and it passes on my local. Seeing what the bot thinks of it...

Helps if you set it to "Needs review". Derp.

lgtm. marking as rtbc.

Is it just me that this name does not really tell you what this permission is about?

+++ b/core/modules/dblog/lib/Drupal/dblog/Form/DblogClearLogForm.php
@@ -53,11 +53,13 @@ public function getFormID() {
+    $account = \Drupal::currentUser();
...
+      '#access' => $account->hasPermission('administer site reports'),

just do '#access' => $this->currentUser()->hasPermission(). And I agree with @dawehner maybe the name could be better since Administer to means means a lot more than ability to purge the db log.

Suggestions, then? "Clear site reports", perhaps?

Why does the permission belong to system.module when it's only used by dblog.module? Not sure there is a use case for other watchdog modules to use this, for example it doesn't make sense for syslog to allow it.

Alternatively, do we need an entire new permission? Is "administer site configuration" enough to cover this?

Definitely should be dblog permission and not a system permission - nice catch @longwave.

@alexdmccabe as this is a dblog specific thing - it is possible that another module will add reports to "/admin/reports" so "Clear site reports" is problematic. How about "Purge watchdog table"? Considering this is a technical task maybe giving the permission a technical name is ok... tricky. Or maybe "administer site configuration" is enough.

Okay, here it is with "administer site configuration", since that seems to be popular and an easy way to accomplish this.

Patch works fine.

Updated patch.

I don't think we want to put the access permission on the 'details' form element ... as for example the submit button would still be there.
You should probably put that on the main level of the form, maybe even in the dblogController.

Moved the access permission back to the fieldset. Interdiff is from my last patch.

Moves access permission to $form.

Unassigning from myself.

Rerolled.

Aaaaand setting to needs review.

This will need a re-roll if #2506449: Transform "Clear log messages" submit button into a link in admin/reports/dblog is commited

Reuploading #24 for retest, as it looks like it should still apply.

Lets add some tests for this feature. I'm afraid the route is still accessible from the URL.

Hm, I was right, so in order to get this issue RTBC the 1 fail test should be fixed by this patch.

The test is passing now, the issue was that the route itself needed the permission changed from access site reports to administer site configuration.

Ok, this looks good, however I'm thinking now in the upgrade scenario.

This update will remove functionality for users with 'access site reports', they won't be able to clear logs anymore.

From Comment #12

Alternatively, do we need an entire new permission? Is "administer site configuration" enough to cover this?

I think we do, because we cannot grant administer site configuration to users with access site reports permissions.

Isn't removing this functionality from users who only have access site reports the whole point?

It is, but you don't know if some site admin rely on this permission to allow delete logs at this moment.

No functionality should be broken with the update. After the update you give more granularity if site builder want to use it.

The error looks unrelated and isn't occurring on my local environment, so I'm setting it back to Needs Review.

Great, so the last thing here is, provide hook_update_N that grant all the users that at the moment can access site reports the permission clear log messages.

Adding "Random test failure" tag because of #45 per https://www.drupal.org/node/2399959.

Ignore that failure, it was testing the interdiff that I accidentally uploaded with a .patch extension. The actual patch passed.

Still not convinced the new permission is needed. If we document in the release notes that users now need "administer site configuration" to clear the logs, then that should be should be enough? Especially if we consider the existing behaviour a bug?

Thanks @alexdmccabe this is working as expected. I tested the upgrade path and is working fine.

Just adding the "Warning: Give to trusted roles only; this permission has security implications" to the permission since this is really important to check.

Still not convinced the new permission is needed. If we document in the release notes that users now need "administer site configuration" to clear the logs, then that should be should be enough? Especially if we consider the existing behaviour a bug?

Well, according OWASP Logging protection

The logging mechanisms and collected event data must be protected from mis-use such as tampering in transit, and unauthorized access, modification and deletion once stored.

If we use "administer site configuration" we are allowing a user that could change the site name also to clear logs, this doesn't make sense to me.

Moving this to RTBC since all my requests on #35, #50 were addressed.

The new permission wouldn't be covered by the list of elevated permissions at https://www.drupal.org/security-advisory-policy, but the existing one is. Similarly 'Administer site configuration' is covered by that policy, and that already allows destructive operations. Personally I'd got for 'Administer site configuration' since that's the catch-all for trusted users really and would then not add a permission for a single link.

Just to give a bit of context here. This are the things that an user with 'administer site configuration' permissions can do at this moment in core.

• Update Manager settings /admin/reports/updates/settings
• Available updates /admin/reports/updates
• Manual update check /admin/reports/updates/check
• Basic site settings /admin/config/system/site-information **
• Cron /admin/config/system/cron
• Logging and errors /admin/config/development/logging
• Performance /admin/config/development/performance **
• File system /admin/config/media/file-system **
• RSS publishing /admin/config/services/rss-publishing
• Regional settings /admin/config/regional/settings
• Image toolkit /admin/config/media/image-toolkit
• Maintenance mode /admin/config/development/maintenance **
• Run cron /admin/reports/status/run-cron
• Date and time formats /admin/config/regional/date-time
• Add date format /admin/config/regional/date-time/formats/add
• Status report /admin/reports/status
• Books /admin/structure/book/settings

I indicated with ** the actions I think may be incompatible with the ability of clear logs.

@dagmar what do you think is incompatible?

@dagmar I'm not sure of the point you are trying to make there. Why do you think those are incompatible with clearing logs? "administer site configuration" should only ever be granted to trusted users; none of the items you list are things that you would want even a casual administrator to be able to do, and none of them are things that you will be reconfiguring on a regular basis.

Note that "administer site configuration" is also needed to change the settings on the "logging and errors" page; this can effectively be used to clear the site logs by changing "Database log messages to keep" to 100, flooding the site in some way to generate 100 messages, and then waiting for cron to run.

@longwave thanks. Now I see your point. So maybe to clarify all this. There are some cases where Drupal is being used as platform where trusted users may not be necessary mean security officers (in the sense of responsible for the security of the system).

I asked in the company where I work to a person focused on company security and he said the clear logs permission should not be shared with other kind of site functionality, like change the site name.

Another scenario I can imagine is the owner of a site, like a client, who want to be able to configure the Site name or the the text for a 404 page and accidentally or in purpose want to clear the logs to hide some mistake.

However, I see what you mean like a way to bypass this behavior using cron to flush the logs. So, it seems the dblog cannot be considered as a full logging system from a security point of view, and syslog or any other solution should be used instead.

Yes for log retention you need to use syslog or similar. Flushing logs manually or them getting rotated away after 1000 entries isn't really different from a security standpoint.

So it sounds like we're going back to "administer site configuration"?

Yes, I created this follow up: #2732113: Clarify in the dblog_help that the dblog module should not be used for forensic log

Removing string change tag, reinstating random test failure tag for #45 (don't know why it was removed, there was indeed a random test failure: #1635646-45: Admin users should know site report permission allows for more than viewing).

Not sure if the "needs security review" tag is still necessary.

This looks fine, I tested the functionality in simplytest.me

Just adding 3 more lines on the tests to check responses. From my side this is RTBC.

This is going to fail again, the interdiff has a .patch extension and the testbot won't like that. I know because I did the same thing in #51. :)

Told ya. Setting back to "Needs review" because the interdiff shouldn't be tested anyway.

Thanks @alexdmccabe!

Removing the Random test failure tag. This tag should be put on issues about random test failures, not any issue that happens to have a random test failure. E.g., see #2724871: Random failure in \Drupal\migrate_drupal_ui\Tests\d7\MigrateUpgrade7Test. Please do not add the tag back as this pollutes the list of issues to fix random test failures.

@xjm - that's not what https://www.drupal.org/node/2399959 says.

If you hit that you should follow the following guideline:

DON'T retest the patch, but just reupload your existing patch in a new comment
Add the "Random test failure" tag to the issue

¯\_(ツ)_/¯

Thanks @alexdmccabe. I think that's just a misunderstanding. I'll try to clarify the handbook page. (I think that handbook page is also partially duplicating more extensive documentation elsewhere.)

No problem, sorry for the confusion and pollution!

Removing needs security review. Do we need a Change record for this?

Change record added https://www.drupal.org/node/2735079

Thanks for exploring other solutions for this issue.

Looking at the summary, the goal of this issue is:

Admin users should know before granting the permission that it allows for more than viewing.

I'm retitling the issue to reflect that scope.

Discussed with @catch. The permission for this page is already marked with restrict access in system.permissions.yml:

access site reports:
  title: 'View site reports'
  restrict access: true

This means it gets a warning in the UI already:

It also means that the security team will not release SAs for exploits that require the permission, etc.

I think the current approach of changing the permission to clear logs to "Administer site configuration" is incorrect. In fact, it might make the situation worse, by conflating this specific permission with an even more powerful one. Plus, the change only for clearing the site reports implies falsely that viewing the site reports is safe, which it is not. All sorts of information is disclosed there.

What I actually think will address the underlying concern of the issue is to simply change the label for the permission to "Administer site reports". That way, it is consistent with what the permission actually does, and also reinforces the restrict access warning. That should address the underlying concern of the original report, without adding an additional permission, making disruptive changes to the permissions scheme, or giving a false sense of security for site reports.

Thanks everyone for your patience and trying out different approaches -- sorry for there being a couple recommended changes of direction.

Thanks for your clarification @xjm. What do you think about this description?

Tagging this as novice to get a review from some English native speaker.

Having tested the 'View Site Reports' permission, what it actually gives you is:

- Recent log messages (including delete)
- Top 'access denied' errors
- Top 'page not found' errors
- Top search phrases
(and that's it)

- On it's own, it doesn't allow you to access /admin/reports/status, so I've removed that.
- I was able to list each report in the same amount of space, so I have, and as "manage the site logs" is in fact just view and delete, I've stated that verbatim too.
- Used Oxford comma as per style guide, report names aren't capitalised as they're not proper nouns.

For committers: given this is such a modest change compared to the initial approach (which involved renaming a permission and changing behaviour) - it's essentially a non-disruptive bug fix - can it be bumped up to 8.3.x? Or does the translatable string disqualify it?

Added Issue Summary template.

Looks good to me.

Thanks @dagmar and @wturrell.

+++ b/core/modules/system/system.permissions.yml
@@ -17,6 +17,7 @@ view the administration theme:
+  description: 'View access denied, not found, and search reports. View and delete log messages.'

I find this confusing to read. "View access denied, not found" in particular can be easily mis-parsed as an error message. Usually in writing we would delineate an "access denied" report or "not found" report with punctuation or typeface. Also, it's somewhat redundant with the label.

More importantly, though: this also has the problem as some other issues do where we are adding a description when we undertook significant work to remove them early in the D8 development cycle. Descriptions are to be avoided; they add more to read and make the page more overwhelming for users.

It would be better to not add a description. Instead, as I stated in #81 (maybe it got lost in a long post, sorry) is that I think we should simply change the label of the permission to:
Administer site reports
And not add a description.

Thanks!

Just changed Label/Title as suggested by @xjm

Patch applied successfully. Changes looks OK

Shouldn't we add additional access control to the 'clear logs' button and leave this permission as is?

[contains minor edits]

If no description is a fixed constraint, and the choice is "View site reports" (as we have now) vs. "Administer site reports", I'd actually favour leaving it how it is. Despite the omission of the ability to clear logs, I find "View site reports" more meaningful (though I don't think the 'trusted users' warning is all that useful if there's no canonical place in Help or drupal.org docs to check what the "security implications" are - see later comments.)

To me, "Administer" suggests a similarity with "Administer content", or "Administer content types", or "Administer menus and menu items" or "Administer modules"… my expectation is a Reports admin page with a list of reports I can control. Perhaps there are pre-installed reports, like the default Views and Content Types, that I can enable, disable, or customise, with an "Add new report" button to create my own.

I also find the term "site reports" a little ambiguous:

- "site" may be redundant, what makes it a "site" report, isn't every report a "site report"?
- this permission only activates three reports on /admin/reports when viewed by UID 1 (four including recent log messages), when administrators might assume granting it will give the role access to all of them. They are:

- "Top 'access denied' errors" (list of 403s)
- "Top 'page not found' errors" (list of 404s)
- "Top search phrases"

So there's nothing to actually administer for three of the four.

Reports this permission by itself does not give access to (talking about a clean install here):

- "Available updates" (which consists of three tabs, List, Settings and Updates)

Several potentially confusing things about this: [drifting off-topic, this could be an issue all of it's own]

i) You need 'Administer site configuration' to get a reports menu item and the List and the Settings tabs (but it doesn't give you Update, so you can't install them). That also enables you (most of?) admin/config. This is confusing because if site configuration permission is granted but software updates isn't, you might not expect to see the list of updates at all.
ii) You additionally need "Administer software updates" to get the Update tab. But if you only have this (software) permission on it's own (i.e. "Administer site configuration is unchecked) you get nothing. You might expect software updates by itself to give you /admin/reports/updates/settings (check frequency & email notifications). Arguably we could add an "Also requires Administer site configuration" message or possibly even JS to auto tick Site configuration when Software updates is ticked (not sure how messy/dangerous that gets, especially when deselecting.)
iii) The "Administer software updates" permission is listed even if (as with a new install) the Update Manager (update) module is not yet enabled. I think a conditional reminder to install it would help.

I'd acknowledge i. and ii. may just be a necessary learning curve (and it's not the only place where there is these contradiction).

The necessary permissions for the remaining reports seem logical enough:

- "Status report" (requires "Administer site configuration")
- "Field list" (this requires "Administer content types" - this is the list of fields and the entity types that use them)
- "Views plugins" (requires "Administer views" - this is the table of plugins, e.g. Pager or RSS feed, each with a clickable list of all the views that use them)

More broadly, if the aim is to reduce the word count/complexity/clutter of /admin/people/permissions, have we previously discussed where users are supposed to go if they're still unclear what the permissions mean? As I said at the top, for View site reports, I'm not convinced the trusted users warning is enough by itself. Community docs and web search results for what various permissions do are patchy at best, with more questions than answers.

Would there be support for creating an extra help page under /admin/help/user, starting with the less obvious ones (e.g. some of those we're currently discussing elsewhere). It'd need to be permissible to write at slightly greater length than we can in the inline descriptions, and to include examples where appropriate. Then linking to it from the top of /admin/people/permissions, because (personal hunch, no supporting evidence) no-one ever looks at Help otherwise.

There is already the ability for people to hide all permission descriptions, though I took the primary reason for that to be to reduce the amount of scrolling.

BTW, sorry @xjm – I did read #81 when looking at the previous patch, but clearly managed to completely misunderstand the last part of it. I think I was thrown because you referred to the 'label' instead of 'title' and I took that to mean the description. I must also have read "permission to" as "permission of" or something, etc.

I've refreshed the Proposed resolution in the Issue summary.

Just rerolled patch #92 .

Reviewing the patch

Try to resolve fail test