Problem/Motivation

Nowhere in BookNavigationBlock do we do anything with roles, the current user, or even permissions. This is just a remnant from long ago that is incorrect. We're working on making this block cacheable in #2483181: Make book navigation block cacheable.

Proposed resolution

Remove the user.roles cache context.

Note that this block has max-age=0 explicitly anyway, so it will never be cached. This does not change that. Hence this cannot possibly create any security problems.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

Why during RC?

Because core setting the right example wrt cacheability metadata is important: developers will look at core's examples and learn from them, core being wrong is problematic.

CommentFileSizeAuthor
#5 2604092-5.patch727 byteswim leers
#3 2604092-2.patch612 byteswim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 2604092-2.patch612 bytes

Status: Needs review » Needs work

The last submitted patch, 3: 2604092-2.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Postponed
StatusFileSize
new 2604092-5.patch727 bytes

Blocked on #2541344: BlockBase subclasses should merge their cache tags/contexts with the parent's (BlockBase's) because patch is relative to that.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: BookNavigationBlock has the 'user.roles' cache context, is wrong » [PP-1] BookNavigationBlock has the 'user.roles' cache context, is wrong
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: [PP-1] BookNavigationBlock has the 'user.roles' cache context, is wrong » BookNavigationBlock has the 'user.roles' cache context, is wrong
Status: Postponed » Needs review

#2541344: BlockBase subclasses should merge their cache tags/contexts with the parent's (BlockBase's) landed, unpostponing.

zaporylie’s picture

zaporylie
he/him
Primary language Polish
commented
Status: Needs review » Reviewed & tested by the community

I think this is ready to be committed. Patch does not break anything on any existing site nor change how BookNavigationBlock blocks are cached - just cleaning up redundant code.

Thanks Wim

catch’s picture

catch
he/him
Primary language English
commented
Issue tags: -rc target triage +rc target

Discussed with @xjm, @effulgentsia and @alexpott.

Tagging for RC target. This as near zero impact, but also absolutely zero disruption.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 2604092-5.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community

Testbot was aborted for whatever reason. Restoring status & re-testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 4000915 and pushed to 8.0.x. Thanks!

  • alexpott committed 4000915 on 8.0.x 
    Issue #2604092 by Wim Leers: BookNavigationBlock has the 'user.roles'...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.