It used to check the group_type-plugin_id combo in v1 and I wanted to be clever or something and changed it to group_type in v2/v3. That was a huge mistake, let's be glad we are only barely out of beta and not in a full release yet or this would be a security issue. Let's fix this ASAP.

This is the second query access bug in a short period of time so let's also make sure we add some simply use cases in tests as a follow-up. The current tests cover a lot of cases, but in an abstract manner. Some tangible tests wouldn't hurt, it would seem.

CommentFileSizeAuthor
#9 group-3343405-9.patch11.81 KBkristiaanvandeneynde
#5 group-3343405-5.patch14.06 KBkristiaanvandeneynde
#3 group-3343405-3.patch11.11 KBkristiaanvandeneynde
#2 group-3343405-2.patch12.45 KBkristiaanvandeneynde

Comments

kristiaanvandeneynde created an issue. See original summary.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Active » Needs review
StatusFileSize
new group-3343405-2.patch12.45 KB

This fixes EntityQueryAlter already.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
StatusFileSize
new group-3343405-3.patch11.11 KB

Cleaner approach and takes care of all queries. Gonna think about optimizations while tests run.

Status: Needs review » Needs work

The last submitted patch, 3: group-3343405-3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs work » Needs review
StatusFileSize
new group-3343405-5.patch14.06 KB

Contains optimizations.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs review » Reviewed & tested by the community

Going to commit this already, but I would like to add the following tests ASAP:

Multiple bundles of a single entity type

  1. Group type gives access to view basic pages, but not articles
  2. Set up one of each across two groups
  3. Visit /node and verify you only see basic pages
  4. Repeat with groups instead of group types

Multiple plugins for a single entity type

  1. Group type gives access to view members, but not "user as content"
  2. Set up one of each across two groups
  3. Visit relationship overview and verify you only see members
  4. Repeat with groups instead of group types

Will leave the issue open and commit ASAP.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
StatusFileSize
new group-3343405-9.patch11.81 KB

Adding tests that went red as expected without this issue's patch. Should go green now that we've committed everything.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Reviewed & tested by the community » Fixed

Sod it, they only add tests and they go green on my local. Committing and letting the on-commit testbot run it again now that I've added the test group.

Status: Fixed » Closed (fixed)

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