From e9584cb8964171fbb70921d4cccc111025b530cb Mon Sep 17 00:00:00 2001
From: Martin Krulis <krulis@ksi.mff.cuni.cz>
Date: Wed, 22 May 2019 15:11:25 +0200
Subject: [PATCH] Fixing bug in notification role testing (and better
 documenting the code to avoid such mistakes).

---
 .../presenters/NotificationsPresenter.php     |  4 +--
 app/V1Module/security/Authorizator.php        |  4 +--
 .../Policies/NotificationPermissionPolicy.php |  2 +-
 app/V1Module/security/Roles.php               | 27 ++++++++++++-------
 4 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/app/V1Module/presenters/NotificationsPresenter.php b/app/V1Module/presenters/NotificationsPresenter.php
index 8f659d76b..d373a1212 100644
--- a/app/V1Module/presenters/NotificationsPresenter.php
+++ b/app/V1Module/presenters/NotificationsPresenter.php
@@ -63,7 +63,7 @@ function (Notification $notification) {
         return $this->notificationAcl->canViewDetail($notification);
       });
 
-    $this->sendSuccessResponse($notifications);
+    $this->sendSuccessResponse(array_values($notifications));
   }
 
   public function checkAll() {
@@ -83,7 +83,7 @@ function (Notification $notification) {
         return $this->notificationAcl->canViewDetail($notification);
       });
 
-    $this->sendSuccessResponse($notifications);
+    $this->sendSuccessResponse(array_values($notifications));
   }
 
   public function checkCreate() {
diff --git a/app/V1Module/security/Authorizator.php b/app/V1Module/security/Authorizator.php
index 966a5400f..7f16cac54 100644
--- a/app/V1Module/security/Authorizator.php
+++ b/app/V1Module/security/Authorizator.php
@@ -48,7 +48,7 @@ protected function checkPermissionsForRoleList($roleList, $resource, $privilege)
     return false;
   }
 
-  protected function isInRole($target, $role): bool {
-    return $this->roles->isInRole($target, $role);
+  protected function isInRole(string $actualTestedRole, string $minimalRequestedRole): bool {
+    return $this->roles->isInRole($actualTestedRole, $minimalRequestedRole);
   }
 }
diff --git a/app/V1Module/security/Policies/NotificationPermissionPolicy.php b/app/V1Module/security/Policies/NotificationPermissionPolicy.php
index 09e917b98..500449f81 100644
--- a/app/V1Module/security/Policies/NotificationPermissionPolicy.php
+++ b/app/V1Module/security/Policies/NotificationPermissionPolicy.php
@@ -27,7 +27,7 @@ public function hasRole(Identity $identity, Notification $notification) {
       return false;
     }
 
-    return $this->roles->isInRole($notification->getRole(), $user->getRole());
+    return $this->roles->isInRole($user->getRole(), $notification->getRole());
   }
 
   public function isGlobal(Identity $identity, Notification $notification) {
diff --git a/app/V1Module/security/Roles.php b/app/V1Module/security/Roles.php
index 519331170..061c17f33 100644
--- a/app/V1Module/security/Roles.php
+++ b/app/V1Module/security/Roles.php
@@ -14,23 +14,32 @@ abstract class Roles
   public const EMPOWERED_SUPERVISOR_ROLE = "empowered-supervisor";
   public const SUPERADMIN_ROLE = "superadmin";
 
-  protected $roles = [];
+  /**
+   * @var array
+   * Indices are role names, values holds a list of all parents (from which a role inherits permissions).
+   */
+  protected $rolesParents = [];
 
 
   public abstract function setup();
 
-  protected function addRole($role, $parents) {
-    $this->roles[$role] = $parents;
+  protected function addRole(string $role, array $parents) {
+    $this->rolesParents[$role] = $parents;
   }
 
-  public function isInRole($target, $role): bool {
-    if ($target === $role) {
+  /**
+   * Verify whether given actual role has at least the permissions of minimal requested role.
+   * @param string $actualTestedRole
+   * @param string $minimalRequestedRole
+   */
+  public function isInRole(string $actualTestedRole, string $minimalRequestedRole): bool {
+    if ($actualTestedRole === $minimalRequestedRole) {
       return true;
     }
 
-    if (array_key_exists($target, $this->roles)) {
-      foreach ($this->roles[$target] as $parent) {
-        if ($this->isInRole($parent, $role)) {
+    if (array_key_exists($actualTestedRole, $this->rolesParents)) {
+      foreach ($this->rolesParents[$actualTestedRole] as $parent) {
+        if ($this->isInRole($parent, $minimalRequestedRole)) {
           return true;
         }
       }
@@ -45,7 +54,7 @@ public function isInRole($target, $role): bool {
    * @return bool true if given role is valid
    */
   public function validateRole(string $role): bool {
-    if (array_key_exists($role, $this->roles)) {
+    if (array_key_exists($role, $this->rolesParents)) {
       return true;
     }