Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
When an access denied error is logged to watchdog then all context why the error happened is lost. Example: a client requests /graphql and gets an access denied. The site admin cannot check why the access denied was triggered and what the cause is.
Proposed resolution
Add more 403 exception context to the log message in ExceptionLoggingSubscriber. That way an admin quickly sees why a request was access denied.
Remaining tasks
Review patch and get consensus that we want this, then write tests.Wait for #3113876: The "Symfony\Component\HttpKernel\Event\GetResponseForExceptionEvent::getException()" method is deprecated since Symfony 4.4, use "getThrowable()" instead to land in 9.0.x.- Final reviews + RTBC.
- Commit.
User interface changes
The log message at /admin/reports/dblog will have a better message.
Before
/admin/reports
After
Path: /admin/reports. Drupal\Core\Http\Exception\CacheableAccessDeniedHttpException: The 'access site reports' permission is required. in Drupal\Core\Routing\AccessAwareRouter->checkAccess() (line 117 of /var/www/html/core/lib/Drupal/Core/Routing/AccessAwareRouter.php).
API changes
None.
Data model changes
None.
Release notes snippet
TBD
Comment | File | Size | Author |
---|---|---|---|
#47 | 3114122-9.1.x-47.patch | 3.31 KB | jungle |
#47 | 3114122-8.9.x-47.patch | 3.32 KB | jungle |
#47 | 3114122-8.8.x-47.patch | 3.35 KB | jungle |
#43 | 3114122-43.patch | 3.32 KB | jungle |
#30 | 3114122-30.patch | 3.88 KB | jungle |
Comments
Comment #2
klausiPatch.
Comment #3
jungleThis is what I am looking for for a long time!!! Thanks, @klausi!
Comment #4
jungleComment #5
jungleA test added only, ignore interdiff as it’s unnecessary.
Comment #6
jungleComment #7
jungleComment #8
klausiLooks like you uploaded the wrong patch file, can you check?
Comment #9
jungleYes, a wrong patch uploaded, sorry!
Comment #10
jungleComment #12
jungleComment #13
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedComment #15
klausiassertText() is deprecated and should not be used anymore.
same for assertRaw()
we should not assert line numbers, then this test will easily break when the file is changed.
Can we use $this->assertSession()->pageTextMatches() with a regular expression? That way we can leave out some details like the line number and just make sure that the permission name is on the page for example.
there should be no space after "("
Comment #16
jungleThanks, @klausi for reviewing!
Patch adjusted according to #15
Comment #17
klausiThanks, looks good to me! Some super minor small things we should fix and then this can be RTBC.
Should be "Tests that a 403 event is logged with the exception triggering it."
Empty line that can be removed.
We should check that the path /admin/reports" is in there.
Empty line that can be removed.
Comment #18
jungleThanks! Addressed #17 1, 2, 3, 4.
Comment #20
jungleThe regex should escape '/' in the path
Comment #22
jungleAnother try
Comment #23
jungleComment #24
jungleShould pass this time. An extra markup surrounds the path, probably. But #18, 20 and 22 passed on my local. strange.
Comment #25
dwwMostly looks great, thanks everyone!
A few nits/concerns. I won't set to NW, but I'm not RTBC'ing, either...
I'm not sure about the legibility of how this is formatted, especially the
@uri.
part (with the dot after the path). But I don't have a better suggestion, so I guess this is fine as-is.We don't re-use this, so we could have it as a literal argument to drupalGet() a few lines down.
Shall we assert that we actually got a 403 response to this?
This comment is unneeded, the code documents itself.
Can we target this assertion more specifically so that we know we're hitting the type, not just seeing 'access denied' anywhere on the page?
Also here -- let's make this a more specific assertion, if possible.
Someone will probably complain that this change is out of scope, so we might as well remove it before they get a chance. ;)
Comment #26
jungleThanks, @dww for your detailed reviewing.
#25.1 skipped, 2 - 7 addressed.
Comment #27
jungle#26 wrong interdiff, missed #25.7
And space after , to xpath queries.
Comment #28
dww@jungle: #27 looks great, thanks!
Sadly, dreditor ate one of my comments that was supposed to be in #25. :( Don't know how that happened. Re-writing it here:
I don't love this regexp. Too much use of
.*
, which should be used sparingly, if at all. If there's only whitespace differences between what's expected and what's appearing, we should use\s*
to only match whitespace.Bigger picture, I'd rather understand why this isn't matching up and solve it with a simpler / cleaner solution than a regexp peppered with
.*
.Looking at the source for
pageTextMatches()
, it relies ongetText()
, which ultimately looks like this:Knowing that's what's happening to the page for the purposes of this assertion, I wonder if we can write the regexp more specifically...
Thanks!
-Derek
Comment #29
jungleThank you @dww! Good point. A new patch is coming soon.
Comment #30
jungleComment #31
jungleComment #32
jungleRemove unnecessary assertions.
Comment #34
jungleFix location assertion
Comment #35
jungleComment #36
jungleA new patch for branch 8.8.x
9.0.x branch tests failed in #34, because of deprecation notices. should it be fixed in a separate issue?
Comment #37
jungleTwo issues related to the 9.0.x branch tests failure in #34,
Comment #38
dwwThanks for sorting that out! Seems like this should wait until the dust settles on those two issues, since we can't safely commit this to 9.0.x until those are complete.
Comment #39
dwwMeanwhile, looking more closely at the latest patches...
We've got
$this->baseUrl
. Why do we also need$base_path
? Why aren't we always using the same base URL for this test?Hrm, this is still a bit clumsy. This wasn't what I had in mind with my previous suggestion...
That's still a very permissive regexp to be asserting. It works, but it's not great.
I still believe it'd be possible to write a single regexp that cleanly matches the entire expected thing, without resorting to all this.
Thanks,
-Derek
Comment #40
jungle@dww, thank you again for reviewing!
Re #39.1
Here $href is a full URL, for example, http://example.com/admin/reports or http://example.com/subdir/admin/reports
here, the path is a relative path. for example /admin/reports or /subdir/admin/reports
So baseUrl is not applicable here.
Re #39.2 and 3.
Writing one regex, it's good, but I don't think it's necessary, special chars in the message, make writing regex hard and less readable.
My approach is to break the message into 3 parts. the first part and last part, and the number part. the number part uses regexp only to address #15.3. If the message contains the first part and last part and matches the regex of number part, assuming it's the expected message. Maybe I should add a comment.
If you still prefer one-line regex, I will do.
Comment #41
dwwRe: #40 re #39.1:
Thanks for the explanation. Makes sense, but then maybe we need a better variable name for
$base_path
. B/c as it is, it's not at all clear from looking at the test what's going on here. E.g.$relative_base_path
or something.Re: #40 re: #39.2 and 3:
It's not really up to me. Personally, I think 1 well-crafted regexp would:
a) Cover everything.
b) Be easy enough to read.
c) Take a lot fewer lines of test code.
d) Be faster to test.
To help this particular regexp be more legible, you don't have to use '/' as the pattern boundary character. You could use something like '@' which wouldn't appear in the rest of the regexp, then you don't need to escape all the '/' chars we're expecting...
Completely untested, but something like:
Agreed we can't know (and don't want to assert) the root of the filesystem path where we find
AccessAwareRouter.php
. Maybe what you had is Good Enough(tm). But we could make sure the relative path within core itself is there, which shouldn't change. Note that I'm proposing.+
here, not.*
since we expect there will be at least 1 character matched (.*
can match 0 things, which wouldn't be valid here).Comment #42
jungleThank you! Will submit a new patch today or tomorrow according to your feedback, have to admit that I am not good at regexp :p
Comment #43
jungleRe #41:
$base_path
vs$relative_base_path
$base_path
isn't used in the patch anymore. But, personally, I prefer$base_path
, as it's being used in the core a lot.Re #41, regex
Thanks, learnt that
/
is not the only delimiter available. Your$regex
is almost right. 3 tiny changes made.\\
-->\\\\
have to use 4 backslashes to escape\
, see a blog post for more, https://www.developwebsites.net/match-backslash-preg_match-php@Path /admin/reports
-->@Path: .+admin/reports
. Missing:
,and as the base path matters. The path could be like/admin/reports
or/sub.directory/admin/reports
, and the base path may contains meta chars like.
as you can see, so use.+
is applicable, or have to consider escaping meta chars..+/core/lib/Drupal
, agree with you on using.+
, one more reason is that the path may contain meta chars too..php)\.@
-->.php\)\.@
Comment #44
jungleCombined with the patch from #3113876 to make sure that #3113876 is the only one blocker.
Comment #45
jungleThe one failed test is irrelevant I think. And yes, 3113876 is the only one blocker. and it's RTBC'd.
Comment #46
jungleComment #47
jungle3113876 landed
Comment #48
dwwSweet, thanks!
Re: #43:
1: Oh right. Silly PHP. :/
2: Yeah, good point.
.+
is fine here.3: 👍
4: 👍
Test now looks great. I intentionally broke this feature in various ways, and the test is failing as expected in each one.
Re: #47:
At this point, as a new feature with a string change, I'd be shocked if the core committers will accept this in the 8.8.x branch. I guess it's nice to provide the 8.8.x patch, just in case, but don't get your hopes up. ;)
Although 8.9.0-beta1 is out, I do think this is viable for the 8.9.x branch, since it's not very disruptive (I don't think we consider the formatting of watchdog messages "API" in any way) and this definitely improves debugging access and 403s significantly. Helping site builders and developers get access control right is an extremely important thing, and anything we can do to help them make sure they're getting it right is worth doing. It is a string change, but it's almost entirely placeholders, and it's only visible to site admins reading the logs (not in the main UI anywhere), so if it's not translated, it's no big deal.
I'm still a bit concerned about #25.1. The message has periods in it in multiple places, although there aren't full sentences. The message also continues after the period inside the exception message with "in ..." as if it's all 1 thing. Reading the code, it looks plausible it could be a real sentence, but the @message itself can be multiple sentences. My sense is we could improve this before commit, but I still don't have a solid suggestion, so I'm not going to NW for this.
In fact, I'll RTBC (since I don't see anything else to improve) and let the core committer(s) decide. ;)
Thanks again!
-Derek
Comment #49
jungleThank you again for reviewing @dww! Do not you sleep? :p
Yes, and it is encouraged that do not translate exception messages right now. #2055851: Remove translation of exception messages is the issue to remove translations from exception messages and Needs Review :)
I had one real experience troubleshooting 403 with this patch in my work, it's useful to me. I am fine with #25.1, as long as those key info are there. But yes, would be great always if we could make it better.
The patch for 9.1.x is applicable to 9.0.x as well.
(Updated Remaining tasks in IS)
Comment #50
dww@jungle re: #49
Not enough. :p A question that also applies to you, it seems! ;)
I'm not talking about the exception text. I'm talking about this:
'Path: @uri. %type: @message in %function (line %line of %file).'
The words: "Path", "in", "line" and "of". ;) Again, it's only visible to admins, and those 4 words are probably understandable to anyone. As you say, the @message part will be expanded with something even more complicated that will be in English. So yeah, I don't think this is a legitimate reason not to get this into 8.9.x, even though beta1 is out. That's what I was trying to preemptively address.
Cheers,
-Derek
Comment #53
catchCommitted 5fbd9c7 and pushed to 9.1.x. Thanks!
A bit close to RC for this one I think, so leaving it there.
Comment #54
jungle@catch, thanks for committing! Is it possible backporting to 8.9.x? This is useful for troubleshooting 403 errors. It's a small change but makes a difference. Hoping that this could be backported to 8.9.x or at least 9.0.x.
Comment #55
dww@jungle re: #54: @catch already answered exactly that question at the end of #53:
I don't personally agree with that assessment. It's a change to a watchdog message. I don't know why we'd consider this risky or potentially disruptive. If I were a release manager (and we can all be thankful I'm not), I'd be happy to call this a 'task' and get it into 9.0.x and 8.9.x. ;) /shrug
Cheers,
-Derek
Comment #56
jungleRe #55, yes, I saw that and I knew that means no chance to be backported. Just wondering is there a 2nd chance. Anyway, it's still possible to use this patch in other branches, just another patch to manage :)
Comment #57
catchOK took another look I think it's fine, but better error messages are tasks rather than features ;)
Comment #60
dwwWhoot! 🎉😉 Thanks for the 2nd look, @catch. Maybe I'm qualified to be a release manager, after all. 😜
Comment #61
jungleThanks, @catch for giving it a 2nd chance! Thanks to @dww again for your tireless and patiently reviews. And thanks @klausi for your initial works. 🍻
Comment #62
klausiGreat work jungle! I see you contributing everywhere, big thanks from me ❤️
Thanks also dww for convincing catch :)
Comment #64
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedI have created a related issue to remove unnecessary backtrace string #3167390: ExceptionLoggingSubscriber should not log backtrace string on access denied exceptions.