Looks like the latest dev and beta have changed how access is handled and now return an array to quiz_take_access() which is the access callback to node/%quiz_menu/take. This access callback expect a boolean and therefor is always TRUE and will let anybody take the quiz regardless of permissions.

Consider reverting this logic build up or do some kind of boolean grab from the key'success' => TRUE

CommentFileSizeAuthor
#11 unintended_anonymous-2797513-11.patch3.48 KBdjdevin
#8 unintended_anonymous-2797513-8.patch3.21 KBjoelpittet
#6 unintended_anonymous-2797513-6.patch1.15 KBdjdevin
#4 2797513-2.patch943 bytesjoelpittet
#2 2797513-1.patch551 bytesjoelpittet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet created an issue. See original summary.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented
Status: Active » Needs review
FileSize
2797513-1.patch551 bytes

Here's a patch to resolve this, hope I took the right route?

Status: Needs review » Needs work

The last submitted patch, 2: 2797513-1.patch, failed testing.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented
Status: Needs work » Needs review
FileSize
2797513-2.patch943 bytes

Actually two problems,
If they don't have access 'success' should be FALSE, no?

   if (!user_access('access quiz') || !node_access('view', $quiz)) {
     $hooks['node_perm_access'] = array(
-      'success' => TRUE,
+      'success' => FALSE,
       'message' => t('You are not allowed to take this @quiz.', array('@quiz' => QUIZ_NAME)),
     )

Status: Needs review » Needs work

The last submitted patch, 4: 2797513-2.patch, failed testing.

djdevin’s picture

djdevin
Location Philadelphia
CreditAttribution: djdevin at Cadmium commented
Status: Needs work » Needs review
FileSize
unintended_anonymous-2797513-6.patch1.15 KB

Just changed one of the calls, should fix the tests.

Status: Needs review » Needs work

The last submitted patch, 6: unintended_anonymous-2797513-6.patch, failed testing.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented
FileSize
unintended_anonymous-2797513-8.patch3.21 KB

Thank you @djdevin for helping fix most of the test failures!

The test that failed needed a user with 'access quiz' permission and anonymous doesn't have that by default, so I logged in a user that does (as mentioned in comment).

Also added some extra tests to ensure the message that says they are blocked isn't showing by accident (consider it a regression test maybe?)

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented
Status: Needs work » Needs review
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet as a volunteer commented

Seems to be good now. Thoughts @djdevin?

djdevin’s picture

djdevin
Location Philadelphia
CreditAttribution: djdevin at Cadmium commented
FileSize
unintended_anonymous-2797513-11.patch3.48 KB

Looks good, I just extended the check to /take as well to make sure that page is blocked as well.

djdevin’s picture

djdevin
Location Philadelphia
CreditAttribution: djdevin at Cadmium commented
Status: Needs review » Fixed

Thanks, fixed!

  • djdevin committed 0a4f9a6 on 7.x-5.x authored by joelpittet 
    Issue #2797513 by joelpittet, djdevin: Unintended anonymous access to...

Status: Fixed » Closed (fixed)

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