Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-21042

Security overview report always displays warning for riskbackup

    Details

      Description

      Selecting 'Users > Permissions > Define roles > Administrator' from the admin block, I've set 'moodle/backup:userinfo' to Prevent, as I want to disable the option for any users of the site (including admins) to include user data in backups.

      I then checked the security overview report, and there was still a warning message displayed, despite there being no roles/overrides/users with that ability (see screenshot).

        Gliffy Diagrams

        1. MDL-21042.diff
          0.8 kB
          Paul Holden
        1. backupuserdatawarning.png
          39 kB

          Activity

          pholden Paul Holden created issue -
          pholden Paul Holden made changes -
          Field Original Value New Value
          Link This issue has been marked as being related by MDL-21043 [ MDL-21043 ]
          Hide
          pholden Paul Holden added a comment -

          Attached patch checks the number of roles returned in the report_security_check_riskbackup() function and sets $result->status accordingly.

          Show
          pholden Paul Holden added a comment - Attached patch checks the number of roles returned in the report_security_check_riskbackup() function and sets $result->status accordingly.
          pholden Paul Holden made changes -
          Attachment MDL-21042.diff [ 19388 ]
          poltawski Dan Poltawski made changes -
          Assignee moodle.com [ moodle.com ] Dan Poltawski [ poltawski ]
          poltawski Dan Poltawski made changes -
          Assignee Dan Poltawski [ poltawski ] moodle.com [ moodle.com ]
          dougiamas Martin Dougiamas made changes -
          Workflow jira [ 34380 ] MDL Workflow [ 45676 ]
          dougiamas Martin Dougiamas made changes -
          Workflow MDL Workflow [ 45676 ] MDL Full Workflow [ 74008 ]
          Hide
          salvetore Michael de Raadt added a comment -

          Thanks for reporting this issue.

          We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported.

          If you believe that this issue is still relevant to current versions (2.1 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

          Michael d;

          lqjjLKA0p6

          Show
          salvetore Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.1 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d; lqjjLKA0p6
          Hide
          pholden Paul Holden added a comment -

          Hi Michael,

          Current 2.x versions are also affected by this. I've added links to my github that fix it in master.

          Show
          pholden Paul Holden added a comment - Hi Michael, Current 2.x versions are also affected by this. I've added links to my github that fix it in master.
          pholden Paul Holden made changes -
          Pull Master Diff URL https://github.com/paulholden/moodle/compare/master...MDL-21042
          Pull Master Branch MDL-21042
          Testing Instructions Remove 'moodle/backup:userinfo' capability from all roles, etc and make sure that you don't still get a warning in security overview report
          Database MySQL [ 10001 ]
          Pull from Repository git://github.com/paulholden/moodle.git
          Hide
          salvetore Michael de Raadt added a comment -

          Thanks for reporting this issue.

          We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported.

          If you believe that this issue is still relevant to current versions (2.3 and beyond), please comment on the issue. Issues left inactive for a further month will be closed.

          Michael d;

          4d6f6f646c6521

          Show
          salvetore Michael de Raadt added a comment - Thanks for reporting this issue. We have detected that this issue has been inactive for over a year has been recorded as affecting versions that are no longer supported. If you believe that this issue is still relevant to current versions (2.3 and beyond), please comment on the issue. Issues left inactive for a further month will be closed. Michael d; 4d6f6f646c6521
          Hide
          pholden Paul Holden added a comment -

          Hi Michael,

          This issue is still present in the 'Security overview report'.

          Show
          pholden Paul Holden added a comment - Hi Michael, This issue is still present in the 'Security overview report'.
          pholden Paul Holden made changes -
          Affects Version/s 2.6.4 [ 13551 ]
          Component/s Reports [ 11833 ]
          Component/s Other [ 10063 ]
          Hide
          pholden Paul Holden added a comment -

          Michael de Raadt, I was just cleaning up a fresh Moodle 2.6.4 installation and ran the security report, only to notice that this is still present - despite the report stating that no roles/overrides/users had the capability to backup user data, there is still a "Warning".

          Show
          pholden Paul Holden added a comment - Michael de Raadt , I was just cleaning up a fresh Moodle 2.6.4 installation and ran the security report, only to notice that this is still present - despite the report stating that no roles/overrides/users had the capability to backup user data, there is still a "Warning".
          markn Mark Nelson made changes -
          Status Open [ 1 ] Waiting for peer review [ 10012 ]
          markn Mark Nelson made changes -
          Assignee moodle.com [ moodle.com ]
          Hide
          markn Mark Nelson added a comment -

          Since this issue has been around for a long time and there is a patch I am setting this to get peer reviewed. Once this review is done and approved this will need to be backported to 2.6 and 2.7. Thanks Paul for your patience.

          Show
          markn Mark Nelson added a comment - Since this issue has been around for a long time and there is a patch I am setting this to get peer reviewed. Once this review is done and approved this will need to be backported to 2.6 and 2.7. Thanks Paul for your patience.
          Hide
          markn Mark Nelson added a comment -

          Wow, your first message was over 4 years ago! Very patient indeed!

          Show
          markn Mark Nelson added a comment - Wow, your first message was over 4 years ago! Very patient indeed!
          cibot CiBoT made changes -
          Labels ci
          Hide
          cibot CiBoT added a comment -

          Fails against automated checks.

          Checked MDL-21042 using repository: git://github.com/paulholden/moodle.git

          • master (branch: MDL-21042 | CI Job)
            • Error: The MDL-21042 branch at git://github.com/paulholden/moodle.git is very old (>60 days ago). Please rebase against current master.

          More information about this report

          Show
          cibot CiBoT added a comment - Fails against automated checks. Checked MDL-21042 using repository: git://github.com/paulholden/moodle.git master (branch: MDL-21042 | CI Job ) Error: The MDL-21042 branch at git://github.com/paulholden/moodle.git is very old (>60 days ago). Please rebase against current master. More information about this report
          markn Mark Nelson made changes -
          Original Estimate 0 minutes [ 0 ]
          Remaining Estimate 0 minutes [ 0 ]
          Status Waiting for peer review [ 10012 ] Peer review in progress [ 10013 ]
          Peer reviewer Mark Nelson [ markn ]
          Hide
          markn Mark Nelson added a comment -

          Hi Paul,

          The code is great, thanks. However, you are removing the comment "there is always at least one admin" which explains why this is always going to show as a warning.

          I think there are a few approaches to this issue -

          1. Stick with your patch.
          2. Edit the description so that it explains that the administrator can always perform this operation in addition to your patch.
          3. Edit the description so that it explains that the administrator can always perform this operation without including your patch.

          I am still undecided which approach is correct. I will need to talk to other devs to see what the general consensus is.

          Show
          markn Mark Nelson added a comment - Hi Paul, The code is great, thanks. However, you are removing the comment "there is always at least one admin" which explains why this is always going to show as a warning. I think there are a few approaches to this issue - Stick with your patch. Edit the description so that it explains that the administrator can always perform this operation in addition to your patch. Edit the description so that it explains that the administrator can always perform this operation without including your patch. I am still undecided which approach is correct. I will need to talk to other devs to see what the general consensus is.
          Hide
          markn Mark Nelson added a comment -

          I thought about it and am going with option 1. I will take your commit and backport. I guess it is up to the integrator to decide.

          Show
          markn Mark Nelson added a comment - I thought about it and am going with option 1. I will take your commit and backport. I guess it is up to the integrator to decide.
          markn Mark Nelson made changes -
          Status Peer review in progress [ 10013 ] Waiting for integration review [ 10010 ]
          markn Mark Nelson made changes -
          Assignee Mark Nelson [ markn ]
          Hide
          pholden Paul Holden added a comment -

          Thanks for picking this up Mark Nelson

          Show
          pholden Paul Holden added a comment - Thanks for picking this up Mark Nelson
          marina Marina Glancy made changes -
          Labels ci ci triaged
          Hide
          cibot CiBoT added a comment -

          Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

          Show
          cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
          cibot CiBoT made changes -
          Status Waiting for integration review [ 10010 ] Waiting for integration review [ 10010 ]
          Currently in integration Yes [ 10041 ]
          cibot CiBoT made changes -
          Labels ci triaged triaged
          cibot CiBoT made changes -
          Labels triaged ci triaged
          marina Marina Glancy made changes -
          Status Waiting for integration review [ 10010 ] Integration review in progress [ 10004 ]
          Integrator Marina Glancy [ marina ]
          Hide
          marina Marina Glancy added a comment -

          thanks for working on it. I agree that suck sticky warning status does look weird.
          Interesting statement:
          if (max($usercount, $systemrolecount, $overriddenrolecount) > 0)
          I would write
          if ($usercount || $systemrolecount || $overrideenrolecount)
          but it's not some error or coding style violation so I'll proceed with integration

          Show
          marina Marina Glancy added a comment - thanks for working on it. I agree that suck sticky warning status does look weird. Interesting statement: if (max($usercount, $systemrolecount, $overriddenrolecount) > 0) I would write if ($usercount || $systemrolecount || $overrideenrolecount) but it's not some error or coding style violation so I'll proceed with integration
          Hide
          marina Marina Glancy added a comment -

          Thanks Paul, Mark, integrated in 2.6, 2.7 and master

          Show
          marina Marina Glancy added a comment - Thanks Paul, Mark, integrated in 2.6, 2.7 and master
          marina Marina Glancy made changes -
          Status Integration review in progress [ 10004 ] Waiting for testing [ 10005 ]
          Fix Version/s 2.6.5 [ 13852 ]
          Fix Version/s 2.7.2 [ 13853 ]
          rajeshtaneja Rajesh Taneja made changes -
          Tester Simey Lameze [ lameze ]
          lameze Simey Lameze made changes -
          Status Waiting for testing [ 10005 ] Testing in progress [ 10011 ]
          Hide
          lameze Simey Lameze added a comment -

          Thanks for work on this Mark, test passed.

          Show
          lameze Simey Lameze added a comment - Thanks for work on this Mark, test passed.
          lameze Simey Lameze made changes -
          Status Testing in progress [ 10011 ] Tested [ 10006 ]
          Hide
          marina Marina Glancy added a comment -

          Thank you, your change is now upstream!

          Show
          marina Marina Glancy added a comment - Thank you, your change is now upstream!
          marina Marina Glancy made changes -
          Status Tested [ 10006 ] Closed [ 6 ]
          Resolution Fixed [ 1 ]
          Currently in integration Yes [ 10041 ]
          Integration date 14/Aug/14
          Subversion JIRA

          Links Hierarchy

           Documentation

          Invalid license: EXPIRED

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                8/Sep/14