From 777027bc48d628f1283b0a734ffabe5dbabebc36 Mon Sep 17 00:00:00 2001
From: Dan Brown <ssddanbrown@googlemail.com>
Date: Sat, 10 Jun 2023 11:37:01 +0100
Subject: [PATCH] Permissions: Updated guest user handling so additional roles
 apply

Previously additional roles would only partially apply (system or "all"
permissions). This aligns the query-handling of permissions so that
additional roles will be used for permission queries.

Adds migration to detach existing roles as a safety precaution since
this is likely to widen permissions in scenarios that the public user
has other roles assigned already.

For #1229
---
 app/Permissions/PermissionApplicator.php      |  4 --
 ...1823_remove_guest_user_secondary_roles.php | 46 +++++++++++++++++++
 tests/PublicActionTest.php                    | 14 ++++++
 3 files changed, 60 insertions(+), 4 deletions(-)
 create mode 100644 database/migrations/2023_06_10_071823_remove_guest_user_secondary_roles.php

diff --git a/app/Permissions/PermissionApplicator.php b/app/Permissions/PermissionApplicator.php
index 8a02f82e8..b4fafaa9e 100644
--- a/app/Permissions/PermissionApplicator.php
+++ b/app/Permissions/PermissionApplicator.php
@@ -183,10 +183,6 @@ class PermissionApplicator
      */
     protected function getCurrentUserRoleIds(): array
     {
-        if (auth()->guest()) {
-            return [Role::getSystemRole('public')->id];
-        }
-
         return $this->currentUser()->roles->pluck('id')->values()->all();
     }
 
diff --git a/database/migrations/2023_06_10_071823_remove_guest_user_secondary_roles.php b/database/migrations/2023_06_10_071823_remove_guest_user_secondary_roles.php
new file mode 100644
index 000000000..8d04efdd9
--- /dev/null
+++ b/database/migrations/2023_06_10_071823_remove_guest_user_secondary_roles.php
@@ -0,0 +1,46 @@
+<?php
+
+use Illuminate\Database\Migrations\Migration;
+use Illuminate\Support\Facades\DB;
+
+return new class extends Migration
+{
+    /**
+     * Run the migrations.
+     *
+     * @return void
+     */
+    public function up()
+    {
+        $guestUserId = DB::table('users')
+            ->where('system_name', '=', 'public')
+            ->first(['id'])->id;
+        $publicRoleId = DB::table('roles')
+            ->where('system_name', '=', 'public')
+            ->first(['id'])->id;
+
+        // This migration deletes secondary "Guest" user role assignments
+        // as a safety precaution upon upgrade since the logic is changing
+        // within the release this is introduced in, which could result in wider
+        // permissions being provided upon upgrade without this intervention.
+
+        // Previously, added roles would only partially apply their permissions
+        // since some permission checks would only consider the originally assigned
+        // public role, and not added roles. Within this release, additional roles
+        // will fully apply.
+        DB::table('role_user')
+            ->where('user_id', '=', $guestUserId)
+            ->where('role_id', '!=', $publicRoleId)
+            ->delete();
+    }
+
+    /**
+     * Reverse the migrations.
+     *
+     * @return void
+     */
+    public function down()
+    {
+        // No structural changes to make, and we cannot know the role ids to re-assign.
+    }
+};
diff --git a/tests/PublicActionTest.php b/tests/PublicActionTest.php
index 97299f757..6f0e2f1d3 100644
--- a/tests/PublicActionTest.php
+++ b/tests/PublicActionTest.php
@@ -193,4 +193,18 @@ class PublicActionTest extends TestCase
         $resp->assertRedirect($book->getUrl());
         $this->followRedirects($resp)->assertSee($book->name);
     }
+
+    public function test_public_view_can_take_on_other_roles()
+    {
+        $this->setSettings(['app-public' => 'true']);
+        $newRole = $this->users->attachNewRole(User::getDefault(), []);
+        $page = $this->entities->page();
+        $this->permissions->disableEntityInheritedPermissions($page);
+        $this->permissions->addEntityPermission($page, ['view', 'update'], $newRole);
+
+        $resp = $this->get($page->getUrl());
+        $resp->assertOk();
+
+        $this->withHtml($resp)->assertLinkExists($page->getUrl('/edit'));
+    }
 }