Moodle
  1. Moodle
  2. MDL-34530

Students with "managegroupentries" permission are able to create a event for any group, but cannot view it

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.3.1
    • Fix Version/s: 2.3.4
    • Component/s: Calendar
    • Labels:
    • Testing Instructions:
      Hide

      Requirements:

      To test this you will need admin permission and a test user login with a test role (e.g. teacher) you can edit freely (there are a number of capability checks in this test so best to change role definition rather than having multiple users with different capabilities).

      A course with your test user enrolled and assigned test role
      Ensure 'Force group mode' in course settings is not set
      A number of groups on the course e.g. "Group A" and "Group B"
      Test user should be a member of some (but not all) of those groups e.g. Member of "Group A"

      Common tasks:
      1. Login to system as test user (useful to do this in separate browser to admin user)
      2. Access course.
      3. Access calendar by either selecting "Go to calendar" on the Calendar block or by direct url e.g. /calendar/view.php and then selecting course from Upcoming events dropdown.
      4. Select New event button
      5 Set Type of event to Group

      Testing steps:
      1. Set the test role to have "moodle/calendar:manageentries" and "moodle/site:accessallgroups" capabilities.
      2. Undertake the tasks.
      3. The user should see all groups for the course in the Group event dropdown.
      4. Remove "moodle/site:accessallgroups" capability from the test user's role.
      5. Undertake the tasks again.
      6. The user should see only the groups they are a member of (e.g. "Group A") in the Group event dropdown
      7. Remove "moodle/calendar:manageentries" capability from the test user's role.
      8. Add "moodle/calendar:managegroupentries" capability to the test user's role.
      9. Undertake the tasks again.
      10. The user should see only the groups they are a member of (e.g. "Group A") in the Group event dropdown
      11. Add "moodle/site:accessallgroups" capability to the test user's role.
      12. Undertake the tasks again.
      13. The user should see all groups for the course in the Group event dropdown.
      14. Remove "moodle/calendar:managegroupentries" capability from the test user's role.
      15. Undertake the tasks again.
      16. Group event option should not be available.

      Show
      Requirements: To test this you will need admin permission and a test user login with a test role (e.g. teacher) you can edit freely (there are a number of capability checks in this test so best to change role definition rather than having multiple users with different capabilities). A course with your test user enrolled and assigned test role Ensure 'Force group mode' in course settings is not set A number of groups on the course e.g. "Group A" and "Group B" Test user should be a member of some (but not all) of those groups e.g. Member of "Group A" Common tasks: 1. Login to system as test user (useful to do this in separate browser to admin user) 2. Access course. 3. Access calendar by either selecting "Go to calendar" on the Calendar block or by direct url e.g. /calendar/view.php and then selecting course from Upcoming events dropdown. 4. Select New event button 5 Set Type of event to Group Testing steps: 1. Set the test role to have "moodle/calendar:manageentries" and "moodle/site:accessallgroups" capabilities. 2. Undertake the tasks. 3. The user should see all groups for the course in the Group event dropdown. 4. Remove "moodle/site:accessallgroups" capability from the test user's role. 5. Undertake the tasks again. 6. The user should see only the groups they are a member of (e.g. "Group A") in the Group event dropdown 7. Remove "moodle/calendar:manageentries" capability from the test user's role. 8. Add "moodle/calendar:managegroupentries" capability to the test user's role. 9. Undertake the tasks again. 10. The user should see only the groups they are a member of (e.g. "Group A") in the Group event dropdown 11. Add "moodle/site:accessallgroups" capability to the test user's role. 12. Undertake the tasks again. 13. The user should see all groups for the course in the Group event dropdown. 14. Remove "moodle/calendar:managegroupentries" capability from the test user's role. 15. Undertake the tasks again. 16. Group event option should not be available.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
      wip-MDL-34530_MASTER
    • Rank:
      42955

      Description

      If a student with the above mentioned permission tries to create an event in a course calendar context, he is allowed to create an event for any group. But the event shows up for him in the calendar only if it belongs to his group. So either we shouldn't allow such students to create events for other groups (what about existing ones?) or we should allow such students to view and edit group events. Either way, this needs to be fixed.

      Replication:-

      1. Allow a student permission calendar:managegroupentries
      2. As the same student create a calendar event for a group which the student doesn't belong to.
      3. Try to find that entry in the calendar monthly view.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Hmmm. This is a trucky one.

          If you are going to give a student moodle/calendar:managegroupentries and not moodle/site:accessallgroups, then they should probably not see the groups they are not a part of, and that would include the time when they are creating calendar events.

          Show
          Michael de Raadt added a comment - Hmmm. This is a trucky one. If you are going to give a student moodle/calendar:managegroupentries and not moodle/site:accessallgroups, then they should probably not see the groups they are not a part of, and that would include the time when they are creating calendar events.
          Hide
          Michael de Raadt added a comment -

          There has been a slight change in behaviour with MDL-34255. Now an error is produced when a student tries to create a group event for a group they are not part of.

          I think students should only be able to create events for groups they are in, unless they have the accessallgroups permission, in which case they should be able to create group events for other groups and see the events for other groups.

          Show
          Michael de Raadt added a comment - There has been a slight change in behaviour with MDL-34255 . Now an error is produced when a student tries to create a group event for a group they are not part of. I think students should only be able to create events for groups they are in, unless they have the accessallgroups permission, in which case they should be able to create group events for other groups and see the events for other groups.
          Hide
          Michael de Raadt added a comment -

          I'm increasing the priority on this as the changes in MDL-34255 now make it worse.

          Show
          Michael de Raadt added a comment - I'm increasing the priority on this as the changes in MDL-34255 now make it worse.
          Hide
          Jason Platts added a comment -

          Having exactly this problem (users with permission to create group events can create for all groups on course, not just ones they are in).

          Is there any progress on development?

          My quick and dirty suggestion would be to update call to groups_get_all_groups() in calendar_get_allowed_types() (calendar/lib.php line 1728 in 2.3.2) so that for those only with moodle/calendar:managegroupentries capability the extra userid parameter is added.

          The only downside of this suggestion is that it is a bit simplistic as groups_get_all_groups() does not take into consideration accessallgroups capability. So essentially you are saying if you can manage all calendar entries on a course then you should have access to all groups and if you only have access to manage group events you won't have accessallgroups...
          Otherwise you could call groups_get_all_groups() differently (with/without user id) in both places in calendar_get_allowed_types() depending on accessallgroups capability.

          Can code up one of those for peer review if anyone is interested?

          Show
          Jason Platts added a comment - Having exactly this problem (users with permission to create group events can create for all groups on course, not just ones they are in). Is there any progress on development? My quick and dirty suggestion would be to update call to groups_get_all_groups() in calendar_get_allowed_types() (calendar/lib.php line 1728 in 2.3.2) so that for those only with moodle/calendar:managegroupentries capability the extra userid parameter is added. The only downside of this suggestion is that it is a bit simplistic as groups_get_all_groups() does not take into consideration accessallgroups capability. So essentially you are saying if you can manage all calendar entries on a course then you should have access to all groups and if you only have access to manage group events you won't have accessallgroups... Otherwise you could call groups_get_all_groups() differently (with/without user id) in both places in calendar_get_allowed_types() depending on accessallgroups capability. Can code up one of those for peer review if anyone is interested?
          Hide
          Jason Platts added a comment -

          Assigned to me to submit for peer review.

          Have made change so that groups shown is dependant on accessallgroups capability.

          Show
          Jason Platts added a comment - Assigned to me to submit for peer review. Have made change so that groups shown is dependant on accessallgroups capability.
          Hide
          Frédéric Massart added a comment -

          Assigning Mark as the peer reviewer. I believe you were not focused when you entered my name in that field . Cheers!

          Show
          Frédéric Massart added a comment - Assigning Mark as the peer reviewer. I believe you were not focused when you entered my name in that field . Cheers!
          Hide
          Mark Nelson added a comment -

          Hi Jason. Your code looks good. However could you add some testing instructions before submitting to integration (not sure if you have the ability to, so let me know when it is done and I will submit for integration for you), thanks.

          Show
          Mark Nelson added a comment - Hi Jason. Your code looks good. However could you add some testing instructions before submitting to integration (not sure if you have the ability to, so let me know when it is done and I will submit for integration for you), thanks.
          Hide
          Jason Platts added a comment -

          Adding test instructions.

          Show
          Jason Platts added a comment - Adding test instructions.
          Hide
          Sam Hemelryk added a comment -

          Thanks guys, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks guys, this has been integrated now
          Hide
          David Monllaó added a comment -

          Tested in 23 and master. All works as expected

          Show
          David Monllaó added a comment - Tested in 23 and master. All works as expected
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Y E S !

          Closing as fixed, many thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Y E S ! Closing as fixed, many thanks!

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: