Follow up from #2584647: Flagging access system not extensible.

This code in UserFlagType::actionAccess() assumes that the $flaggable object will always be available, but the function signature allows for null values.

  /**
   * {@inheritdoc}
   */
  public function actionAccess($action, FlagInterface $flag, AccountInterface $account, EntityInterface $flaggable = NULL) {
    $access = parent::actionAccess($action, $flag, $account, $flaggable);

    // If the acting upon yourself check for permission.
    $is_current_user = $account->id() == $flaggable->id();

Steps to reproduce

  • Enable flag module, and add a user entity flag
  • Select the 'Display checkbox on entity edit form' option when adding the flag
  • Visit user/1/edit and note the fatal error
CommentFileSizeAuthor
#10 interdiff-2802653-7-10.txt724 bytesmartin107
#10 2802653-10.patch1.71 KBmartin107
#7 2802653-07.patch1023 bytesjhedstrom
#7 interdiff.txt1.01 KBjhedstrom
#3 2802653-03.patch691 bytesjhedstrom
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom created an issue. See original summary.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Assigned: Unassigned » jhedstrom
Priority: Major » Critical
Issue summary: View changes

I was incorrect. This can be reproduced with just the flag module. I'm bumping to critical since this was introduced by a critical issue, and results in fatal errors. I've updated the IS with steps to reproduce. I'll post a patch shortly.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Assigned: jhedstrom » Unassigned
Status: Active » Needs review
FileSize
2802653-03.patch691 bytes

This fix seems to work in local testing.

Status: Needs review » Needs work

The last submitted patch, 3: 2802653-03.patch, failed testing.

The last submitted patch, 3: 2802653-03.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

That's not the same at all, your code assumes that flaggable is always the current user.

The whole thing needs to be wrapped in an if ($flaggable). A check without $flaggable means we can't do flaggable specific checks.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.01 KB
2802653-07.patch1023 bytes

This takes the approach suggested in #6.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

Can we add a test for this? I think just calling getFlags() when a flag on users exist should be enough?

martin107’s picture

martin107 CreditAttribution: martin107 commented
Assigned: Unassigned » martin107

I will add the test.

martin107’s picture

martin107 CreditAttribution: martin107 commented
Assigned: martin107 » Unassigned
FileSize
2802653-10.patch1.71 KB
interdiff-2802653-7-10.txt724 bytes

Added extra two checks to AccessTest::testUserFlag().

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented 
+++ b/tests/src/Kernel/AccessTest.php
@@ -149,6 +149,9 @@ class AccessTest extends FlagKernelTestBase {
+    $this->assertTrue($noSelfiesFlag->actionAccess('flag', $user_alice)->isAllowed());

Ha! I love the noSelfiesFlag!

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Reviewed & tested by the community

Works for me.

  • joachim committed c6038af on 8.x-4.x authored by jhedstrom 
    Issue #2802653 by jhedstrom, martin107: Fixed UserFlagType::actionAccess...
joachim’s picture

joachim CreditAttribution: joachim commented
Status: Reviewed & tested by the community » Fixed

Committed. Thanks everyone!

Status: Fixed » Closed (fixed)

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