[C] Update and fix failing tests

timfrazee · timfrazee · commit 6e6c22985329 · 2026-03-18T14:55:43.000-07:00
diff --git a/api/app/controllers/oauth_controller.rb b/api/app/controllers/oauth_controller.rb
@@ -58,6 +58,8 @@ def redirect_resource_query_string
   end
 
   def redirect_body
+    scrubbed_provider_name = Loofah.scrub_html5_fragment(params[:provider], :prune).to_s
+
     <<~HEREDOC
       <!DOCTYPE html>
       <html>
@@ -68,7 +70,7 @@ def redirect_body
         <body>
           <div style="margin-top: 100px; text-align: center;">
             <h1>Redirecting, Please Wait</h1>
-            <form id="auth_redirect_form" action="/auth/#{params[:provider]}" method="POST">
+            <form id="auth_redirect_form" action="/auth/#{scrubbed_provider_name}" method="POST">
               <input type="hidden" name="authenticity_token" value="#{session["_csrf_token"]}" />
               <input type="submit" id="auth_redirect_submit" value="Click here if you're not automatically redirected" />
             </form>
diff --git a/api/app/models/user_group.rb b/api/app/models/user_group.rb
@@ -14,6 +14,10 @@ class UserGroup < ApplicationRecord
   validates :name, presence: true, uniqueness: true
 
   def entitlement_subjects
-    entitleables.includes(:entitleable).map(&:entitleable).compact
+    entitleables.map(&:entitleable).compact
+  end
+
+  def sync_member_entitlements!
+    UserGroups::SyncMemberEntitlements.new.call(self)
   end
 end
diff --git a/api/app/models/user_group_entitleable.rb b/api/app/models/user_group_entitleable.rb
@@ -10,6 +10,8 @@ class UserGroupEntitleable < ApplicationRecord
   attr_accessor :target_url
 
   before_validation :maybe_derive_entitleable
+  after_create :sync_member_entitlements!
+  after_destroy :sync_member_entitlements!
 
   private
 
@@ -21,4 +23,8 @@ def maybe_derive_entitleable
     self.entitleable = nil
     Rils.logger.warn("Entitleable not found for GID #{target_url}")
   end
+
+  def sync_member_entitlements!
+    user_group.sync_member_entitlements!
+  end
 end
diff --git a/api/app/models/user_group_membership.rb b/api/app/models/user_group_membership.rb
@@ -10,7 +10,13 @@ class UserGroupMembership < ApplicationRecord
 
   belongs_to :source, polymorphic: true, optional: true
 
+  after_create :sync_entitlements!
+
   def name
     "#{user_group.name} membership for #{user.name}"
   end
+
+  def sync_entitlements!
+    UserGroupMemberships::SyncEntitlements.new.call(self)
+  end
 end
diff --git a/api/app/operations/identities/sync_managed_entitlements.rb b/api/app/operations/identities/sync_managed_entitlements.rb
@@ -18,7 +18,9 @@ def call(identity, auth_hash)
       @auth_hash = auth_hash
 
       to_remove.each do |entitleable|
-        Entitlement.by_entitling_entity(identity).by_subject(entitleable).destroy_all
+        Entitlement.by_entitling_entity(identity)
+                   .by_subject(entitleable)
+                   .destroy_all
       end
 
       to_add.each do |entitleable|
diff --git a/api/app/operations/user_group_memberships/sync_entitlements.rb b/api/app/operations/user_group_memberships/sync_entitlements.rb
@@ -0,0 +1,65 @@
+# frozen_string_literal: true
+
+module UserGroupMemberships
+  class SyncEntitlements
+    include Dry::Monads[:result, :do]
+
+    attr_reader :membership
+
+    delegate :user, :user_group, to: :membership
+
+    # @param [User] user
+    # @return [Dry::Monads::Result]
+    def call(membership)
+      @membership = membership
+
+      destroy_excess_entitlements
+      create_missing_entitlements
+
+      return Success()
+    end
+
+    def create_missing_entitlements
+      subjects = entitlements_to_create.map { _1[:subject_type].constantize.find _1[:subject_id] }
+      subjects.each do |subject|
+        Entitlements::Create.run(
+          subject:,
+          target: user,
+          entitling_entity: membership,
+          scoped_roles: { read_access: true }
+        )
+      end
+    end
+
+    def destroy_excess_entitlements
+      entitlements_to_destroy.map do
+        Entitlement.by_entitling_entity(membership).where(**_1)
+      end.reduce do |memo, query|
+        memo.or(query)
+      end&.destroy_all
+    end
+
+    def entitlements_to_create
+      expected_entitlements - existing_entitlements
+    end
+
+    def entitlements_to_destroy
+      existing_entitlements - expected_entitlements
+    end
+
+    def existing_entitlements
+      @existing_entitlements ||= Entitlement.where(
+        target: user,
+        entitler: membership.to_upsertable_entitler
+      ).map do
+        { subject_type: _1.subject_type, subject_id: _1.subject_id }
+      end
+    end
+
+    def expected_entitlements
+      @expected_entitlements ||= user_group.entitleables.reload.map do
+        { subject_type: _1.entitleable_type, subject_id: _1.entitleable_id }
+      end
+    end
+  end
+end
diff --git a/api/app/operations/user_groups/add_member.rb b/api/app/operations/user_groups/add_member.rb
@@ -3,9 +3,6 @@
 module UserGroups
   class AddMember
     include Dry::Monads[:result, :do]
-    include ManifoldApi::Deps[
-      upsert_user_group_entitlements: "user_groups.upsert_entitlements",
-    ]
 
     include UserGroups::ParsesIdentity
 
diff --git a/api/app/operations/user_groups/sync_member_entitlements.rb b/api/app/operations/user_groups/sync_member_entitlements.rb
@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+module UserGroups
+  class SyncMemberEntitlements
+    include Dry::Monads[:result, :do]
+
+    attr_reader :user_group
+
+    delegate :user, :memberships, to: :user_group
+
+    # @param [UserGroup] user_group
+    # @return [Dry::Monads::Result]
+    def call(user_group)
+      user_group.memberships.each do |membership|
+        UserGroupMemberships::SyncEntitlements.new.call(membership)
+      end
+
+      Success()
+    end
+  end
+end
diff --git a/api/app/operations/user_groups/upsert_entitlements.rb b/api/app/operations/user_groups/upsert_entitlements.rb
diff --git a/api/app/serializers/v1/concerns/project_serializer.rb b/api/app/serializers/v1/concerns/project_serializer.rb
@@ -39,7 +39,7 @@ module ProjectSerializer
 
         when_full do
           metadata(metadata: true, properties: true, formatted: true)
-          typed_attribute :external_identifier, Types::String do |object, params|
+          typed_attribute :external_identifier, Types::String.optional do |object, params|
             object.external_identifier&.identifier
           end
           typed_attribute :hero_styles, Types::Serializer::Attachment.meta(read_only: true)
diff --git a/api/app/serializers/v1/journal_serializer.rb b/api/app/serializers/v1/journal_serializer.rb
@@ -49,7 +49,7 @@ class JournalSerializer < ManifoldSerializer
       typed_attribute :image_credits, Types::String.optional
       typed_attribute :image_credits_formatted, Types::String.meta(read_only: true)
       typed_attribute :pending_slug, Types::String
-      typed_attribute :external_identifier, Types::String do |object, params|
+      typed_attribute :external_identifier, Types::String.optional do |object, params|
         object.external_identifier&.identifier
       end
 
diff --git a/api/app/serializers/v1/project_collection_serializer.rb b/api/app/serializers/v1/project_collection_serializer.rb
@@ -45,7 +45,7 @@ class ProjectCollectionSerializer < ManifoldSerializer
     typed_attribute :social_description, Types::String.optional
     typed_attribute :social_title, Types::String.optional
     typed_attribute :social_image_styles, Types::Serializer::Attachment.meta(read_only: true)
-    typed_attribute :external_identifier, Types::String do |object, params|
+    typed_attribute :external_identifier, Types::String.optional do |object, params|
       object.external_identifier&.identifier
     end
 
diff --git a/api/app/serializers/v1/user_group_serializer.rb b/api/app/serializers/v1/user_group_serializer.rb
@@ -9,7 +9,7 @@ class UserGroupSerializer < ManifoldSerializer
     typed_attribute :id, Types::Serializer::ID
     typed_attribute :created_at, Types::DateTime.meta(read_only: true)
     typed_attribute :name, Types::String.meta(example: "Manifold SAML users")
-    typed_attribute :external_identifier, Types::String do |object, params|
+    typed_attribute :external_identifier, Types::String.optional do |object, params|
       object.external_identifier&.identifier
     end
 
diff --git a/api/config/routes.rb b/api/config/routes.rb
@@ -26,6 +26,9 @@
   # Omniauth
   get "auth/:provider/redirect", to: "oauth#redirect"
   post "auth/:provider/callback", to: "oauth#authorize"
+
+  # Omniauth tests issue a redirect to the callback, and require a GET endpoint
+  # Real world callbacks will be POSTs
   get "auth/:provider/callback", to: "oauth#authorize" if Rails.env.test?
 
   namespace :api do
diff --git a/api/spec/models/user_group_entitleable_spec.rb b/api/spec/models/user_group_entitleable_spec.rb
@@ -3,5 +3,26 @@
 require 'rails_helper'
 
 RSpec.describe UserGroupEntitleable, type: :model do
-  pending "add some examples to (or delete) #{__FILE__}"
+  let(:project) { FactoryBot.create(:project) }
+  let(:user) { FactoryBot.create(:user) }
+
+  let!(:user_group) { FactoryBot.create(:user_group) }
+
+  let(:user_group_entitleable) { FactoryBot.create(:user_group_entitleable, user_group:, entitleable: project) }
+  let(:user_group_membership) { FactoryBot.create(:user_group_membership, user_group:, user:) }
+
+  it "creates entitlements for members when an entitleable is added" do
+    user_group_membership
+
+    expect do
+      user_group_entitleable
+    end.to change(Entitlement, :count).by(1)
+  end
+
+  it "destroys entitlements upon its destruction" do
+    user_group_membership
+    user_group_entitleable
+
+    expect { user_group_entitleable.destroy }.to change(Entitlement, :count).by(-1)
+  end
 end
diff --git a/api/spec/models/user_group_membership_spec.rb b/api/spec/models/user_group_membership_spec.rb
@@ -19,6 +19,6 @@
   it "destroys entitlements upon its destruction" do
     user_group_membership
 
-    expect { user_group_membership.destroy }.to change(Entitlement, :count).by(1)
+    expect { user_group_membership.destroy }.to change(Entitlement, :count).by(-1)
   end
 end
diff --git a/api/spec/models/user_group_spec.rb b/api/spec/models/user_group_spec.rb
@@ -3,5 +3,26 @@
 require 'rails_helper'
 
 RSpec.describe UserGroup, type: :model do
-  pending "add some examples to (or delete) #{__FILE__}"
+  let(:project) { FactoryBot.create(:project) }
+  let(:user) { FactoryBot.create(:user) }
+
+  let!(:user_group) { FactoryBot.create(:user_group) }
+
+  let(:user_group_entitleable) { FactoryBot.create(:user_group_entitleable, user_group:, entitleable: project) }
+  let(:user_group_membership) { FactoryBot.create(:user_group_membership, user_group:, user:) }
+
+  it "creates entitlements for members when an entitleable is added" do
+    user_group_membership
+
+    expect do
+      user_group_entitleable
+    end.to change(Entitlement, :count).by(1)
+  end
+
+  it "destroys entitlements upon its destruction" do
+    user_group_membership
+    user_group_entitleable
+
+    expect { user_group.destroy }.to change(Entitlement, :count).by(-1)
+  end
 end
diff --git a/api/spec/requests/oauth_spec.rb b/api/spec/requests/oauth_spec.rb
@@ -123,6 +123,7 @@
     before do
       allow(SamlConfig).to receive(:provider_names).and_return([provider.to_s])
       allow_any_instance_of(SamlConfig).to receive(:provider_names).and_return([provider.to_s])
+      allow_any_instance_of(AbstractSamlProviderConfig).to receive(:enabled?).and_return(true)
 
       # Clear any cached config
       SamlConfig.instance_variable_set("@instance", nil)
diff --git a/api/spec/requests/user_groups/relationships/user_group_memberships_spec.rb b/api/spec/requests/user_groups/relationships/user_group_memberships_spec.rb
@@ -3,12 +3,12 @@
 RSpec.describe "User Group Memberships API", type: :request do
   let!(:user_group) { FactoryBot.create(:user_group) }
   let!(:user) { FactoryBot.create(:user) }
-  let!(:user_group_membership) { FactoryBot.creat(:user_group_membership) }
+  let!(:user_group_membership) { FactoryBot.create(:user_group_membership, user_group:) }
 
   base_path = "/api/v1/user_groups/:user_group_id/relationships/user_group_memberships"
 
-  let(:path) { api_v1_user_group_relationships_user_group_memberships(user_group_id: user_group.id) }
-  let(:membership_path) { api_v1_user_group_relationships_user_group_membership(user_group_id: user_group.id, id: user_group_membership.id) }
+  let(:path) { api_v1_user_group_relationships_user_group_memberships_path(user_group_id: user_group.id) }
+  let(:membership_path) { api_v1_user_group_relationships_user_group_membership_path(user_group_id: user_group.id, id: user_group_membership.id) }
 
   let(:params) do
     {
@@ -32,6 +32,7 @@
         end.to change(UserGroupMembership, :count).by(1)
         expect(response).to have_http_status(:created)
       end
+
     end
 
     context "when the user is a reader" do
diff --git a/api/spec/requests/user_groups_spec.rb b/api/spec/requests/user_groups_spec.rb
@@ -35,9 +35,6 @@ def expect_making_the_request(method: request_method, path: request_path, header
         build_json_payload(
           attributes: {
             name:
-          }, relationships: {
-            members: [{ id: user.id }],
-            entitleables: [{ id: entitleable.id }]
           }
         )
       end
