[HFH-2463] Fix updating user data (#398)

Only allow `externalId` for setting `external_users`, then refetches all
information based on resource configuration.

- **Introduce Resource#all**
- **Refetch extra users based on externalId**

> On the [update action of the
contexts_controller](https://github.com/powerhome/audiences/blob/998c71aa031d16257e6ce8a7d1d736c6f20aac97/audiences/app/controllers/audiences/contexts_controller.rb#L9-L11),
clients can add extra_users to an audience.
> 
> Normally, the clients retrieve user data from SCIM before
adding/removing users from the extra_users collection. But we found that
users can modify details of other users by modifying the request params.
I tested it out locally and confirmed I could change a user’s display
name on the request params.
> 
> That change persisted in the extra_users column on the contexts table,
and the data column on the audiences_external_users table. I haven't
tested it but I believe clients could modify other user data in the same
manner, such as externalId, photos and job titles.

audiences/Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    audiences (1.3.0)
+    audiences (1.3.1)
       rails (>= 6.0)
 
 GEM

audiences/app/controllers/audiences/contexts_controller.rb

@@ -44,7 +44,7 @@ def context_params
       params.permit(
         :match_all,
         criteria: [groups: {}],
-        extra_users: Audiences.config.resources[:Users].attributes
+        extra_users: %i[externalId]
       ).to_h.symbolize_keys
     end
   end

audiences/app/models/audiences/context_users.rb

@@ -20,8 +20,8 @@ def each(...)
   private
 
     def all_users
-      users = Scim.resource(:Users).query
-      ExternalUser.wrap(users.all)
+      users = Scim.resource(:Users).all
+      ExternalUser.wrap(users)
     end
 
     def matching_users

audiences/app/models/audiences/criterion_users.rb

@@ -21,8 +21,8 @@ def each(...)
 
     def groups_users(group_ids)
       filter = group_ids.map { "groups.value eq #{_1}" }.join(" OR ")
-      users = Audiences::Scim.resource(:Users).query(filter: filter)
-      ExternalUser.wrap(users.all)
+      users = Audiences::Scim.resource(:Users).all(filter: filter)
+      ExternalUser.wrap(users)
     end
   end
 end

audiences/app/models/audiences/external_user.rb

@@ -10,6 +10,13 @@ class ExternalUser < ApplicationRecord
                             inverse_of: false
     end
 
+    def self.fetch(external_ids)
+      return [] unless external_ids.any?
+
+      filter = Array(external_ids).map { "externalId eq #{_1}" }.join(" OR ")
+      Audiences::Scim.resource(:Users).all(filter: filter)
+    end
+
     def self.wrap(resources)
       return [] unless resources&.any?
 

audiences/docs/CHANGELOG.md

@@ -1,5 +1,10 @@
 # Unreleased
 
+# Version 1.3.1 (2024-10-11)
+
+- Forward pagination parameters to SCIM on proxy [#397](https://github.com/powerhome/audiences/pull/397)
+- Fix security flaw when setting extra users [#398](https://github.com/powerhome/audiences/pull/398)
+
 # Version 1.3.0 (2024-09-03)
 
 - Filter out inactive users by default [#382](https://github.com/powerhome/audiences/pull/382)

audiences/gemfiles/rails_6_1.gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: ..
   specs:
-    audiences (1.3.0)
+    audiences (1.3.1)
       rails (>= 6.0)
 
 GEM

audiences/gemfiles/rails_7_0.gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: ..
   specs:
-    audiences (1.3.0)
+    audiences (1.3.1)
       rails (>= 6.0)
 
 GEM

audiences/gemfiles/rails_7_1.gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: ..
   specs:
-    audiences (1.3.0)
+    audiences (1.3.1)
       rails (>= 6.0)
 
 GEM

audiences/lib/audiences.rb

@@ -23,11 +23,12 @@ module Audiences
   # @param params [Hash] the updated params
   # @return Audience::Context
   #
-  def update(key, criteria: [], **attrs)
+  def update(key, criteria: [], extra_users: [], match_all: false)
     Audiences::Context.load(key) do |context|
       context.update!(
+        match_all: match_all,
         criteria: ::Audiences::Criterion.map(criteria),
-        **attrs
+        extra_users: ::Audiences::ExternalUser.fetch(extra_users.pluck("externalId"))
       )
       context.refresh_users!
     end

audiences/lib/audiences/scim/resource.rb

@@ -20,6 +20,10 @@ def query(**options)
                                         **@options, **options)
       end
 
+      def all(...)
+        query(...).all
+      end
+
       def scim_attributes
         @attributes.reduce([]) do |attrs, attr|
           case attr

audiences/lib/audiences/version.rb

@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 module Audiences
-  VERSION = "1.3.0"
+  VERSION = "1.3.1"
 end

audiences/spec/lib/audiences_spec.rb

@@ -17,12 +17,18 @@
       expect(updated_context).to be_match_all
     end
 
-    it "updates an direct resources collection" do
-      updated_context = Audiences.update(token, extra_users: [{ externalId: 678 }])
-      expect(updated_context.extra_users).to eql([{ "externalId" => 678 }])
+    it "updates extra users fetching latest information" do
+      stub_request(:get, "http://example.com/scim/v2/Users")
+        .with(query: {
+                attributes: "id,externalId,displayName,active,photos.type,photos.value",
+                filter: "(active eq true) and (externalId eq 678 OR externalId eq 321)",
+              })
+        .to_return(status: 200, body: { "Resources" => [{ "displayName" => "John", "externalId" => 678 },
+                                                        { "displayName" => "Doe", "externalId" => 321 }] }.to_json)
 
-      updated_context = Audiences.update(token, extra_users: [{ externalId: 123 }, { externalId: 321 }])
-      expect(updated_context.extra_users).to eql([{ "externalId" => 123 }, { "externalId" => 321 }])
+      updated_context = Audiences.update(token, extra_users: [{ "externalId" => 678 }, { "externalId" => 321 }])
+      expect(updated_context.extra_users).to eql([{ "displayName" => "John", "externalId" => 678 },
+                                                  { "displayName" => "Doe", "externalId" => 321 }])
     end
 
     it "updates group criterion" do

audiences/spec/requests/contexts_api_spec.rb

@@ -38,6 +38,13 @@
     end
 
     it "updates the context extra users" do
+      stub_request(:get, "http://example.com/scim/v2/Users")
+        .with(query: {
+                attributes: "id,externalId,displayName,active,photos.type,photos.value",
+                filter: "(active eq true) and (externalId eq 123)",
+              })
+        .to_return(status: 200, body: { "Resources" => [{ "displayName" => "John Doe", "externalId" => 123 }] }.to_json)
+
       put audience_context_path(example_owner, :members),
           as: :json,
           params: { extra_users: [{ externalId: 123, displayName: "John Doe",
@@ -48,23 +55,13 @@
       expect(context.extra_users).to eql [{
         "externalId" => 123,
         "displayName" => "John Doe",
-        "photos" => [{ "value" => "http://example.com" }],
       }]
-    end
-
-    it "responds with the audience context json" do
-      put audience_context_path(example_owner, :members),
-          as: :json,
-          params: { extra_users: [{ externalId: 123, displayName: "John Doe",
-                                    photos: [{ value: "http://example.com" }] }] }
-
       expect(response.parsed_body).to match({
                                               "match_all" => false,
                                               "count" => 1,
                                               "extra_users" => [{
                                                 "externalId" => 123,
                                                 "displayName" => "John Doe",
-                                                "photos" => [{ "value" => "http://example.com" }],
                                               }],
                                               "criteria" => [],
                                             })
@@ -106,7 +103,7 @@
 
         expect(response.parsed_body).to match({
                                                 "match_all" => false,
-                                                "extra_users" => nil,
+                                                "extra_users" => [],
                                                 "count" => 2,
                                                 "criteria" => [
                                                   {