The user formatter has a setting "Link to the user". The link is only displayed if the account (user that the output is being generated for) has access to view the target user (the one whose name is being displayed).
Expected behaviour: Drupal check access accurately, calling all custom hooks such as hook_entity_access.
Actual behaviour: Drupal checks permission 'access user profiles'
This isn't a security issue - when the link is clicked to visit the page the correct access check is performed, so there will be access denied/403 if necessary.
Comment | File | Size | Author |
---|---|---|---|
#28 | user.username.formatter.2921114-24.patch | 5.8 KB | AdamPS |
#24 | user.username.formatter.2921114-interdiff-22-24.txt | 1.1 KB | AdamPS |
#22 | user.username.formatter.2921114-22.patch | 5.79 KB | AdamPS |
#18 | user.username.formatter.2921114-18.patch | 5.78 KB | AdamPS |
Comments
Comment #2
AdamPS CreditAttribution: AdamPS commentedComment #3
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedBlocker for Administer Users by Role.
Comment #5
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedSome of the existing tests cover this. At the moment they assert the presence of a span, which is incorrect for a user with admin permission - it should be a. First patch is test-only, should fail.
Comment #6
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedComment #9
a.milkovskyThe patch looks good and makes sense +1 for RTBC.
Comment #10
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedStatus is RTBC based on #9
Comment #12
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedReroll
Comment #13
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedThis was just a re-roll so assume it doesn't need another review - putting status back to RTBC
Comment #14
a.milkovskyThe re-roll looks good.
Comment #15
alexpottFor a more BC friendly change should this be something like (there might be neater logic)
Also the test coverage seems quite indirect. I think we could do with some more explicit test coverage. Especially with the extra conditionality being introduced by this patch.
Comment #16
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedHere is a new patch with an explicit test.
Regarding BC, I propose that in this case the existing behavior is a bug so we do want to change it. The specific case that is relevant to BC is if the user has 'access user profiles' permission but does not have access. That can happen if an access hook returns forbidden.
Please can someone review the test change then set back to RTBC?
Comment #17
BerdirWhy is this added?
Usually we just duplicate paths in test, this just adds indirection which makes it IMHO harder to understand.
Comment #18
AdamPS CreditAttribution: AdamPS at AlbanyWeb commented@Berdir thanks for the review - that's now fixed in the new patch.
Comment #19
AdamPS CreditAttribution: AdamPS at AlbanyWeb commented@a.milkovsky, @Berdir or anyone else please can you set to RTBC so we can unblock this and hopefully get it fixed in time for D8.7?
It just needs a quick review of the tests, which maybe Berdir has done already? No need to actually test because the code hasn't changed since last time, only tests have changed.
Comment #20
a.milkovskyLooks good -> RTBC
Comment #21
larowlanComment #22
AdamPS CreditAttribution: AdamPS at AlbanyWeb commented@larowlan Thanks here is the reroll
Comment #24
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedFix more deprecations and coding standards
Comment #25
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedThis is just a reroll, the patch is the same as before, so it doesn't need reviewing again
Comment #26
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedI tend to think this issue does not need a change notice because it is just fixing a bug. In case people think it does I don't want to delay commit of the issue so I have written one just in case - feel free to delete/ignore it.
Comment #27
alexpott#24 includes another patch.
Comment #28
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedSorry that was the wrong patch file
Comment #29
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedComment #30
larowlanAdding issue credit
Comment #31
larowlanCommitted bd6a7df and pushed to 8.7.x. Thanks!
Comment #33
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedGreat, many thanks @larowlan
@a.milkovsky helped by reviewing 3 patches so I wonder if we could add credit for him too?
Comment #34
larowlanSure
Comment #35
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedThanks.
Just one blocker in this area left now! #2992848: Allow customisation of permission to create user with blank email