From beeeae3bb6cadf66ec87802c1e5eee6f51dd9dd0 Mon Sep 17 00:00:00 2001
From: Nick Muerdter <12112+GUI@users.noreply.github.com>
Date: Wed, 24 Jan 2024 18:38:26 -0700
Subject: [PATCH] Fixes and test coverage for returning proper groups for
 limited admins.

---
 .../web-app/actions/v1/admins.lua             |   4 +-
 src/api-umbrella/web-app/models/admin.lua     |  40 ++--
 .../web-app/policies/admin_policy.lua         |   8 +-
 test/apis/v1/admins/test_admin_permissions.rb | 174 +++++++++++++++++-
 test/factories/admin_groups.rb                |   3 +
 test/factories/admins.rb                      |   6 +
 6 files changed, 207 insertions(+), 28 deletions(-)

diff --git a/src/api-umbrella/web-app/actions/v1/admins.lua b/src/api-umbrella/web-app/actions/v1/admins.lua
index 8b732318f..6574b264f 100644
--- a/src/api-umbrella/web-app/actions/v1/admins.lua
+++ b/src/api-umbrella/web-app/actions/v1/admins.lua
@@ -63,7 +63,9 @@ function _M.index(self)
       "created_at",
       "updated_at",
     },
-    preload = { "groups" },
+    preload = {
+      "authorized_groups",
+    },
     csv_filename = "admins",
   })
 end
diff --git a/src/api-umbrella/web-app/models/admin.lua b/src/api-umbrella/web-app/models/admin.lua
index 0e189e433..c8e2972f3 100644
--- a/src/api-umbrella/web-app/models/admin.lua
+++ b/src/api-umbrella/web-app/models/admin.lua
@@ -100,6 +100,12 @@ Admin = model_ext.new_class("admins", {
       foreign_key = "admin_id",
       association_foreign_key = "admin_group_id",
       order = "name",
+    }),
+    model_ext.has_and_belongs_to_many("authorized_groups", "AdminGroup", {
+      join_table = "admin_groups_admins",
+      foreign_key = "admin_id",
+      association_foreign_key = "admin_group_id",
+      order = "name",
       transform_sql = function(sql)
         local scope_sql = admin_group_policy.authorized_query_scope(ngx.ctx.current_admin)
         if scope_sql then
@@ -178,26 +184,26 @@ Admin = model_ext.new_class("admins", {
     return decrypted
   end,
 
-  group_ids = function(self)
-    local group_ids = {}
-    local groups = self:get_groups()
+  authorized_group_ids = function(self)
+    local authorized_group_ids = {}
+    local groups = self:get_authorized_groups()
     for _, group in ipairs(groups) do
-      table.insert(group_ids, group.id)
+      table.insert(authorized_group_ids, group.id)
     end
 
-    return group_ids
+    return authorized_group_ids
   end,
 
-  group_names = function(self)
-    local group_names = {}
-    for _, group in ipairs(self:get_groups()) do
-      table.insert(group_names, group.name)
+  authorized_group_names = function(self)
+    local authorized_group_names = {}
+    for _, group in ipairs(self:get_authorized_groups()) do
+      table.insert(authorized_group_names, group.name)
     end
     if self.superuser then
-      table.insert(group_names, t("Superuser"))
+      table.insert(authorized_group_names, t("Superuser"))
     end
 
-    return group_names
+    return authorized_group_names
   end,
 
   group_permission_ids = function(self)
@@ -240,9 +246,9 @@ Admin = model_ext.new_class("admins", {
     return false
   end,
 
-  groups_as_json = function(self)
+  authorized_groups_as_json = function(self)
     local admin_groups = {}
-    for _, admin_group in ipairs(self:get_groups()) do
+    for _, admin_group in ipairs(self:get_authorized_groups()) do
       table.insert(admin_groups, admin_group:embedded_json())
     end
 
@@ -276,9 +282,9 @@ Admin = model_ext.new_class("admins", {
       updater = {
         username = json_null_default(self.updated_by_username),
       },
-      groups = json_null_default(self:groups_as_json()),
-      group_ids = json_null_default(self:group_ids()),
-      group_names = json_null_default(self:group_names()),
+      groups = json_null_default(self:authorized_groups_as_json()),
+      group_ids = json_null_default(self:authorized_group_ids()),
+      group_names = json_null_default(self:authorized_group_names()),
       deleted_at = json_null,
       version = 1,
     }
@@ -313,7 +319,7 @@ Admin = model_ext.new_class("admins", {
   as_csv = function(self)
     return {
       json_null_default(self.username),
-      json_null_default(table.concat(self:group_names(), "\n")),
+      json_null_default(table.concat(self:authorized_group_names(), "\n")),
       json_null_default(time.postgres_to_iso8601(self.current_sign_in_at)),
       json_null_default(time.postgres_to_iso8601(self.created_at)),
     }
diff --git a/src/api-umbrella/web-app/policies/admin_policy.lua b/src/api-umbrella/web-app/policies/admin_policy.lua
index bacfca113..104a65b03 100644
--- a/src/api-umbrella/web-app/policies/admin_policy.lua
+++ b/src/api-umbrella/web-app/policies/admin_policy.lua
@@ -69,14 +69,14 @@ function _M.is_authorized_show(current_admin, data, permission_id)
     return true
   end
 
-  if data["superuser"] and data["superuser"] ~= false then
-    return false
-  end
-
   if not permission_id then
     permission_id = "admin_view"
   end
 
+  if data["superuser"] and data["superuser"] ~= false and permission_id ~= "admin_view" then
+    return false
+  end
+
   local any_groups_allowed = false
   local all_groups_allowed = false
   if not is_empty(data["group_ids"]) then
diff --git a/test/apis/v1/admins/test_admin_permissions.rb b/test/apis/v1/admins/test_admin_permissions.rb
index 9cc961491..cf8748c3e 100644
--- a/test/apis/v1/admins/test_admin_permissions.rb
+++ b/test/apis/v1/admins/test_admin_permissions.rb
@@ -37,7 +37,7 @@ def test_multi_group_multi_scope_permitted_as_multi_scope_admin
     assert_admin_permitted(factory, admin)
   end
 
-  def test_multi_group_multi_scope_forbidden_as_single_scope_admin
+  def test_multi_group_multi_scope_permitted_read_forbidden_modify_as_single_scope_admin
     factory = :google_and_yahoo_multi_group_admin
 
     admin = FactoryBot.create(:limited_admin, :groups => [
@@ -45,14 +45,14 @@ def test_multi_group_multi_scope_forbidden_as_single_scope_admin
         ApiScope.find_or_create_by_instance!(FactoryBot.build(:google_api_scope)),
       ]),
     ])
-    assert_admin_forbidden(factory, admin)
+    assert_admin_permitted_read_forbidden_modify(factory, admin)
 
     admin = FactoryBot.create(:limited_admin, :groups => [
       FactoryBot.create(:admin_group, :api_scopes => [
         ApiScope.find_or_create_by_instance!(FactoryBot.build(:yahoo_api_scope)),
       ]),
     ])
-    assert_admin_forbidden(factory, admin)
+    assert_admin_permitted_read_forbidden_modify(factory, admin)
   end
 
   def test_single_group_multi_scope_permitted_as_superuser
@@ -72,7 +72,7 @@ def test_single_group_multi_scope_permitted_as_multi_scope_admin
     assert_admin_permitted(factory, admin)
   end
 
-  def test_single_group_multi_scope_forbidden_as_single_scope_admin
+  def test_single_group_multi_scope_permitted_read_forbidden_modify_as_single_scope_admin
     factory = :google_and_yahoo_single_group_admin
 
     admin = FactoryBot.create(:limited_admin, :groups => [
@@ -80,14 +80,14 @@ def test_single_group_multi_scope_forbidden_as_single_scope_admin
         ApiScope.find_or_create_by_instance!(FactoryBot.build(:google_api_scope)),
       ]),
     ])
-    assert_admin_forbidden(factory, admin)
+    assert_admin_permitted_read_forbidden_modify(factory, admin)
 
     admin = FactoryBot.create(:limited_admin, :groups => [
       FactoryBot.create(:admin_group, :api_scopes => [
         ApiScope.find_or_create_by_instance!(FactoryBot.build(:yahoo_api_scope)),
       ]),
     ])
-    assert_admin_forbidden(factory, admin)
+    assert_admin_permitted_read_forbidden_modify(factory, admin)
   end
 
   def test_superuser_as_superuser
@@ -297,6 +297,160 @@ def test_notes_only_visible_to_admin_managers_and_superusers
     refute_includes("Private notes", response.body)
   end
 
+  def test_only_returns_permitted_groups_even_if_other_groups_exist
+    superuser_admin = FactoryBot.create(:admin)
+    superuser_google_admin = FactoryBot.create(:google_admin, :superuser => true)
+    google_and_yahoo_multi_group_admin = FactoryBot.create(:google_and_yahoo_multi_group_admin)
+    google_and_yahoo_single_group_admin = FactoryBot.create(:google_and_yahoo_single_group_admin)
+    google_admin = FactoryBot.create(:google_admin)
+    yahoo_admin = FactoryBot.create(:yahoo_admin)
+    admins = [
+      superuser_admin,
+      superuser_google_admin,
+      google_and_yahoo_multi_group_admin,
+      google_and_yahoo_single_group_admin,
+      google_admin,
+      yahoo_admin,
+    ]
+
+    admins.each do |calling_admin|
+      # Make the request to fetch all of the admins as each different admin to
+      # see how the responses differ.
+      response = Typhoeus.get("https://127.0.0.1:9081/api-umbrella/v1/admins.json", http_options.deep_merge(admin_token(calling_admin)))
+      assert_response_code(200, response)
+      data = MultiJson.load(response.body)
+
+      # Verify the expected admins in the response depending on what admin is
+      # making the request.
+      admin_ids = data.fetch("data").map { |r| r["id"] }
+      case calling_admin
+      # Superusers should return all admins and all groups they belong to.
+      when superuser_admin
+        assert_equal([
+          superuser_admin.id,
+          superuser_google_admin.id,
+          google_and_yahoo_multi_group_admin.id,
+          google_and_yahoo_single_group_admin.id,
+          google_admin.id,
+          yahoo_admin.id,
+        ].sort, admin_ids.sort)
+
+      # Admins authorized to both scopes should return all admins in either
+      # scope.
+      when google_and_yahoo_multi_group_admin, google_and_yahoo_single_group_admin
+        assert_equal([
+          superuser_google_admin.id,
+          google_and_yahoo_multi_group_admin.id,
+          google_and_yahoo_single_group_admin.id,
+          google_admin.id,
+          yahoo_admin.id,
+        ].sort, admin_ids.sort)
+
+      # Admins only authorized to a single scope should return any admins with
+      # intersecting permissions, even if they won't have edit privileges on
+      # some admins that belong to groups they are not fully authorized to (like
+      # google_and_yahoo_multi_group_admin and
+      # google_and_yahoo_single_group_admin).
+      when google_admin
+        assert_equal([
+          superuser_google_admin.id,
+          google_and_yahoo_multi_group_admin.id,
+          google_and_yahoo_single_group_admin.id,
+          google_admin.id,
+        ].sort, admin_ids.sort)
+      when yahoo_admin
+        assert_equal([
+          google_and_yahoo_multi_group_admin.id,
+          google_and_yahoo_single_group_admin.id,
+          yahoo_admin.id,
+        ].sort, admin_ids.sort)
+      end
+
+      # Verify the data on each admin that's part of the API response.
+      data.fetch("data").each do |admin_data|
+        case admin_data.fetch("id")
+        when superuser_admin.id
+          groups = superuser_admin.groups
+          assert_equal(0, groups.length)
+          assert_equal([], admin_data.fetch("groups"))
+          assert_equal([], admin_data.fetch("group_ids"))
+          assert_equal(["Superuser"], admin_data.fetch("group_names"))
+
+        when superuser_google_admin.id
+          groups = superuser_google_admin.groups
+          assert_equal(1, groups.length)
+          assert_equal([{ "id" => groups.first.id, "name" => groups.first.name }], admin_data.fetch("groups"))
+          assert_equal([groups.first.id], admin_data.fetch("group_ids"))
+          assert_equal([groups.first.name, "Superuser"], admin_data.fetch("group_names"))
+
+        when google_and_yahoo_multi_group_admin.id
+          groups = google_and_yahoo_multi_group_admin.groups
+          assert_equal(2, groups.length)
+
+          # There are 2 groups, but for our more limited admins, it should only
+          # return the 1 that the calling admin is authorized to.
+          case calling_admin
+          when google_admin
+            assert_equal([{ "id" => groups.first.id, "name" => groups.first.name }], admin_data.fetch("groups"))
+            assert_match(/Google Admin Group/, admin_data.fetch("groups").first.fetch("name"))
+            assert_equal([groups.first.id], admin_data.fetch("group_ids"))
+            assert_equal([groups.first.name], admin_data.fetch("group_names"))
+            assert_match(/Google Admin Group/, admin_data.fetch("group_names").first)
+          when yahoo_admin
+            assert_equal([{ "id" => groups.last.id, "name" => groups.last.name }], admin_data.fetch("groups"))
+            assert_match(/Yahoo Admin Group/, admin_data.fetch("groups").first.fetch("name"))
+            assert_equal([groups.last.id], admin_data.fetch("group_ids"))
+            assert_equal([groups.last.name], admin_data.fetch("group_names"))
+            assert_match(/Yahoo Admin Group/, admin_data.fetch("group_names").first)
+          else
+            assert_equal(groups.map { |group| { "id" => group.id, "name" => group.name } }.sort_by { |g| g.fetch("id") }, admin_data.fetch("groups").sort_by { |g| g.fetch("id") })
+            assert_equal(groups.map { |group| group.id }.sort, admin_data.fetch("group_ids").sort)
+            assert_equal(groups.map { |group| group.name }.sort, admin_data.fetch("group_names").sort)
+          end
+
+        when google_and_yahoo_single_group_admin.id
+          groups = google_and_yahoo_single_group_admin.groups
+          assert_equal(1, groups.length)
+
+          # For single-scope admins, the admin can view this other admin (since
+          # its groups intersect), but it won't be able to update it or view its
+          # groups it belongs to (since this is not a group that the
+          # single-scope admin is fully authorized to).
+          case calling_admin
+          when google_admin, yahoo_admin
+            assert_equal([], admin_data.fetch("groups"))
+            assert_equal([], admin_data.fetch("group_ids"))
+            assert_equal([], admin_data.fetch("group_names"))
+          else
+            assert_equal([{ "id" => groups.first.id, "name" => groups.first.name }], admin_data.fetch("groups"))
+            assert_equal([groups.first.id], admin_data.fetch("group_ids"))
+            assert_equal([groups.first.name], admin_data.fetch("group_names"))
+          end
+        when google_admin.id
+          groups = google_admin.groups
+          assert_equal(1, groups.length)
+          assert_equal([{ "id" => groups.first.id, "name" => groups.first.name }], admin_data.fetch("groups"))
+          assert_equal([groups.first.id], admin_data.fetch("group_ids"))
+          assert_equal([groups.first.name], admin_data.fetch("group_names"))
+        when yahoo_admin.id
+          groups = yahoo_admin.groups
+          assert_equal(1, groups.length)
+          assert_equal([{ "id" => groups.first.id, "name" => groups.first.name }], admin_data.fetch("groups"))
+          assert_equal([groups.first.id], admin_data.fetch("group_ids"))
+          assert_equal([groups.first.name], admin_data.fetch("group_names"))
+        else
+          raise "Unknown admin id"
+        end
+
+        # Verify that the "show" API returns the same nested group data.
+        response = Typhoeus.get("https://127.0.0.1:9081/api-umbrella/v1/admins/#{admin_data.fetch("id")}.json", http_options.deep_merge(admin_token(calling_admin)))
+        assert_response_code(200, response)
+        show_data = MultiJson.load(response.body)
+        assert_equal(admin_data, show_data.fetch("admin"))
+      end
+    end
+  end
+
   private
 
   def assert_admin_permitted(factory, admin)
@@ -322,6 +476,14 @@ def assert_admin_forbidden(factory, admin)
     assert_admin_forbidden_destroy(factory, admin)
   end
 
+  def assert_admin_permitted_read_forbidden_modify(factory, admin)
+    assert_admin_permitted_index(factory, admin)
+    assert_admin_permitted_show(factory, admin)
+    assert_admin_forbidden_create(factory, admin)
+    assert_admin_forbidden_update(factory, admin)
+    assert_admin_forbidden_destroy(factory, admin)
+  end
+
   def assert_admin_permitted_index(factory, admin)
     record = FactoryBot.create(factory)
     response = Typhoeus.get("https://127.0.0.1:9081/api-umbrella/v1/admins.json", http_options.deep_merge(admin_token(admin)))
diff --git a/test/factories/admin_groups.rb b/test/factories/admin_groups.rb
index b10f04faf..fa876bdbd 100644
--- a/test/factories/admin_groups.rb
+++ b/test/factories/admin_groups.rb
@@ -55,14 +55,17 @@
     end
 
     factory :google_admin_group do
+      sequence(:name) { |n| "Google Admin Group #{n}" }
       api_scopes { [ApiScope.find_or_create_by_instance!(FactoryBot.build(:google_api_scope))] }
     end
 
     factory :yahoo_admin_group do
+      sequence(:name) { |n| "Yahoo Admin Group #{n}" }
       api_scopes { [ApiScope.find_or_create_by_instance!(FactoryBot.build(:yahoo_api_scope))] }
     end
 
     factory :google_and_yahoo_multi_scope_admin_group do
+      sequence(:name) { |n| "Google & Yahoo Admin Group #{n}" }
       api_scopes do
         [
           ApiScope.find_or_create_by_instance!(FactoryBot.build(:google_api_scope)),
diff --git a/test/factories/admins.rb b/test/factories/admins.rb
index 64ec45f48..fe6b7a104 100644
--- a/test/factories/admins.rb
+++ b/test/factories/admins.rb
@@ -24,6 +24,12 @@
         end
       end
 
+      factory :yahoo_admin do
+        groups do
+          [FactoryBot.create(:yahoo_admin_group)]
+        end
+      end
+
       factory :google_and_yahoo_multi_group_admin do
         groups do
           [