Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.3
    • Fix Version/s: 1.9
    • Component/s: Choice, Roles / Access
    • Labels:
      None
    • Environment:
      Redhat
    • Database:
      MySQL
    • Affected Branches:
      MOODLE_18_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      29856

      Description

      I have added a Choice with the settings "Always show results to students" and "Publish full results, showing names and their choices" which shows all participants, students can see hidden teachers too. No students have the viewhiddenassigns capability, so I assume this is a bug.

      1. hiddenusers_lib.php.patch
        3 kB
        Dan Marsden
      2. hiddenusers_lib.php.patch
        3 kB
        Dan Marsden

        Issue Links

          Activity

          Hide
          Yu Zhang added a comment -

          Should be fixed now, thanks for the report.

          Show
          Yu Zhang added a comment - Should be fixed now, thanks for the report.
          Hide
          Bente Olsen added a comment -

          Thanks a lot!

          Show
          Bente Olsen added a comment - Thanks a lot!
          Hide
          Dan Marsden added a comment -

          I'm not convinced on this patch... - my feeling would be that if a "hidden" user selected a "choice" then their selection should be shown. - this potentially screws up the limit function available in the choice.

          (of course they shouldn't be shown in the unanswered column...)

          did you test this patch with the limit in the choice?

          thanks!

          Dan

          Show
          Dan Marsden added a comment - I'm not convinced on this patch... - my feeling would be that if a "hidden" user selected a "choice" then their selection should be shown. - this potentially screws up the limit function available in the choice. (of course they shouldn't be shown in the unanswered column...) did you test this patch with the limit in the choice? thanks! Dan
          Hide
          Yu Zhang added a comment -

          Hi Dan,

          The limit is unmodified and will work as before, hidden teacher's choices are recorded, but if you are a student with no viewhidden capability then you do not see the listing of that hidden teacher's result on display. Teachers with viewhidden capability will be able to see all user's choices. I think this should be the correct behaviour. Is there a specific scenario that this would be bad?

          Cheers,

          Yu

          Show
          Yu Zhang added a comment - Hi Dan, The limit is unmodified and will work as before, hidden teacher's choices are recorded, but if you are a student with no viewhidden capability then you do not see the listing of that hidden teacher's result on display. Teachers with viewhidden capability will be able to see all user's choices. I think this should be the correct behaviour. Is there a specific scenario that this would be bad? Cheers, Yu
          Hide
          Dan Marsden added a comment -

          I haven't had a chance to test it yet, but can you check the following scenario please?

          set up a choice with the limit allowing 1 selection to option X - log in as a hidden teacher, and select that option.
          then login as a normal student, and check to make sure you can't select that option, as it should be full.

          thanks!

          Dan

          Show
          Dan Marsden added a comment - I haven't had a chance to test it yet, but can you check the following scenario please? set up a choice with the limit allowing 1 selection to option X - log in as a hidden teacher, and select that option. then login as a normal student, and check to make sure you can't select that option, as it should be full. thanks! Dan
          Hide
          Dan Marsden added a comment -

          this patch doesn't really seem to have "fixed" a bug, rather introduced one. - Yu - did you test this properly?

          Show
          Dan Marsden added a comment - this patch doesn't really seem to have "fixed" a bug, rather introduced one. - Yu - did you test this properly?
          Hide
          Dan Marsden added a comment -

          I can't back this change out as I haven't got access to the new CVS server yet - If you get a chance, can you please back this change out to all versions that you have patched?

          if an admin selects a choice, if you click on "view responses" (only available to admins) - the admin user is not shown like it should be. (I'm guessing hidden users aren't shown either.)

          Show
          Dan Marsden added a comment - I can't back this change out as I haven't got access to the new CVS server yet - If you get a chance, can you please back this change out to all versions that you have patched? if an admin selects a choice, if you click on "view responses" (only available to admins) - the admin user is not shown like it should be. (I'm guessing hidden users aren't shown either.)
          Hide
          David Mudrak added a comment -

          I agree with Dan. A user (let us say teacher) enroled by a hidden assignment may stay invisible to the rest of participants, but only unless (s)he does not submit something into the course.

          Show
          David Mudrak added a comment - I agree with Dan. A user (let us say teacher) enroled by a hidden assignment may stay invisible to the rest of participants, but only unless (s)he does not submit something into the course.
          Hide
          Dan Marsden added a comment -

          I've posted a possible solution to this in MDL-12890 which involves changes to acceslib.php function get_users_by_capability

          this code:
          $from = " FROM {$CFG->prefix}user u
          JOIN (SELECT DISTINCT ssra.userid,
          FROM {$CFG->prefix}role_assignments ssra
          WHERE ssra.contextid IN ($ctxids)
          AND ssra.roleid IN (".implode(',',$roleids) .")
          $sscondhiddenra
          ) ra ON ra.userid = u.id
          $uljoin ";

          by changing to this:
          $from = " FROM {$CFG->prefix}user u
          JOIN (SELECT DISTINCT ssra.userid, ssra.hidden
          FROM {$CFG->prefix}role_assignments ssra
          WHERE ssra.contextid IN ($ctxids)
          AND ssra.roleid IN (".implode(',',$roleids) .")
          $sscondhiddenra
          ) ra ON ra.userid = u.id
          $uljoin ";

          and then when calling get users_by_capability adding 'hidden' to the $fields list

          Show
          Dan Marsden added a comment - I've posted a possible solution to this in MDL-12890 which involves changes to acceslib.php function get_users_by_capability this code: $from = " FROM {$CFG->prefix}user u JOIN (SELECT DISTINCT ssra.userid, FROM {$CFG->prefix}role_assignments ssra WHERE ssra.contextid IN ($ctxids) AND ssra.roleid IN (".implode(',',$roleids) .") $sscondhiddenra ) ra ON ra.userid = u.id $uljoin "; by changing to this: $from = " FROM {$CFG->prefix}user u JOIN (SELECT DISTINCT ssra.userid, ssra.hidden FROM {$CFG->prefix}role_assignments ssra WHERE ssra.contextid IN ($ctxids) AND ssra.roleid IN (".implode(',',$roleids) .") $sscondhiddenra ) ra ON ra.userid = u.id $uljoin "; and then when calling get users_by_capability adding 'hidden' to the $fields list
          Hide
          Dan Marsden added a comment -

          I'm finding it hard to find an easy way of finding if a user assigned to a capability is a given their role via a hidden assignment.....

          I 'could' call get_users_by capability once including hidden users, and then a 2nd time without, and compare the 2 arrays, but that would take a performance hit!

          I need to return all hidden users on the first call, as hidden users should be counted in the results, but not displayed in the list of unanswered users.

          Ideally it would be good if get_users_by_capability returned a variable for each user stating if their role assignment was hidden or not.... - see the hack posted in the comment above for an initial example

          Adding MartinL and Skodak to this - do you guys have any ideas how I might be able to do this? - is there a way without modifying accesslib?

          thanks!

          Show
          Dan Marsden added a comment - I'm finding it hard to find an easy way of finding if a user assigned to a capability is a given their role via a hidden assignment..... I 'could' call get_users_by capability once including hidden users, and then a 2nd time without, and compare the 2 arrays, but that would take a performance hit! I need to return all hidden users on the first call, as hidden users should be counted in the results, but not displayed in the list of unanswered users. Ideally it would be good if get_users_by_capability returned a variable for each user stating if their role assignment was hidden or not.... - see the hack posted in the comment above for an initial example Adding MartinL and Skodak to this - do you guys have any ideas how I might be able to do this? - is there a way without modifying accesslib? thanks!
          Hide
          Martín Langhoff added a comment -

          Dan - this is somewhat tricky for 2 reasons:

          • the hidden role assignment might deny/prohibit the capability!
          • in practice, it's tricky to "blame" a specific RA for a capability the user has.

          is that SQL from 1.8? Have a look at 1.9 to see the scope of the problem.

          Show
          Martín Langhoff added a comment - Dan - this is somewhat tricky for 2 reasons: the hidden role assignment might deny/prohibit the capability! in practice, it's tricky to "blame" a specific RA for a capability the user has. is that SQL from 1.8? Have a look at 1.9 to see the scope of the problem.
          Hide
          Dan Marsden added a comment -

          Hi Martin, yeah - I did see the complexities - I was hoping it could be easier!

          that SQL is in 1.9 when there aren't any Negative Permissions......

          there must be a way of returning whether the user got the capability via a hidden role assignment when there are negative permissions! I definately don't want to call get_users_by_capability twice!

          The other option is to force it so that users given the "mod/choice:choose" capability via a hidden role assignment just can't submit a choice. But that would likely cause confusion as well!

          Show
          Dan Marsden added a comment - Hi Martin, yeah - I did see the complexities - I was hoping it could be easier! that SQL is in 1.9 when there aren't any Negative Permissions...... there must be a way of returning whether the user got the capability via a hidden role assignment when there are negative permissions! I definately don't want to call get_users_by_capability twice! The other option is to force it so that users given the "mod/choice:choose" capability via a hidden role assignment just can't submit a choice. But that would likely cause confusion as well!
          Hide
          Martín Langhoff added a comment -

          > that SQL is in 1.9 when there aren't any Negative Permissions

          hah! I knew it - that's pretty close to the original implementation in 1.7/1.8.

          I think it can be done, but it will take significant work on get_users_by_capability() or a separate function. Maybe call it twice as a temporary measure, while we try and sort this out? I don't want to block the release on this

          Show
          Martín Langhoff added a comment - > that SQL is in 1.9 when there aren't any Negative Permissions hah! I knew it - that's pretty close to the original implementation in 1.7/1.8. I think it can be done, but it will take significant work on get_users_by_capability() or a separate function. Maybe call it twice as a temporary measure, while we try and sort this out? I don't want to block the release on this
          Hide
          Dan Marsden added a comment -

          attaching patch which calls get_users_capabilities twice, and a couple of if statements to check against the 2nd array to see if they exist.

          thanks Martin.

          Show
          Dan Marsden added a comment - attaching patch which calls get_users_capabilities twice, and a couple of if statements to check against the 2nd array to see if they exist. thanks Martin.
          Hide
          Dan Marsden added a comment -

          oops - missed a bit - here's the right one!

          Show
          Dan Marsden added a comment - oops - missed a bit - here's the right one!
          Hide
          Dan Marsden added a comment -

          fix now in 1.9 and HEAD.

          Show
          Dan Marsden added a comment - fix now in 1.9 and HEAD.

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: