From a63cd3e2ca8f864a832c5e050677acbf1bda1701 Mon Sep 17 00:00:00 2001
From: Andrew Hancox <andrewdchancox@googlemail.com>
Date: Wed, 15 Nov 2017 15:08:10 +0000
Subject: [PATCH 1/3] MDL-50666 core: Add function get_viewable_roles to set
 role visibility

---
 admin/roles/allow.php                         |   6 +-
 admin/roles/classes/allow_view_page.php       |  77 +++++++++++++
 .../classes/define_role_table_advanced.php    |  35 ++++--
 admin/roles/classes/preset.php                |   4 +-
 admin/roles/classes/preset_form.php           |   1 +
 admin/roles/define.php                        |   6 +-
 admin/roles/managetabs.php                    |   1 +
 admin/roles/role_schema.xml                   |   8 ++
 admin/roles/tests/preset_test.php             |   2 +-
 badges/criteria/award_criteria_manual.php     |   4 +
 badges/tests/behat/role_visibility.feature    |  51 ++++++++
 enrol/locallib.php                            |  25 +++-
 enrol/tests/behat/role_visibility.feature     |  38 ++++++
 enrol/users_forms.php                         |   6 +-
 group/lib.php                                 |   6 +-
 group/tests/behat/role_visibility.feature     |  53 +++++++++
 lang/en/admin.php                             |   1 +
 lang/en/role.php                              |   4 +
 lib/accesslib.php                             |  90 ++++++++++++++-
 lib/classes/event/role_allow_view_updated.php |  82 +++++++++++++
 lib/db/install.php                            |   2 +-
 lib/db/install.xml                            |  14 +++
 lib/db/upgrade.php                            |  43 +++++++
 lib/statslib.php                              |   1 +
 lib/testing/generator/data_generator.php      |   4 +-
 lib/tests/accesslib_test.php                  | 109 ++++++++++++++++++
 lib/upgrade.txt                               |   3 +
 report/stats/locallib.php                     |   8 +-
 user/classes/output/user_roles_editable.php   |  22 +++-
 user/classes/participants_table.php           |   7 +-
 user/renderer.php                             |   7 +-
 version.php                                   |   2 +-
 32 files changed, 685 insertions(+), 37 deletions(-)
 create mode 100644 admin/roles/classes/allow_view_page.php
 create mode 100644 badges/tests/behat/role_visibility.feature
 create mode 100644 enrol/tests/behat/role_visibility.feature
 create mode 100644 group/tests/behat/role_visibility.feature
 create mode 100644 lib/classes/event/role_allow_view_updated.php

diff --git a/admin/roles/allow.php b/admin/roles/allow.php
index 916e6c2b0c20e..71e9bcb3d0ad7 100644
--- a/admin/roles/allow.php
+++ b/admin/roles/allow.php
@@ -29,7 +29,8 @@
 $classformode = array(
     'assign' => 'core_role_allow_assign_page',
     'override' => 'core_role_allow_override_page',
-    'switch' => 'core_role_allow_switch_page'
+    'switch' => 'core_role_allow_switch_page',
+    'view' => 'core_role_allow_view_page'
 );
 if (!isset($classformode[$mode])) {
     print_error('invalidmode', '', '', $mode);
@@ -58,6 +59,9 @@
         case 'switch':
             $event = \core\event\role_allow_switch_updated::create(array('context' => $syscontext));
             break;
+        case 'view':
+            $event = \core\event\role_allow_view_updated::create(array('context' => $syscontext));
+            break;
     }
     if ($event) {
         $event->trigger();
diff --git a/admin/roles/classes/allow_view_page.php b/admin/roles/classes/allow_view_page.php
new file mode 100644
index 0000000000000..2899fc631b5d6
--- /dev/null
+++ b/admin/roles/classes/allow_view_page.php
@@ -0,0 +1,77 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+/**
+ * Role view matrix.
+ *
+ * @package    core_role
+ * @copyright  2016 onwards Andrew Hancox <andrewdchancox@googlemail.com>
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+
+defined('MOODLE_INTERNAL') || die();
+
+/**
+ * Subclass of role_allow_role_page for the Allow views tab.
+ *
+ * @package    core_role
+ * @copyright  2016 onwards Andrew Hancox <andrewdchancox@googlemail.com>
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+class core_role_allow_view_page extends core_role_allow_role_page {
+    /** @var array */
+    protected $allowedtargetroles;
+
+    /**
+     * core_role_allow_view_page constructor.
+     */
+    public function __construct() {
+        parent::__construct('role_allow_view', 'allowview');
+    }
+
+
+    /**
+     * Allow from role to view target role.
+     * @param int $fromroleid
+     * @param int $targetroleid
+     */
+    protected function set_allow($fromroleid, $targetroleid) {
+        allow_view($fromroleid, $targetroleid);
+    }
+
+    /**
+     * Get tool tip for cell.
+     * @param string $fromrole
+     * @param string $targetrole
+     * @return string
+     * @throws \coding_exception
+     */
+    protected function get_cell_tooltip($fromrole, $targetrole) {
+        $a = new stdClass;
+        $a->fromrole = $fromrole->localname;
+        $a->targetrole = $targetrole->localname;
+        return get_string('allowroletoview', 'core_role', $a);
+    }
+
+    /**
+     * Get intro text for role allow view page.
+     * @return string
+     * @throws \coding_exception
+     */
+    public function get_intro_text() {
+        return get_string('configallowview', 'core_admin');
+    }
+}
diff --git a/admin/roles/classes/define_role_table_advanced.php b/admin/roles/classes/define_role_table_advanced.php
index bcf6a495d1f59..36cce176d4fa9 100644
--- a/admin/roles/classes/define_role_table_advanced.php
+++ b/admin/roles/classes/define_role_table_advanced.php
@@ -42,6 +42,7 @@ class core_role_define_role_table_advanced extends core_role_capability_table_wi
     protected $allowassign;
     protected $allowoverride;
     protected $allowswitch;
+    protected $allowview;
 
     public function __construct($context, $roleid) {
         $this->roleid = $roleid;
@@ -72,6 +73,7 @@ protected function load_current_permissions() {
             $this->allowassign = array_keys($this->get_allow_roles_list('assign'));
             $this->allowoverride = array_keys($this->get_allow_roles_list('override'));
             $this->allowswitch = array_keys($this->get_allow_roles_list('switch'));
+            $this->allowview = array_keys($this->get_allow_roles_list('view'));
 
         } else {
             $this->role = new stdClass;
@@ -83,6 +85,7 @@ protected function load_current_permissions() {
             $this->allowassign = array();
             $this->allowoverride = array();
             $this->allowswitch = array();
+            $this->allowview = array();
         }
         parent::load_current_permissions();
     }
@@ -162,6 +165,10 @@ public function read_submitted_permissions() {
         if (!is_null($allow)) {
             $this->allowswitch = $allow;
         }
+        $allow = optional_param_array('allowview', null, PARAM_INT);
+        if (!is_null($allow)) {
+            $this->allowview = $allow;
+        }
 
         // Now read the permissions for each capability.
         parent::read_submitted_permissions();
@@ -178,7 +185,8 @@ public function is_submission_valid() {
      * @param int $roleid role id or 0 for no role
      * @param array $options array with following keys:
      *      'name', 'shortname', 'description', 'permissions', 'archetype',
-     *      'contextlevels', 'allowassign', 'allowoverride', 'allowswitch'
+     *      'contextlevels', 'allowassign', 'allowoverride', 'allowswitch',
+     *      'allowview'
      */
     public function force_duplicate($roleid, array $options) {
         global $DB;
@@ -215,6 +223,9 @@ public function force_duplicate($roleid, array $options) {
             if ($options['allowswitch']) {
                 $this->allowswitch = array();
             }
+            if ($options['allowview']) {
+                $this->allowview = array();
+            }
 
             if ($options['permissions']) {
                 foreach ($this->capabilities as $capid => $cap) {
@@ -260,6 +271,9 @@ public function force_duplicate($roleid, array $options) {
         if ($options['allowswitch']) {
             $this->allowswitch = array_keys($this->get_allow_roles_list('switch', $roleid));
         }
+        if ($options['allowview']) {
+            $this->allowview = array_keys($this->get_allow_roles_list('view', $roleid));
+        }
 
         if ($options['permissions']) {
             $this->permissions = $DB->get_records_menu('role_capabilities',
@@ -280,7 +294,8 @@ public function force_duplicate($roleid, array $options) {
      * @param string $archetype
      * @param array $options array with following keys:
      *      'name', 'shortname', 'description', 'permissions', 'archetype',
-     *      'contextlevels', 'allowassign', 'allowoverride', 'allowswitch'
+     *      'contextlevels', 'allowassign', 'allowoverride', 'allowswitch',
+     *      'allowview'
      */
     public function force_archetype($archetype, array $options) {
         $archetypes = get_role_archetypes();
@@ -321,6 +336,9 @@ public function force_archetype($archetype, array $options) {
         if ($options['allowswitch']) {
             $this->allowswitch = get_default_role_archetype_allows('switch', $archetype);
         }
+        if ($options['allowview']) {
+            $this->allowview = get_default_role_archetype_allows('view', $archetype);
+        }
 
         if ($options['permissions']) {
             $defaultpermissions = get_default_capabilities($archetype);
@@ -340,7 +358,8 @@ public function force_archetype($archetype, array $options) {
      * @param string $xml
      * @param array $options array with following keys:
      *      'name', 'shortname', 'description', 'permissions', 'archetype',
-     *      'contextlevels', 'allowassign', 'allowoverride', 'allowswitch'
+     *      'contextlevels', 'allowassign', 'allowoverride', 'allowswitch',
+     *      'allowview'
      */
     public function force_preset($xml, array $options) {
         if (!$info = core_role_preset::parse_preset($xml)) {
@@ -377,7 +396,7 @@ public function force_preset($xml, array $options) {
             }
         }
 
-        foreach (array('assign', 'override', 'switch') as $type) {
+        foreach (array('assign', 'override', 'switch', 'view') as $type) {
             if ($options['allow'.$type]) {
                 if (isset($info['allow'.$type])) {
                     $this->{'allow'.$type} = $info['allow'.$type];
@@ -438,6 +457,7 @@ public function save_changes() {
         $this->save_allow('assign');
         $this->save_allow('override');
         $this->save_allow('switch');
+        $this->save_allow('view');
 
         // Permissions.
         parent::save_changes();
@@ -523,7 +543,7 @@ protected function get_assignable_levels_control() {
     protected function get_allow_roles_list($type, $roleid = null) {
         global $DB;
 
-        if ($type !== 'assign' and $type !== 'switch' and $type !== 'override') {
+        if ($type !== 'assign' and $type !== 'switch' and $type !== 'override' and $type !== 'view') {
             debugging('Invalid role allowed type specified', DEBUG_DEVELOPER);
             return array();
         }
@@ -547,11 +567,11 @@ protected function get_allow_roles_list($type, $roleid = null) {
     /**
      * Returns an array of roles with the allowed type.
      *
-     * @param string $type Must be one of: assign, switch, or override.
+     * @param string $type Must be one of: assign, switch, override or view.
      * @return array Am array of role names with the allowed type
      */
     protected function get_allow_role_control($type) {
-        if ($type !== 'assign' and $type !== 'switch' and $type !== 'override') {
+        if ($type !== 'assign' and $type !== 'switch' and $type !== 'override' and $type !== 'view') {
             debugging('Invalid role allowed type specified', DEBUG_DEVELOPER);
             return '';
         }
@@ -641,6 +661,7 @@ public function display() {
         $this->print_field('menuallowassign', get_string('allowassign', 'core_role'), $this->get_allow_role_control('assign'));
         $this->print_field('menuallowoverride', get_string('allowoverride', 'core_role'), $this->get_allow_role_control('override'));
         $this->print_field('menuallowswitch', get_string('allowswitch', 'core_role'), $this->get_allow_role_control('switch'));
+        $this->print_field('menuallowview', get_string('allowview', 'core_role'), $this->get_allow_role_control('view'));
         if ($risks = $this->get_role_risks_info()) {
             $this->print_field('', get_string('rolerisks', 'core_role'), $risks);
         }
diff --git a/admin/roles/classes/preset.php b/admin/roles/classes/preset.php
index 256f635d5ba84..b99434255ece6 100644
--- a/admin/roles/classes/preset.php
+++ b/admin/roles/classes/preset.php
@@ -84,7 +84,7 @@ public static function get_export_xml($roleid) {
             $contextlevels->appendChild($dom->createElement('level', $name));
         }
 
-        foreach (array('assign', 'override', 'switch') as $type) {
+        foreach (array('assign', 'override', 'switch', 'view') as $type) {
             $allows = $dom->createElement('allow'.$type);
             $top->appendChild($allows);
             $records = $DB->get_records('role_allow_'.$type, array('roleid'=>$roleid), "allow$type ASC");
@@ -205,7 +205,7 @@ public static function parse_preset($xml) {
             }
         }
 
-        foreach (array('assign', 'override', 'switch') as $type) {
+        foreach (array('assign', 'override', 'switch', 'view') as $type) {
             $values = self::get_node_children_values($dom, '/role/allow'.$type, 'shortname');
             if (!isset($values)) {
                 $info['allow'.$type] = null;
diff --git a/admin/roles/classes/preset_form.php b/admin/roles/classes/preset_form.php
index ff2024d7d9c11..426c3d8d38fc9 100644
--- a/admin/roles/classes/preset_form.php
+++ b/admin/roles/classes/preset_form.php
@@ -79,6 +79,7 @@ protected function definition() {
             $mform->addElement('advcheckbox', 'allowassign', get_string('allowassign', 'core_role'));
             $mform->addElement('advcheckbox', 'allowoverride', get_string('allowoverride', 'core_role'));
             $mform->addElement('advcheckbox', 'allowswitch', get_string('allowswitch', 'core_role'));
+            $mform->addElement('advcheckbox', 'allowview', get_string('allowview', 'core_role'));
             $mform->addElement('advcheckbox', 'permissions', get_string('permissions', 'core_role'));
         }
 
diff --git a/admin/roles/define.php b/admin/roles/define.php
index d670bae45d85b..de3eac1b1c462 100644
--- a/admin/roles/define.php
+++ b/admin/roles/define.php
@@ -103,7 +103,8 @@
             'contextlevels' => 1,
             'allowassign'   => 1,
             'allowoverride' => 1,
-            'allowswitch'   => 1);
+            'allowswitch'   => 1,
+            'allowview'   => 1);
         if ($showadvanced) {
             $definitiontable = new core_role_define_role_table_advanced($systemcontext, 0);
         } else {
@@ -150,7 +151,8 @@
             'contextlevels' => $data->contextlevels,
             'allowassign'   => $data->allowassign,
             'allowoverride' => $data->allowoverride,
-            'allowswitch'   => $data->allowswitch);
+            'allowswitch'   => $data->allowswitch,
+            'allowview'     => $data->allowview);
         if ($showadvanced) {
             $definitiontable = new core_role_define_role_table_advanced($systemcontext, $roleid);
         } else {
diff --git a/admin/roles/managetabs.php b/admin/roles/managetabs.php
index 78f9f7532dbdd..a9a8385a13d89 100644
--- a/admin/roles/managetabs.php
+++ b/admin/roles/managetabs.php
@@ -29,6 +29,7 @@
 $toprow[] = new tabobject('assign', new moodle_url('/admin/roles/allow.php', array('mode'=>'assign')), get_string('allowassign', 'core_role'));
 $toprow[] = new tabobject('override', new moodle_url('/admin/roles/allow.php', array('mode'=>'override')), get_string('allowoverride', 'core_role'));
 $toprow[] = new tabobject('switch', new moodle_url('/admin/roles/allow.php', array('mode'=>'switch')), get_string('allowswitch', 'core_role'));
+$toprow[] = new tabobject('view', new moodle_url('/admin/roles/allow.php', ['mode' => 'view']), get_string('allowview', 'core_role'));
 
 echo $OUTPUT->tabtree($toprow, $currenttab);
 
diff --git a/admin/roles/role_schema.xml b/admin/roles/role_schema.xml
index f0a33c9ecc0bc..bb3de8976b7a3 100644
--- a/admin/roles/role_schema.xml
+++ b/admin/roles/role_schema.xml
@@ -11,6 +11,7 @@
                 <xs:element ref="allowassign" minOccurs="0"/>
                 <xs:element ref="allowoverride" minOccurs="0"/>
                 <xs:element ref="allowswitch" minOccurs="0"/>
+                <xs:element ref="allowview" minOccurs="0"/>
                 <xs:element ref="permissions" minOccurs="0"/>
             </xs:sequence>
         </xs:complexType>
@@ -45,6 +46,13 @@
             </xs:sequence>
         </xs:complexType>
     </xs:element>
+    <xs:element name="allowview">
+        <xs:complexType>
+            <xs:sequence>
+                <xs:element maxOccurs="unbounded" minOccurs="0" ref="shortname"/>
+            </xs:sequence>
+        </xs:complexType>
+    </xs:element>
     <xs:element name="permissions">
         <xs:complexType>
             <xs:sequence>
diff --git a/admin/roles/tests/preset_test.php b/admin/roles/tests/preset_test.php
index 0899480031412..4ab0ebc35ed65 100644
--- a/admin/roles/tests/preset_test.php
+++ b/admin/roles/tests/preset_test.php
@@ -44,7 +44,7 @@ public function test_xml() {
             $contextlevels = get_role_contextlevels($role->id);
             $this->assertEquals(array_values($contextlevels), array_values($info['contextlevels']));
 
-            foreach (array('assign', 'override', 'switch') as $type) {
+            foreach (array('assign', 'override', 'switch', 'view') as $type) {
                 $records = $DB->get_records('role_allow_'.$type, array('roleid'=>$role->id), "allow$type ASC");
                 $allows = array();
                 foreach ($records as $record) {
diff --git a/badges/criteria/award_criteria_manual.php b/badges/criteria/award_criteria_manual.php
index 83fde27038a5a..ff1e613d6d98a 100644
--- a/badges/criteria/award_criteria_manual.php
+++ b/badges/criteria/award_criteria_manual.php
@@ -65,6 +65,7 @@ public function get_options(&$mform) {
         $none = true;
 
         $roles = get_roles_with_capability('moodle/badges:awardbadge', CAP_ALLOW, $PAGE->context);
+        $visibleroles = get_viewable_roles($PAGE->context);
         $roleids = array_map(function($o) {
             return $o->id;
         }, $roles);
@@ -89,6 +90,9 @@ public function get_options(&$mform) {
             $mform->addElement('header', 'first_header', $this->get_title());
             $mform->addHelpButton('first_header', 'criteria_' . $this->criteriatype, 'badges');
             foreach ($roleids as $rid) {
+                if (!key_exists($rid, $visibleroles)) {
+                    continue;
+                }
                 $checked = false;
                 if (in_array($rid, $existing)) {
                     $checked = true;
diff --git a/badges/tests/behat/role_visibility.feature b/badges/tests/behat/role_visibility.feature
new file mode 100644
index 0000000000000..5bb0a73543b5a
--- /dev/null
+++ b/badges/tests/behat/role_visibility.feature
@@ -0,0 +1,51 @@
+@core @core_badges @role_visibility
+Feature: Test role visibility
+  In order to control access
+  As an admin
+  I need to control which roles can see each other
+
+  Background: Add a bunch of users
+    Given  the following "courses" exist:
+      | fullname | shortname |
+      | Course 1 | C1        |
+    And the following "users" exist:
+      | username | firstname | lastname | email                |
+      | teacher1 | Teacher   | 1        | teacher1@example.com |
+      | manager1 | Manager   | 1        | manager1@example.com |
+    And the following "course enrolments" exist:
+      | user     | course | role    |
+      | teacher1 | C1     | editingteacher |
+      | manager1 | C1     | manager        |
+
+  @javascript @_file_upload
+  Scenario: Check the default roles are visible
+    Given I log in as "manager1"
+    And I am on "Course 1" course homepage
+    And I navigate to "Add a new badge" node in "Course administration > Badges"
+    And I follow "Add a new badge"
+    And I set the following fields to these values:
+      | Name | Course Badge |
+      | Description | Course badge description |
+      | issuername | Tester of course badge |
+    And I upload "badges/tests/behat/badge.png" file to "Image" filemanager
+    And I press "Create badge"
+    And I set the field "type" to "Manual issue by role"
+    Then I should see "Teacher"
+    And I should see "Manager"
+
+  @javascript @_file_upload
+  Scenario: Check hidden roles are not visible
+    Given I log in as "teacher1"
+    And I am on "Course 1" course homepage
+    And I navigate to "Add a new badge" node in "Course administration > Badges"
+    And I follow "Add a new badge"
+    And I set the following fields to these values:
+      | Name | Course Badge |
+      | Description | Course badge description |
+      | issuername | Tester of course badge |
+    And I upload "badges/tests/behat/badge.png" file to "Image" filemanager
+    And I press "Create badge"
+    And I set the following fields to these values:
+      | Add badge criteria | Manual issue by role |
+    Then I should see "Teacher"
+    And I should not see "Manager"
\ No newline at end of file
diff --git a/enrol/locallib.php b/enrol/locallib.php
index 75564193163b4..0447951ff4feb 100644
--- a/enrol/locallib.php
+++ b/enrol/locallib.php
@@ -117,6 +117,7 @@ class course_enrolment_manager {
     private $_plugins = null;
     private $_allplugins = null;
     private $_roles = null;
+    private $_visibleroles = null;
     private $_assignableroles = null;
     private $_assignablerolesothers = null;
     private $_groups = null;
@@ -593,6 +594,18 @@ public function get_all_roles() {
         return $this->_roles;
     }
 
+    /**
+     * Gets all of the roles this course can contain.
+     *
+     * @return array
+     */
+    public function get_viewable_roles() {
+        if ($this->_visibleroles === null) {
+            $this->_visibleroles = get_viewable_roles($this->context);
+        }
+        return $this->_visibleroles;
+    }
+
     /**
      * Gets all of the assignable roles for this course.
      *
@@ -1032,7 +1045,7 @@ public function get_users_for_display(course_enrolment_manager $manager, $sort,
         $strunenrol = get_string('unenrol', 'enrol');
         $stredit = get_string('edit');
 
-        $allroles   = $this->get_all_roles();
+        $visibleroles   = $this->get_viewable_roles();
         $assignable = $this->get_assignable_roles();
         $allgroups  = $this->get_all_groups();
         $context    = $this->get_context();
@@ -1054,7 +1067,15 @@ public function get_users_for_display(course_enrolment_manager $manager, $sort,
                 if (!is_siteadmin() and !isset($assignable[$rid])) {
                     $unchangeable = true;
                 }
-                $details['roles'][$rid] = array('text'=>$allroles[$rid]->localname, 'unchangeable'=>$unchangeable);
+
+                if (isset($visibleroles[$rid])) {
+                    $label = $visibleroles[$rid];
+                } else {
+                    $label = get_string('novisibleroles', 'role');
+                    $unchangeable = true;
+                }
+
+                $details['roles'][$rid] = array('text' => $label, 'unchangeable' => $unchangeable);
             }
 
             // Users
diff --git a/enrol/tests/behat/role_visibility.feature b/enrol/tests/behat/role_visibility.feature
new file mode 100644
index 0000000000000..8a482b7443c73
--- /dev/null
+++ b/enrol/tests/behat/role_visibility.feature
@@ -0,0 +1,38 @@
+@core @core_enrol @role_visibility
+Feature: Test role visibility
+  In order to control access
+  As an admin
+  I need to control which roles can see each other
+
+  Background: Add a bunch of users
+    Given  the following "courses" exist:
+      | fullname | shortname |
+      | Course 1 | C1        |
+    And the following "users" exist:
+      | username | firstname | lastname | email                |
+      | learner1 | Learner   | 1        | learner1@example.com |
+      | teacher1 | Teacher   | 1        | teacher1@example.com |
+      | manager1 | Manager   | 1        | manager1@example.com |
+    And the following "course enrolments" exist:
+      | user     | course | role           |
+      | learner1 | C1     | student        |
+      | teacher1 | C1     | editingteacher |
+      | manager1 | C1     | manager        |
+
+  Scenario: Check the default roles are visible
+    Given I log in as "manager1"
+    And I follow "Course 1"
+    When I navigate to "Enrolled users" node in "Course administration > Users"
+    Then "Learner 1" row "Roles" column of "participants" table should contain "Student"
+    And "Teacher 1" row "Roles" column of "participants" table should contain "Teacher"
+    And "Manager 1" row "Roles" column of "participants" table should contain "Manager"
+    And I should not see "No Roles" in the "table#participants" "css_element"
+
+  Scenario: Do not allow managers to view any roles but manager and check they are hidden
+    Given I log in as "teacher1"
+    And I am on "Course 1" course homepage
+    When I navigate to "Enrolled users" node in "Course administration > Users"
+    Then "Learner 1" row "Roles" column of "participants" table should contain "Student"
+    And "Teacher 1" row "Roles" column of "participants" table should contain "Teacher"
+    And "Manager 1" row "Roles" column of "participants" table should not contain "Manager"
+    And "Manager 1" row "Roles" column of "participants" table should contain "No roles"
\ No newline at end of file
diff --git a/enrol/users_forms.php b/enrol/users_forms.php
index 97b62b7365503..aa3ada707b9fc 100644
--- a/enrol/users_forms.php
+++ b/enrol/users_forms.php
@@ -156,10 +156,10 @@ function definition() {
         // names if applied. The reason for not restricting to roles that can
         // be assigned at course level is that upper-level roles display in the
         // enrolments table so it makes sense to let users filter by them.
-        $allroles = $manager->get_all_roles();
+        $visibleroles = $manager->get_viewable_roles();
         $rolenames = array();
-        foreach ($allroles as $id => $role) {
-            $rolenames[$id] = $role->localname;
+        foreach ($visibleroles as $id => $role) {
+            $rolenames[$id] = $role;
         }
         $mform->addElement('select', 'role', get_string('role'),
                 array(0 => get_string('all')) + $rolenames);
diff --git a/group/lib.php b/group/lib.php
index 57bd61610dacd..ca774c1704ceb 100644
--- a/group/lib.php
+++ b/group/lib.php
@@ -978,6 +978,7 @@ function groups_calculate_role_people($rs, $context) {
     }
 
     $allroles = role_fix_names(get_all_roles($context), $context);
+    $visibleroles = get_viewable_roles($context);
 
     // Array of all involved roles
     $roles = array();
@@ -1036,14 +1037,15 @@ function groups_calculate_role_people($rs, $context) {
 
     // Now we rearrange the data to store users by role
     foreach ($users as $userid=>$userdata) {
-        $rolecount = count($userdata->roles);
+        $visibleuserroles = array_intersect_key($userdata->roles, $visibleroles);
+        $rolecount = count($visibleuserroles);
         if ($rolecount == 0) {
             // does not have any roles
             $roleid = 0;
         } else if($rolecount > 1) {
             $roleid = '*';
         } else {
-            $userrole = reset($userdata->roles);
+            $userrole = reset($visibleuserroles);
             $roleid = $userrole->id;
         }
         $roles[$roleid]->users[$userid] = $userdata;
diff --git a/group/tests/behat/role_visibility.feature b/group/tests/behat/role_visibility.feature
new file mode 100644
index 0000000000000..f21d2acd27b32
--- /dev/null
+++ b/group/tests/behat/role_visibility.feature
@@ -0,0 +1,53 @@
+@core @core_group @role_visibility
+Feature: Test role visibility
+  In order to control access
+  As an admin
+  I need to control which roles can see each other
+
+  Background: Set up some groups
+    Given  the following "courses" exist:
+      | fullname | shortname |
+      | Course 1 | C1        |
+    And the following "users" exist:
+      | username | firstname | lastname | email                |
+      | learner1 | Learner   | 1        | learner1@example.com |
+      | teacher1 | Teacher   | 1        | teacher1@example.com |
+      | manager1 | Manager   | 1        | manager1@example.com |
+    And the following "course enrolments" exist:
+      | user     | course | role           |
+      | learner1 | C1     | student        |
+      | teacher1 | C1     | editingteacher |
+      | manager1 | C1     | manager        |
+    And the following "groups" exist:
+      | name    | course | idnumber |
+      | Group 1 | C1     | G1       |
+    And the following "group members" exist:
+      | user     | group |
+      | learner1 | G1    |
+      | teacher1 | G1    |
+      | manager1 | G1    |
+
+  @javascript
+  Scenario: Check the default roles are visible
+    Given I log in as "manager1"
+    And I am on "Course 1" course homepage
+    And I navigate to "Groups" node in "Course administration > Users"
+    When I set the field "groups" to "Group 1 (3)"
+    And I press "Show members for group"
+    Then "optgroup[label='No roles']" "css_element" should not exist in the "#members" "css_element"
+    And "optgroup[label='Student']" "css_element" should exist in the "#members" "css_element"
+    And "optgroup[label='Teacher']" "css_element" should exist in the "#members" "css_element"
+    And "optgroup[label='Manager']" "css_element" should exist in the "#members" "css_element"
+    And I log out
+
+  @javascript
+  Scenario: Do not allow managers to view any roles and check they are hidden
+    Given I log in as "teacher1"
+    And I am on "Course 1" course homepage
+    And I navigate to "Groups" node in "Course administration > Users"
+    When I set the field "groups" to "Group 1 (3)"
+    Then "optgroup[label='No roles']" "css_element" should exist in the "#members" "css_element"
+    And "optgroup[label='Student']" "css_element" should exist in the "#members" "css_element"
+    And "optgroup[label='Teacher']" "css_element" should exist in the "#members" "css_element"
+    And "optgroup[label='Manager']" "css_element" should not exist in the "#members" "css_element"
+    And I log out
\ No newline at end of file
diff --git a/lang/en/admin.php b/lang/en/admin.php
index 69291ba062148..01ee8b7e728de 100644
--- a/lang/en/admin.php
+++ b/lang/en/admin.php
@@ -154,6 +154,7 @@
 $string['configallowuserswitchrolestheycantassign'] = 'By default, moodle/role:assign is required for users to switch roles. Enabling this setting removes this requirement, and results in the roles available in the "Switch role to" dropdown menu being determined by settings in the "Allow role assignments" table only.
 It is recommended that the settings in the "Allow role assignments" table do not allow users to switch to a role with more capabilities than their existing role.';
 $string['configallowuserthemes'] = 'If you enable this, then users will be allowed to set their own themes.  User themes override site themes (but not course themes)';
+$string['configallowview'] = 'Select which roles a user will see, be able to filter by etc. based on which roles they already have.';
 $string['configallusersaresitestudents'] = 'For activities on the front page of the site, should ALL users be considered as students?  If you answer "Yes", then any confirmed user account will be allowed to participate as a student in those activities.  If you answer "No", then only users who are already a participant in at least one course will be able to take part in those front page activities. Only admins and specially assigned teachers can act as teachers for these front page activities.';
 $string['configauthenticationplugins'] = 'Please choose the authentication plugins you wish to use and arrange them in order of failthrough.';
 $string['configautolang'] = 'Detect default language from browser setting, if disabled site default is used.';
diff --git a/lang/en/role.php b/lang/en/role.php
index f1052255f4897..8c8072ec82ef7 100644
--- a/lang/en/role.php
+++ b/lang/en/role.php
@@ -32,7 +32,9 @@
 $string['allowroletoassign'] = 'Allow users with role {$a->fromrole} to assign the role {$a->targetrole}';
 $string['allowroletooverride'] = 'Allow users with role {$a->fromrole} to override the role {$a->targetrole}';
 $string['allowroletoswitch'] = 'Allow users with role {$a->fromrole} to switch roles to the role {$a->targetrole}';
+$string['allowroletoview'] = 'Allow users with role {$a->fromrole} to view the role {$a->targetrole}';
 $string['allowswitch'] = 'Allow role switches';
+$string['allowview'] = 'Allow role to view';
 $string['allsiteusers'] = 'All site users';
 $string['analytics:listinsights'] = 'List insights';
 $string['analytics:managemodels'] = 'Manage models';
@@ -219,6 +221,7 @@
 $string['eventroleallowassignupdated'] = 'Allow role assignment';
 $string['eventroleallowoverrideupdated'] = 'Allow role override';
 $string['eventroleallowswitchupdated'] = 'Allow role switch';
+$string['eventroleallowviewupdated'] = 'Allow role view';
 $string['eventroleassigned'] = 'Role assigned';
 $string['eventrolecapabilitiesupdated'] = 'Role capabilities updated';
 $string['eventroledeleted'] = 'Role deleted';
@@ -292,6 +295,7 @@
 $string['norole'] = 'No role';
 $string['noroles'] = 'No roles';
 $string['noroleassignments'] = 'This user does not have any role assignments anywhere in this site.';
+$string['novisibleroles'] = 'No roles';
 $string['notabletoassignroleshere'] = 'Assigning of roles in this context has not been enabled by an administrator.';
 $string['notabletooverrideroleshere'] = 'You are not able to override the permissions on any roles here';
 $string['notes:manage'] = 'Manage notes';
diff --git a/lib/accesslib.php b/lib/accesslib.php
index 856a80924b906..a9f1d19cfe24c 100644
--- a/lib/accesslib.php
+++ b/lib/accesslib.php
@@ -1984,7 +1984,7 @@ function get_default_capabilities($archetype) {
  * Return default roles that can be assigned, overridden or switched
  * by give role archetype.
  *
- * @param string $type  assign|override|switch
+ * @param string $type  assign|override|switch|view
  * @param string $archetype
  * @return array of role ids
  */
@@ -2034,6 +2034,16 @@ function get_default_role_archetype_allows($type, $archetype) {
             'user'           => array(),
             'frontpage'      => array(),
         ),
+        'view' => array(
+            'manager'        => array('manager', 'coursecreator', 'editingteacher', 'teacher', 'student', 'guest', 'user', 'frontpage'),
+            'coursecreator'  => array('coursecreator', 'editingteacher', 'teacher', 'student'),
+            'editingteacher' => array('coursecreator', 'editingteacher', 'teacher', 'student'),
+            'teacher'        => array('coursecreator', 'editingteacher', 'teacher', 'student'),
+            'student'        => array('coursecreator', 'editingteacher', 'teacher', 'student'),
+            'guest'          => array(),
+            'user'           => array(),
+            'frontpage'      => array(),
+        ),
     );
 
     if (!isset($defaults[$type][$archetype])) {
@@ -2602,10 +2612,14 @@ function get_user_roles_in_course($userid, $courseid) {
     $rolestring = '';
 
     if ($roles = $DB->get_records_sql($sql, $params)) {
-        $rolenames = role_fix_names($roles, $context, ROLENAME_ALIAS, true);   // Substitute aliases
+        $viewableroles = get_viewable_roles($context, $userid);
 
-        foreach ($rolenames as $roleid => $rolename) {
-            $rolenames[$roleid] = '<a href="'.$CFG->wwwroot.'/user/index.php?contextid='.$context->id.'&amp;roleid='.$roleid.'">'.$rolename.'</a>';
+        $rolenames = array();
+        foreach ($roles as $roleid => $unused) {
+            if (isset($viewableroles[$roleid])) {
+                $url = new moodle_url('/user/index.php', ['contextid' => $context->id, 'roleid' => $roleid]);
+                $rolenames[] = '<a href="' . $url . '">' . $viewableroles[$roleid] . '</a>';
+            }
         }
         $rolestring = implode(',', $rolenames);
     }
@@ -2883,6 +2897,22 @@ function allow_switch($fromroleid, $targetroleid) {
     $DB->insert_record('role_allow_switch', $record);
 }
 
+/**
+ * Creates a record in the role_allow_view table
+ *
+ * @param int $fromroleid source roleid
+ * @param int $targetroleid target roleid
+ * @return void
+ */
+function core_role_set_view_allowed($fromroleid, $targetroleid) {
+    global $DB;
+
+    $record = new stdClass();
+    $record->roleid      = $fromroleid;
+    $record->allowview = $targetroleid;
+    $DB->insert_record('role_allow_view', $record);
+}
+
 /**
  * Gets a list of roles that this user can assign in this context
  *
@@ -3023,6 +3053,58 @@ function get_switchable_roles(context $context) {
     return role_fix_names($roles, $context, ROLENAME_ALIAS, true);
 }
 
+/**
+ * Gets a list of roles that this user can view in a context
+ *
+ * @param context $context a context.
+ * @param int $userid id of user.
+ * @return array an array $roleid => $rolename.
+ */
+function get_viewable_roles(context $context, $userid = null) {
+    global $USER, $DB;
+
+    if ($userid == null) {
+        $userid = $USER->id;
+    }
+
+    $params = array();
+    $extrajoins = '';
+    $extrawhere = '';
+    if (!is_siteadmin()) {
+        // Admins are allowed to view any role.
+        // Others are subject to the additional constraint that the view role must be allowed by
+        // 'role_allow_view' for some role they have assigned in this context or any parent.
+        $contexts = $context->get_parent_context_ids(true);
+        list($insql, $inparams) = $DB->get_in_or_equal($contexts, SQL_PARAMS_NAMED);
+
+        $extrajoins = "JOIN {role_allow_view} ras ON ras.allowview = r.id
+                       JOIN {role_assignments} ra ON ra.roleid = ras.roleid";
+        $extrawhere = "WHERE ra.userid = :userid AND ra.contextid $insql";
+
+        $params += $inparams;
+        $params['userid'] = $userid;
+    }
+
+    if ($coursecontext = $context->get_course_context(false)) {
+        $params['coursecontext'] = $coursecontext->id;
+    } else {
+        $params['coursecontext'] = 0; // No course aliases.
+        $coursecontext = null;
+    }
+
+    $query = "
+        SELECT r.id, r.name, r.shortname, rn.name AS coursealias
+          FROM {role} r
+          $extrajoins
+     LEFT JOIN {role_names} rn ON (rn.contextid = :coursecontext AND rn.roleid = r.id)
+          $extrawhere
+      GROUP BY r.id, r.name, r.shortname, rn.name
+      ORDER BY r.sortorder";
+    $roles = $DB->get_records_sql($query, $params);
+
+    return role_fix_names($roles, $context, ROLENAME_ALIAS, true);
+}
+
 /**
  * Gets a list of roles that this user can override in this context.
  *
diff --git a/lib/classes/event/role_allow_view_updated.php b/lib/classes/event/role_allow_view_updated.php
new file mode 100644
index 0000000000000..1d08df97eb8e7
--- /dev/null
+++ b/lib/classes/event/role_allow_view_updated.php
@@ -0,0 +1,82 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+/**
+ * Role allow view updated event.
+ *
+ * @package    core
+ * @since      Moodle 3.4
+ * @copyright  2017 Andrew Hancox <andrewdchancox@googlemail.com>
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+
+namespace core\event;
+
+defined('MOODLE_INTERNAL') || die();
+
+/**
+ * Role allow view updated event class.
+ *
+ * @package    core
+ * @since      Moodle 2.6
+ * @copyright  2013 Rajesh Taneja <rajesh@moodle.com>
+ * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
+ */
+class role_allow_view_updated extends base {
+    /**
+     * Initialise event parameters.
+     */
+    protected function init() {
+        $this->data['crud'] = 'u';
+        $this->data['edulevel'] = self::LEVEL_OTHER;
+    }
+
+    /**
+     * Returns localised event name.
+     *
+     * @return string
+     */
+    public static function get_name() {
+        return get_string('eventroleallowviewupdated', 'role');
+    }
+
+    /**
+     * Returns non-localised event description with id's for admin use only.
+     *
+     * @return string
+     */
+    public function get_description() {
+        return "The user with id '$this->userid' updated Allow role views.";
+    }
+
+    /**
+     * Returns relevant URL.
+     *
+     * @return \moodle_url
+     */
+    public function get_url() {
+        return new \moodle_url('/admin/roles/allow.php', array('mode' => 'view'));
+    }
+
+    /**
+     * Returns array of parameters to be passed to legacy add_to_log() function.
+     *
+     * @return array
+     */
+    protected function get_legacy_logdata() {
+        return array(SITEID, 'role', 'edit allow view', 'admin/roles/allow.php?mode=view');
+    }
+}
diff --git a/lib/db/install.php b/lib/db/install.php
index 22816a158cfd7..2c2e576accc5f 100644
--- a/lib/db/install.php
+++ b/lib/db/install.php
@@ -266,7 +266,7 @@ function xmldb_main_install() {
 
     // Default allow role matrices.
     foreach ($DB->get_records('role') as $role) {
-        foreach (array('assign', 'override', 'switch') as $type) {
+        foreach (array('assign', 'override', 'switch', 'view') as $type) {
             $function = 'allow_'.$type;
             $allows = get_default_role_archetype_allows($type, $role->archetype);
             foreach ($allows as $allowid) {
diff --git a/lib/db/install.xml b/lib/db/install.xml
index 821cf37726b25..f95a699618c64 100644
--- a/lib/db/install.xml
+++ b/lib/db/install.xml
@@ -1077,6 +1077,20 @@
         <INDEX NAME="roleid-allowoverride" UNIQUE="true" FIELDS="roleid, allowswitch" COMMENT="Each pair (roleid, allowswitch) must appear at most once."/>
       </INDEXES>
     </TABLE>
+    <TABLE NAME="role_allow_view" COMMENT="This table stores which which other roles a user is allowed to view to if they have one role.">
+      <FIELDS>
+        <FIELD NAME="id" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="true"/>
+        <FIELD NAME="roleid" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="false" COMMENT="The role the user has."/>
+        <FIELD NAME="allowview" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="false" COMMENT="The id of a role that the user is allowed to view to as a result of having this role."/>
+      </FIELDS>
+      <KEYS>
+        <KEY NAME="primary" TYPE="primary" FIELDS="id"/>
+        <KEY NAME="roleid" TYPE="foreign" FIELDS="roleid" REFTABLE="role" REFFIELDS="id"/>
+      </KEYS>
+      <INDEXES>
+        <INDEX NAME="roleid-allowview" UNIQUE="true" FIELDS="roleid, allowview" COMMENT="Each pair (roleid, allowview) must appear at most once."/>
+      </INDEXES>
+    </TABLE>
     <TABLE NAME="role_assignments" COMMENT="assigning roles in different context">
       <FIELDS>
         <FIELD NAME="id" TYPE="int" LENGTH="10" NOTNULL="true" SEQUENCE="true"/>
diff --git a/lib/db/upgrade.php b/lib/db/upgrade.php
index 31019f526b748..e2e68c1526b1e 100644
--- a/lib/db/upgrade.php
+++ b/lib/db/upgrade.php
@@ -1859,5 +1859,48 @@ function xmldb_main_upgrade($oldversion) {
         upgrade_main_savepoint(true, 2017121200.00);
     }
 
+    if ($oldversion < 2017121900.00) {
+
+        // Define table role_allow_view to be created.
+        $table = new xmldb_table('role_allow_view');
+
+        // Adding fields to table role_allow_view.
+        $table->add_field('id', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, XMLDB_SEQUENCE, null);
+        $table->add_field('roleid', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, null);
+        $table->add_field('allowview', XMLDB_TYPE_INTEGER, '10', null, XMLDB_NOTNULL, null, null);
+
+        // Adding keys to table role_allow_view.
+        $table->add_key('primary', XMLDB_KEY_PRIMARY, array('id'));
+        $table->add_key('roleid', XMLDB_KEY_FOREIGN, array('roleid'), 'role', array('id'));
+        $table->add_key('allowview', XMLDB_KEY_FOREIGN, array('allowview'), 'role', array('id'));
+
+        // Conditionally launch create table for role_allow_view.
+        if (!$dbman->table_exists($table)) {
+            $dbman->create_table($table);
+        }
+
+        $index = new xmldb_index('roleid-allowview', XMLDB_INDEX_UNIQUE, array('roleid', 'allowview'));
+
+        // Conditionally launch add index roleid.
+        if (!$dbman->index_exists($table, $index)) {
+            $dbman->add_index($table, $index);
+        }
+
+        $roles = $DB->get_records('role', array(), 'sortorder ASC');
+
+        $DB->delete_records('role_allow_view');
+        foreach ($roles as $role) {
+            foreach ($roles as $allowedrole) {
+                $record = new stdClass();
+                $record->roleid      = $role->id;
+                $record->allowview = $allowedrole->id;
+                $DB->insert_record('role_allow_view', $record);
+            }
+        }
+
+        // Main savepoint reached.
+        upgrade_main_savepoint(true, 2017121900.00);
+    }
+
     return true;
 }
diff --git a/lib/statslib.php b/lib/statslib.php
index 9dd95ca0378fe..9dfd7f05f74f6 100644
--- a/lib/statslib.php
+++ b/lib/statslib.php
@@ -1404,6 +1404,7 @@ function stats_get_report_options($courseid,$mode) {
             $sql = 'SELECT r.id, r.name, r.shortname FROM {role} r JOIN {stats_daily} s ON s.roleid = r.id
                  WHERE s.courseid = :courseid GROUP BY r.id, r.name, r.shortname';
             if ($roles = $DB->get_records_sql($sql, array('courseid' => $courseid))) {
+                $roles = array_intersect_key($roles, get_viewable_roles($context));
                 foreach ($roles as $role) {
                     $reportoptions[STATS_REPORT_ACTIVITYBYROLE.$role->id] = get_string('statsreport'.STATS_REPORT_ACTIVITYBYROLE).
                         ' ' . role_get_name($role, $context);
diff --git a/lib/testing/generator/data_generator.php b/lib/testing/generator/data_generator.php
index 4bf2e8b42bf53..6d422506a12d0 100644
--- a/lib/testing/generator/data_generator.php
+++ b/lib/testing/generator/data_generator.php
@@ -789,9 +789,9 @@ public function create_role($record=null) {
 
         if ($record['archetype']) {
 
-            // We copy all the roles the archetype can assign, override and switch to.
+            // We copy all the roles the archetype can assign, override, switch to and view.
             if ($record['archetype']) {
-                $types = array('assign', 'override', 'switch');
+                $types = array('assign', 'override', 'switch', 'view');
                 foreach ($types as $type) {
                     $rolestocopy = get_default_role_archetype_allows($type, $record['archetype']);
                     foreach ($rolestocopy as $tocopy) {
diff --git a/lib/tests/accesslib_test.php b/lib/tests/accesslib_test.php
index a4d16e5d60b27..d18cb52b23b65 100644
--- a/lib/tests/accesslib_test.php
+++ b/lib/tests/accesslib_test.php
@@ -918,6 +918,9 @@ public function test_get_default_role_archetype_allows() {
 
             $result = get_default_role_archetype_allows('switch', $archetype);
             $this->assertInternalType('array', $result);
+
+            $result = get_default_role_archetype_allows('view', $archetype);
+            $this->assertInternalType('array', $result);
         }
 
         $result = get_default_role_archetype_allows('assign', '');
@@ -929,6 +932,9 @@ public function test_get_default_role_archetype_allows() {
         $result = get_default_role_archetype_allows('switch', '');
         $this->assertSame(array(), $result);
 
+        $result = get_default_role_archetype_allows('view', '');
+        $this->assertSame(array(), $result);
+
         $result = get_default_role_archetype_allows('assign', 'wrongarchetype');
         $this->assertSame(array(), $result);
         $this->assertDebuggingCalled();
@@ -940,6 +946,10 @@ public function test_get_default_role_archetype_allows() {
         $result = get_default_role_archetype_allows('switch', 'wrongarchetype');
         $this->assertSame(array(), $result);
         $this->assertDebuggingCalled();
+
+        $result = get_default_role_archetype_allows('view', 'wrongarchetype');
+        $this->assertSame(array(), $result);
+        $this->assertDebuggingCalled();
     }
 
     /**
@@ -1029,6 +1039,35 @@ public function test_allow_switch() {
         $this->assertEventLegacyLogData($expectedlegacylog, $event);
     }
 
+    /**
+     * Test allowing of role switching.
+     */
+    public function test_allow_view() {
+        global $DB, $CFG;
+
+        $this->resetAfterTest();
+
+        $otherid = create_role('Other role', 'other', 'Some other role', '');
+        $student = $DB->get_record('role', array('shortname' => 'student'), '*', MUST_EXIST);
+
+        $this->assertFalse($DB->record_exists('role_allow_view', array('roleid' => $otherid, 'allowview' => $student->id)));
+        allow_view($otherid, $student->id);
+        $this->assertTrue($DB->record_exists('role_allow_view', array('roleid' => $otherid, 'allowview' => $student->id)));
+
+        // Test event trigger.
+        $allowroleassignevent = \core\event\role_allow_view_updated::create(array('context' => context_system::instance()));
+        $sink = $this->redirectEvents();
+        $allowroleassignevent->trigger();
+        $events = $sink->get_events();
+        $sink->close();
+        $event = array_pop($events);
+        $this->assertInstanceOf('\core\event\role_allow_view_updated', $event);
+        $mode = 'view';
+        $baseurl = new moodle_url('/admin/roles/allow.php', array('mode' => $mode));
+        $expectedlegacylog = array(SITEID, 'role', 'edit allow ' . $mode, str_replace($CFG->wwwroot . '/', '', $baseurl));
+        $this->assertEventLegacyLogData($expectedlegacylog, $event);
+    }
+
     /**
      * Test returning of assignable roles in context.
      */
@@ -1287,6 +1326,74 @@ public function test_get_overridable_roles() {
         }
     }
 
+    /**
+     * Test getting of all overridable roles.
+     */
+    public function test_get_viewable_roles_course() {
+        global $DB;
+
+        $this->resetAfterTest();
+
+        $course = $this->getDataGenerator()->create_course();
+        $coursecontext = context_course::instance($course->id);
+
+        $teacherrole = $DB->get_record('role', array('shortname' => 'editingteacher'), '*', MUST_EXIST);
+        $teacher = $this->getDataGenerator()->create_user();
+        role_assign($teacherrole->id, $teacher->id, $coursecontext);
+
+        $studentrole = $DB->get_record('role', array('shortname' => 'student'), '*', MUST_EXIST);
+        $studentrolerename = (object) array('roleid' => $studentrole->id, 'name' => 'Učitel', 'contextid' => $coursecontext->id);
+        $DB->insert_record('role_names', $studentrolerename);
+
+        // By default teacher can see student.
+        $this->setUser($teacher);
+        $viewableroles = get_viewable_roles($coursecontext);
+        $this->assertContains($studentrolerename->name, array_values($viewableroles));
+        // Remove view permission.
+        $DB->delete_records('role_allow_view', array('roleid' => $teacherrole->id, 'allowview' => $studentrole->id));
+        $viewableroles = get_viewable_roles($coursecontext);
+        // Teacher can no longer see student role.
+        $this->assertNotContains($studentrolerename->name, array_values($viewableroles));
+        // Allow again teacher to view student.
+        allow_view($teacherrole->id, $studentrole->id);
+        // Teacher can now see student role.
+        $viewableroles = get_viewable_roles($coursecontext);
+        $this->assertContains($studentrolerename->name, array_values($viewableroles));
+    }
+
+    /**
+     * Test getting of all overridable roles.
+     */
+    public function test_get_viewable_roles_system() {
+        global $DB;
+
+        $this->resetAfterTest();
+
+        $context = context_system::instance();
+
+        $teacherrole = $DB->get_record('role', array('shortname' => 'editingteacher'), '*', MUST_EXIST);
+        $teacher = $this->getDataGenerator()->create_user();
+        role_assign($teacherrole->id, $teacher->id, $context);
+
+        $studentrole = $DB->get_record('role', array('shortname' => 'student'), '*', MUST_EXIST);
+        $studentrolename = role_get_name($studentrole, $context);
+
+        // By default teacher can see student.
+        $this->setUser($teacher);
+        $viewableroles = get_viewable_roles($context);
+        $this->assertContains($studentrolename, array_values($viewableroles));
+        // Remove view permission.
+        $DB->delete_records('role_allow_view', array('roleid' => $teacherrole->id, 'allowview' => $studentrole->id));
+        $viewableroles = get_viewable_roles($context);
+        // Teacher can no longer see student role.
+        $this->assertNotContains($studentrolename, array_values($viewableroles));
+        // Allow again teacher to view student.
+        allow_view($teacherrole->id, $studentrole->id);
+        // Teacher can now see student role.
+        $viewableroles = get_viewable_roles($context);
+        $this->assertContains($studentrolename, array_values($viewableroles));
+    }
+
     /**
      * Test we have context level defaults.
      */
@@ -1553,6 +1660,8 @@ public function test_get_user_roles_in_course() {
         $user4 = $this->getDataGenerator()->create_user();
         role_assign($managerrole->id, $user4->id, $coursecontext->id);
 
+        $this->setAdminUser();
+
         $roles = get_user_roles_in_course($user1->id, $course->id);
         $this->assertEquals(1, preg_match_all('/,/', $roles, $matches));
         $this->assertTrue(strpos($roles, role_get_name($teacherrole, $coursecontext)) !== false);
diff --git a/lib/upgrade.txt b/lib/upgrade.txt
index ccc2381f8e62e..924e5af972587 100644
--- a/lib/upgrade.txt
+++ b/lib/upgrade.txt
@@ -58,6 +58,9 @@ information provided here is intended especially for developers.
 * $CFG->loginhttps is now deprecated, do not use it.
 * $PAGE->https_required and $PAGE->verify_https_required() are now deprecated. They are no longer used and will throw a coding_exception.
 * $CFG->httpswwwroot is now deprecated and will always result in the same value as wwwroot.
+* Added function core_role_set_view_allowed() to check if a user should be able to see a given role.
+  This should be checked whenever displaying a list of roles to a user, however, core_role_set_assign_allowed may need to override it
+  in some cases.
 
 === 3.3.1 ===
 
diff --git a/report/stats/locallib.php b/report/stats/locallib.php
index 75b91b680f77d..65389046c75de 100644
--- a/report/stats/locallib.php
+++ b/report/stats/locallib.php
@@ -287,14 +287,14 @@ function report_stats_report($course, $report, $mode, $user, $roleid, $time) {
                 $times = array();
                 $missedlines = array();
                 $coursecontext = context_course::instance($course->id);
-                $rolenames = role_fix_names(get_all_roles($coursecontext), $coursecontext, ROLENAME_ALIAS, true);
+                $rolenames = get_viewable_roles($coursecontext);
                 foreach ($stats as $stat) {
                     if (!empty($stat->zerofixed)) {
                         $missedlines[] = $stat->timeend;
                     }
                     $data[$stat->timeend][$stat->roleid] = $stat->line1;
                     if ($stat->roleid != 0) {
-                        if (!array_key_exists($stat->roleid,$roles)) {
+                        if (!array_key_exists($stat->roleid, $roles) && array_key_exists($stat->roleid, $rolenames)) {
                             $roles[$stat->roleid] = $rolenames[$stat->roleid];
                         }
                     } else {
@@ -419,7 +419,7 @@ function report_stats_print_chart($courseid, $report, $time, $mode, $userid = 0,
         $times = array();
         $roles = array();
         $missedlines = array();
-        $rolenames = role_fix_names(get_all_roles($coursecontext), $coursecontext, ROLENAME_ALIAS, true);
+        $rolenames = get_viewable_roles($coursecontext);
 
         foreach ($stats as $stat) {
             $data[$stat->roleid][$stat->timeend] = $stat->line1;
@@ -427,7 +427,7 @@ function report_stats_print_chart($courseid, $report, $time, $mode, $userid = 0,
                 $missedlines[] = $stat->timeend;
             }
             if ($stat->roleid != 0) {
-                if (!array_key_exists($stat->roleid, $roles)) {
+                if (!array_key_exists($stat->roleid, $roles) && array_key_exists($stat->roleid, $rolenames)) {
                     $roles[$stat->roleid] = $rolenames[$stat->roleid];
                 }
             } else {
diff --git a/user/classes/output/user_roles_editable.php b/user/classes/output/user_roles_editable.php
index 56c8aec0803ff..12fc5c51e7b41 100644
--- a/user/classes/output/user_roles_editable.php
+++ b/user/classes/output/user_roles_editable.php
@@ -49,6 +49,12 @@ class user_roles_editable extends \core\output\inplace_editable {
     /** @var \stdClass[] $profileroles */
     private $profileroles;
 
+    /** @var \stdClass[] $viewableroles */
+    private $viewableroles;
+
+    /** @var \stdClass[] $assignableroles */
+    private $assignableroles;
+
     /**
      * Constructor.
      *
@@ -60,7 +66,11 @@ class user_roles_editable extends \core\output\inplace_editable {
      * @param \stdClass[] $profileroles The list of roles that should be visible in a users profile.
      * @param \stdClass[] $userroles The list of user roles.
      */
-    public function __construct($course, $context, $user, $courseroles, $assignableroles, $profileroles, $userroles) {
+    public function __construct($course, $context, $user, $courseroles, $assignableroles, $profileroles, $userroles, $viewableroles = null) {
+        if ($viewableroles === null) {
+            debugging('Constructor for user_roles_editable now needs the result of get_viewable_roles passed as viewableroles');
+        }
+
         // Check capabilities to get editable value.
         $editable = has_capability('moodle/role:assign', $context);
 
@@ -77,6 +87,8 @@ public function __construct($course, $context, $user, $courseroles, $assignabler
         // Remember these for the display value.
         $this->courseroles = $courseroles;
         $this->profileroles = $profileroles;
+        $this->viewableroles = array_keys($viewableroles);
+        $this->assignableroles = array_keys($assignableroles);
         $this->context = $context;
 
         parent::__construct('core_user', 'user_roles', $itemid, $editable, $value, $value);
@@ -106,8 +118,9 @@ public function __construct($course, $context, $user, $courseroles, $assignabler
     public function export_for_template(\renderer_base $output) {
         $listofroles = [];
         $roleids = json_decode($this->value);
+        $viewableroleids = array_intersect($roleids, array_merge($this->viewableroles, $this->assignableroles));
 
-        foreach ($roleids as $id) {
+        foreach ($viewableroleids as $id) {
             // If this is a student, we only show a subset of the roles.
             if ($this->editable || array_key_exists($id, $this->profileroles)) {
                 $listofroles[] = format_string($this->courseroles[$id]->localname, true, ['context' => $this->context]);
@@ -116,6 +129,8 @@ public function export_for_template(\renderer_base $output) {
 
         if (!empty($listofroles)) {
             $this->displayvalue = implode($listofroles, ', ');
+        } else if (!empty($roleids) && empty($viewableroleids)) {
+            $this->displayvalue = get_string('novisibleroles', 'role');
         } else {
             $this->displayvalue = get_string('noroles', 'role');
         }
@@ -160,6 +175,7 @@ public static function update($itemid, $newvalue) {
         // Check that all the groups belong to the course.
         $allroles = role_fix_names(get_all_roles($context), $context);
         $assignableroles = get_assignable_roles($context, ROLENAME_ALIAS, false);
+        $viewableroles = get_viewable_roles($context);
         $userrolesbyid = get_user_roles($context, $userid, true, 'c.contextlevel DESC, r.sortorder ASC');
         $profileroles = get_profile_roles($context);
 
@@ -224,6 +240,6 @@ public static function update($itemid, $newvalue) {
 
         $course = get_course($courseid);
         $user = core_user::get_user($userid);
-        return new self($course, $context, $user, $allroles, $assignableroles, $profileroles, $userroles);
+        return new self($course, $context, $user, $allroles, $assignableroles, $profileroles, $userroles, $viewableroles);
     }
 }
diff --git a/user/classes/participants_table.php b/user/classes/participants_table.php
index bc66a9efbe101..09f92468e71b9 100644
--- a/user/classes/participants_table.php
+++ b/user/classes/participants_table.php
@@ -129,6 +129,9 @@ class participants_table extends \table_sql {
      */
     protected $profileroles;
 
+    /** @var \stdClass[] $viewableroles */
+    private $viewableroles;
+
     /**
      * Sets up the table.
      *
@@ -236,6 +239,7 @@ public function __construct($courseid, $currentgroup, $accesssince, $roleid, $en
         $this->allroleassignments = get_users_roles($this->context, [], true, 'c.contextlevel DESC, r.sortorder ASC');
         $this->assignableroles = get_assignable_roles($this->context, ROLENAME_ALIAS, false);
         $this->profileroles = get_profile_roles($this->context);
+        $this->viewableroles = get_viewable_roles($this->context);
     }
 
     /**
@@ -299,7 +303,8 @@ public function col_roles($data) {
                                                               $this->allroles,
                                                               $this->assignableroles,
                                                               $this->profileroles,
-                                                              $roles);
+                                                              $roles,
+                                                              $this->viewableroles);
 
         return $OUTPUT->render_from_template('core/inplace_editable', $editable->export_for_template($OUTPUT));
     }
diff --git a/user/renderer.php b/user/renderer.php
index ca0fe04d4075f..d3af40a1c8bb4 100644
--- a/user/renderer.php
+++ b/user/renderer.php
@@ -127,7 +127,12 @@ public function unified_filter($course, $context, $filtersapplied) {
         $filteroptions = [];
 
         // Filter options for role.
-        $roles = role_fix_names(get_profile_roles($context), $context, ROLENAME_ALIAS, true);
+        $roleseditable = has_capability('moodle/role:assign', $context);
+        $roles = get_viewable_roles($context);
+        if ($roleseditable) {
+            $roles += get_assignable_roles($context, ROLENAME_ALIAS);
+        }
+
         $criteria = get_string('role');
         $roleoptions = [];
         foreach ($roles as $id => $role) {
diff --git a/version.php b/version.php
index 454b481422d9b..fd60f44e0b9d7 100644
--- a/version.php
+++ b/version.php
@@ -29,7 +29,7 @@
 
 defined('MOODLE_INTERNAL') || die();
 
-$version  = 2017121400.00;              // YYYYMMDD      = weekly release date of this DEV branch.
+$version  = 2017121900.00;              // YYYYMMDD      = weekly release date of this DEV branch.
                                         //         RR    = release increments - 00 in DEV branches.
                                         //           .XX = incremental changes.
 

From 64cd459648a85a9b96a6a6824e93f9bbe5645f8e Mon Sep 17 00:00:00 2001
From: Andrew Hancox <andrewdchancox@googlemail.com>
Date: Fri, 24 Nov 2017 12:30:40 +0000
Subject: [PATCH 2/3] MDL-50666 core: Rename allow_ACTION to
 core_role_set_ACTION_allowed

---
 admin/roles/classes/allow_assign_page.php     |  2 +-
 admin/roles/classes/allow_override_page.php   |  2 +-
 admin/roles/classes/allow_switch_page.php     |  2 +-
 admin/roles/classes/allow_view_page.php       |  2 +-
 .../classes/define_role_table_advanced.php    |  2 +-
 backup/moodle2/tests/moodle2_test.php         |  2 +-
 enrol/manual/tests/externallib_test.php       |  2 +-
 lib/accesslib.php                             | 14 +++---
 lib/db/install.php                            |  2 +-
 lib/deprecatedlib.php                         | 45 +++++++++++++++++++
 lib/testing/generator/data_generator.php      |  2 +-
 lib/tests/accesslib_test.php                  | 22 ++++-----
 lib/upgrade.txt                               |  1 +
 13 files changed, 73 insertions(+), 27 deletions(-)

diff --git a/admin/roles/classes/allow_assign_page.php b/admin/roles/classes/allow_assign_page.php
index 8d31cc082a0e1..a89228fdd1fcf 100644
--- a/admin/roles/classes/allow_assign_page.php
+++ b/admin/roles/classes/allow_assign_page.php
@@ -33,7 +33,7 @@ public function __construct() {
     }
 
     protected function set_allow($fromroleid, $targetroleid) {
-        allow_assign($fromroleid, $targetroleid);
+        core_role_set_assign_allowed($fromroleid, $targetroleid);
     }
 
     protected function get_cell_tooltip($fromrole, $targetrole) {
diff --git a/admin/roles/classes/allow_override_page.php b/admin/roles/classes/allow_override_page.php
index a277abb6d6d34..4b160ee015e27 100644
--- a/admin/roles/classes/allow_override_page.php
+++ b/admin/roles/classes/allow_override_page.php
@@ -33,7 +33,7 @@ public function __construct() {
     }
 
     protected function set_allow($fromroleid, $targetroleid) {
-        allow_override($fromroleid, $targetroleid);
+        core_role_set_override_allowed($fromroleid, $targetroleid);
     }
 
     protected function get_cell_tooltip($fromrole, $targetrole) {
diff --git a/admin/roles/classes/allow_switch_page.php b/admin/roles/classes/allow_switch_page.php
index 934bda365d282..5b22e1e37113d 100644
--- a/admin/roles/classes/allow_switch_page.php
+++ b/admin/roles/classes/allow_switch_page.php
@@ -41,7 +41,7 @@ protected function load_required_roles() {
     }
 
     protected function set_allow($fromroleid, $targetroleid) {
-        allow_switch($fromroleid, $targetroleid);
+        core_role_set_switch_allowed($fromroleid, $targetroleid);
     }
 
     protected function is_allowed_target($targetroleid) {
diff --git a/admin/roles/classes/allow_view_page.php b/admin/roles/classes/allow_view_page.php
index 2899fc631b5d6..f1a1031e5bb3e 100644
--- a/admin/roles/classes/allow_view_page.php
+++ b/admin/roles/classes/allow_view_page.php
@@ -49,7 +49,7 @@ public function __construct() {
      * @param int $targetroleid
      */
     protected function set_allow($fromroleid, $targetroleid) {
-        allow_view($fromroleid, $targetroleid);
+        core_role_set_view_allowed($fromroleid, $targetroleid);
     }
 
     /**
diff --git a/admin/roles/classes/define_role_table_advanced.php b/admin/roles/classes/define_role_table_advanced.php
index 36cce176d4fa9..957fe852a8c57 100644
--- a/admin/roles/classes/define_role_table_advanced.php
+++ b/admin/roles/classes/define_role_table_advanced.php
@@ -469,7 +469,7 @@ protected function save_allow($type) {
         $current = array_keys($this->get_allow_roles_list($type));
         $wanted = $this->{'allow'.$type};
 
-        $addfunction = 'allow_'.$type;
+        $addfunction = "core_role_set_{$type}_allowed";
         $deltable = 'role_allow_'.$type;
         $field = 'allow'.$type;
 
diff --git a/backup/moodle2/tests/moodle2_test.php b/backup/moodle2/tests/moodle2_test.php
index 3b45eea4601d8..0589755d4d29a 100644
--- a/backup/moodle2/tests/moodle2_test.php
+++ b/backup/moodle2/tests/moodle2_test.php
@@ -596,7 +596,7 @@ protected function prepare_for_enrolments_test($target, $additionalcaps = []) {
             assign_capability($cap, CAP_ALLOW, $roleidcat, $categorycontext);
         }
 
-        allow_assign($roleidcat, $studentrole->id);
+        core_role_set_assign_allowed($roleidcat, $studentrole->id);
         role_assign($roleidcat, $user->id, $categorycontext);
         accesslib_clear_all_caches_for_unit_testing();
 
diff --git a/enrol/manual/tests/externallib_test.php b/enrol/manual/tests/externallib_test.php
index 2eefd4ae4fef1..6b224dd36b1fb 100644
--- a/enrol/manual/tests/externallib_test.php
+++ b/enrol/manual/tests/externallib_test.php
@@ -62,7 +62,7 @@ public function test_enrol_users() {
         $this->assignUserCapability('moodle/course:view', $context2->id, $roleid);
         $this->assignUserCapability('moodle/role:assign', $context2->id, $roleid);
 
-        allow_assign($roleid, 3);
+        core_role_set_assign_allowed($roleid, 3);
 
         // Call the external function.
         enrol_manual_external::enrol_users(array(
diff --git a/lib/accesslib.php b/lib/accesslib.php
index a9f1d19cfe24c..ed4c2bb4576ae 100644
--- a/lib/accesslib.php
+++ b/lib/accesslib.php
@@ -2852,16 +2852,16 @@ function get_user_roles_with_special(context $context, $userid = 0) {
 /**
  * Creates a record in the role_allow_override table
  *
- * @param int $sroleid source roleid
- * @param int $troleid target roleid
+ * @param int $fromroleid source roleid
+ * @param int $targetroleid target roleid
  * @return void
  */
-function allow_override($sroleid, $troleid) {
+function core_role_set_override_allowed($fromroleid, $targetroleid) {
     global $DB;
 
     $record = new stdClass();
-    $record->roleid        = $sroleid;
-    $record->allowoverride = $troleid;
+    $record->roleid        = $fromroleid;
+    $record->allowoverride = $targetroleid;
     $DB->insert_record('role_allow_override', $record);
 }
 
@@ -2872,7 +2872,7 @@ function allow_override($sroleid, $troleid) {
  * @param int $targetroleid target roleid
  * @return void
  */
-function allow_assign($fromroleid, $targetroleid) {
+function core_role_set_assign_allowed($fromroleid, $targetroleid) {
     global $DB;
 
     $record = new stdClass();
@@ -2888,7 +2888,7 @@ function allow_assign($fromroleid, $targetroleid) {
  * @param int $targetroleid target roleid
  * @return void
  */
-function allow_switch($fromroleid, $targetroleid) {
+function core_role_set_switch_allowed($fromroleid, $targetroleid) {
     global $DB;
 
     $record = new stdClass();
diff --git a/lib/db/install.php b/lib/db/install.php
index 2c2e576accc5f..ddf0aa26b9d32 100644
--- a/lib/db/install.php
+++ b/lib/db/install.php
@@ -267,7 +267,7 @@ function xmldb_main_install() {
     // Default allow role matrices.
     foreach ($DB->get_records('role') as $role) {
         foreach (array('assign', 'override', 'switch', 'view') as $type) {
-            $function = 'allow_'.$type;
+            $function = "core_role_set_{$type}_allowed";
             $allows = get_default_role_archetype_allows($type, $role->archetype);
             foreach ($allows as $allowid) {
                 $function($role->id, $allowid);
diff --git a/lib/deprecatedlib.php b/lib/deprecatedlib.php
index 9ef00198a56b8..a2065967b1d4a 100644
--- a/lib/deprecatedlib.php
+++ b/lib/deprecatedlib.php
@@ -6511,3 +6511,48 @@ function calendar_get_upcoming($courses, $groups, $users, $daysinfuture, $maxeve
 
     return $output;
 }
+
+/**
+ * Creates a record in the role_allow_override table
+ *
+ * @param int $sroleid source roleid
+ * @param int $troleid target roleid
+ * @return void
+ * @deprecated since Moodle 3.4. MDL-50666
+ */
+function allow_override($sroleid, $troleid) {
+    debugging('allow_override() has been deprecated. Please update your code to use core_role_set_override_allowed.',
+            DEBUG_DEVELOPER);
+
+    core_role_set_override_allowed($sroleid, $troleid);
+}
+
+/**
+ * Creates a record in the role_allow_assign table
+ *
+ * @param int $fromroleid source roleid
+ * @param int $targetroleid target roleid
+ * @return void
+ * @deprecated since Moodle 3.4. MDL-50666
+ */
+function allow_assign($fromroleid, $targetroleid) {
+    debugging('allow_assign() has been deprecated. Please update your code to use core_role_set_assign_allowed.',
+            DEBUG_DEVELOPER);
+
+    core_role_set_assign_allowed($fromroleid, $targetroleid);
+}
+
+/**
+ * Creates a record in the role_allow_switch table
+ *
+ * @param int $fromroleid source roleid
+ * @param int $targetroleid target roleid
+ * @return void
+ * @deprecated since Moodle 3.4. MDL-50666
+ */
+function allow_switch($fromroleid, $targetroleid) {
+    debugging('allow_switch() has been deprecated. Please update your code to use core_role_set_switch_allowed.',
+            DEBUG_DEVELOPER);
+
+    core_role_set_switch_allowed($fromroleid, $targetroleid);
+}
diff --git a/lib/testing/generator/data_generator.php b/lib/testing/generator/data_generator.php
index 6d422506a12d0..b3499c7d95f9c 100644
--- a/lib/testing/generator/data_generator.php
+++ b/lib/testing/generator/data_generator.php
@@ -795,7 +795,7 @@ public function create_role($record=null) {
                 foreach ($types as $type) {
                     $rolestocopy = get_default_role_archetype_allows($type, $record['archetype']);
                     foreach ($rolestocopy as $tocopy) {
-                        $functionname = 'allow_' . $type;
+                        $functionname = "core_role_set_{$type}_allowed";
                         $functionname($newroleid, $tocopy);
                     }
                 }
diff --git a/lib/tests/accesslib_test.php b/lib/tests/accesslib_test.php
index d18cb52b23b65..6ad832cf9b05f 100644
--- a/lib/tests/accesslib_test.php
+++ b/lib/tests/accesslib_test.php
@@ -955,7 +955,7 @@ public function test_get_default_role_archetype_allows() {
     /**
      * Test allowing of role assignments.
      */
-    public function test_allow_assign() {
+    public function test_core_role_set_assign_allowed() {
         global $DB, $CFG;
 
         $this->resetAfterTest();
@@ -964,7 +964,7 @@ public function test_allow_assign() {
         $student = $DB->get_record('role', array('shortname'=>'student'), '*', MUST_EXIST);
 
         $this->assertFalse($DB->record_exists('role_allow_assign', array('roleid'=>$otherid, 'allowassign'=>$student->id)));
-        allow_assign($otherid, $student->id);
+        core_role_set_assign_allowed($otherid, $student->id);
         $this->assertTrue($DB->record_exists('role_allow_assign', array('roleid'=>$otherid, 'allowassign'=>$student->id)));
 
         // Test event trigger.
@@ -984,7 +984,7 @@ public function test_allow_assign() {
     /**
      * Test allowing of role overrides.
      */
-    public function test_allow_override() {
+    public function test_core_role_set_override_allowed() {
         global $DB, $CFG;
 
         $this->resetAfterTest();
@@ -993,7 +993,7 @@ public function test_allow_override() {
         $student = $DB->get_record('role', array('shortname'=>'student'), '*', MUST_EXIST);
 
         $this->assertFalse($DB->record_exists('role_allow_override', array('roleid'=>$otherid, 'allowoverride'=>$student->id)));
-        allow_override($otherid, $student->id);
+        core_role_set_override_allowed($otherid, $student->id);
         $this->assertTrue($DB->record_exists('role_allow_override', array('roleid'=>$otherid, 'allowoverride'=>$student->id)));
 
         // Test event trigger.
@@ -1013,7 +1013,7 @@ public function test_allow_override() {
     /**
      * Test allowing of role switching.
      */
-    public function test_allow_switch() {
+    public function test_core_role_set_switch_allowed() {
         global $DB, $CFG;
 
         $this->resetAfterTest();
@@ -1022,7 +1022,7 @@ public function test_allow_switch() {
         $student = $DB->get_record('role', array('shortname'=>'student'), '*', MUST_EXIST);
 
         $this->assertFalse($DB->record_exists('role_allow_switch', array('roleid'=>$otherid, 'allowswitch'=>$student->id)));
-        allow_switch($otherid, $student->id);
+        core_role_set_switch_allowed($otherid, $student->id);
         $this->assertTrue($DB->record_exists('role_allow_switch', array('roleid'=>$otherid, 'allowswitch'=>$student->id)));
 
         // Test event trigger.
@@ -1042,7 +1042,7 @@ public function test_allow_switch() {
     /**
      * Test allowing of role switching.
      */
-    public function test_allow_view() {
+    public function test_core_role_set_view_allowed() {
         global $DB, $CFG;
 
         $this->resetAfterTest();
@@ -1051,7 +1051,7 @@ public function test_allow_view() {
         $student = $DB->get_record('role', array('shortname' => 'student'), '*', MUST_EXIST);
 
         $this->assertFalse($DB->record_exists('role_allow_view', array('roleid' => $otherid, 'allowview' => $student->id)));
-        allow_view($otherid, $student->id);
+        core_role_set_view_allowed($otherid, $student->id);
         $this->assertTrue($DB->record_exists('role_allow_view', array('roleid' => $otherid, 'allowview' => $student->id)));
 
         // Test event trigger.
@@ -1355,7 +1355,7 @@ public function test_get_viewable_roles_course() {
         // Teacher can no longer see student role.
         $this->assertNotContains($studentrolerename->name, array_values($viewableroles));
         // Allow again teacher to view student.
-        allow_view($teacherrole->id, $studentrole->id);
+        core_role_set_view_allowed($teacherrole->id, $studentrole->id);
         // Teacher can now see student role.
         $viewableroles = get_viewable_roles($coursecontext);
         $this->assertContains($studentrolerename->name, array_values($viewableroles));
@@ -1388,7 +1388,7 @@ public function test_get_viewable_roles_system() {
         // Teacher can no longer see student role.
         $this->assertNotContains($studentrolename, array_values($viewableroles));
         // Allow again teacher to view student.
-        allow_view($teacherrole->id, $studentrole->id);
+        core_role_set_view_allowed($teacherrole->id, $studentrole->id);
         // Teacher can now see student role.
         $viewableroles = get_viewable_roles($context);
         $this->assertContains($studentrolename, array_values($viewableroles));
@@ -3520,7 +3520,7 @@ public function test_get_profile_roles() {
         create_role('Custom role', 'customrole', 'Custom course role');
         $customrole = $DB->get_record('role', array('shortname' => 'customrole'), '*', MUST_EXIST);
         set_role_contextlevels($customrole->id, [CONTEXT_COURSE]);
-        allow_assign($teacherrole->id, $customrole->id); // Allow teacher to assign the role in the course.
+        core_role_set_assign_allowed($teacherrole->id, $customrole->id); // Allow teacher to assign the role in the course.
 
         // Set the site policy 'profileroles' to show student, teacher and non-editing teacher roles (i.e. not the custom role).
         $neteacherrole = $DB->get_record('role', array('shortname' => 'teacher'), '*', MUST_EXIST);
diff --git a/lib/upgrade.txt b/lib/upgrade.txt
index 924e5af972587..008e68017c304 100644
--- a/lib/upgrade.txt
+++ b/lib/upgrade.txt
@@ -61,6 +61,7 @@ information provided here is intended especially for developers.
 * Added function core_role_set_view_allowed() to check if a user should be able to see a given role.
   This should be checked whenever displaying a list of roles to a user, however, core_role_set_assign_allowed may need to override it
   in some cases.
+* Deprecated allow_override, allow_assign and allow_switch and replaced with core_role_set_*_allowed to avoid function names conflicting.
 
 === 3.3.1 ===
 

From eaf01ad4aa245f445dd9b31852c70714b82de719 Mon Sep 17 00:00:00 2001
From: Jun Pataleta <jun@moodle.com>
Date: Wed, 20 Dec 2017 10:16:44 +1300
Subject: [PATCH 3/3] MDL-50666 group: Make test run without @javascript

---
 group/tests/behat/role_visibility.feature | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/group/tests/behat/role_visibility.feature b/group/tests/behat/role_visibility.feature
index f21d2acd27b32..4d2bd1fc43ecb 100644
--- a/group/tests/behat/role_visibility.feature
+++ b/group/tests/behat/role_visibility.feature
@@ -27,7 +27,6 @@ Feature: Test role visibility
       | teacher1 | G1    |
       | manager1 | G1    |
 
-  @javascript
   Scenario: Check the default roles are visible
     Given I log in as "manager1"
     And I am on "Course 1" course homepage
@@ -40,14 +39,14 @@ Feature: Test role visibility
     And "optgroup[label='Manager']" "css_element" should exist in the "#members" "css_element"
     And I log out
 
-  @javascript
   Scenario: Do not allow managers to view any roles and check they are hidden
     Given I log in as "teacher1"
     And I am on "Course 1" course homepage
     And I navigate to "Groups" node in "Course administration > Users"
     When I set the field "groups" to "Group 1 (3)"
+    And I press "Show members for group"
     Then "optgroup[label='No roles']" "css_element" should exist in the "#members" "css_element"
     And "optgroup[label='Student']" "css_element" should exist in the "#members" "css_element"
     And "optgroup[label='Teacher']" "css_element" should exist in the "#members" "css_element"
     And "optgroup[label='Manager']" "css_element" should not exist in the "#members" "css_element"
-    And I log out
\ No newline at end of file
+    And I log out