From 23ecc06a65bb44825ceda7f454dd5de362f6c696 Mon Sep 17 00:00:00 2001 From: Carlos Palhares Date: Fri, 11 Oct 2024 15:02:37 -0300 Subject: [PATCH] [HFH-2463] Fix updating user data (#398) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 2 +- .../audiences/contexts_controller.rb | 2 +- .../app/models/audiences/context_users.rb | 4 ++-- .../app/models/audiences/criterion_users.rb | 4 ++-- .../app/models/audiences/external_user.rb | 7 +++++++ audiences/docs/CHANGELOG.md | 5 +++++ audiences/gemfiles/rails_6_1.gemfile.lock | 2 +- audiences/gemfiles/rails_7_0.gemfile.lock | 2 +- audiences/gemfiles/rails_7_1.gemfile.lock | 2 +- audiences/lib/audiences.rb | 5 +++-- audiences/lib/audiences/scim/resource.rb | 4 ++++ audiences/lib/audiences/version.rb | 2 +- audiences/spec/lib/audiences_spec.rb | 16 +++++++++++----- audiences/spec/requests/contexts_api_spec.rb | 19 ++++++++----------- 14 files changed, 48 insertions(+), 28 deletions(-) diff --git a/audiences/Gemfile.lock b/audiences/Gemfile.lock index c8311aed..e8c209f9 100644 --- a/audiences/Gemfile.lock +++ b/audiences/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - audiences (1.3.0) + audiences (1.3.1) rails (>= 6.0) GEM diff --git a/audiences/app/controllers/audiences/contexts_controller.rb b/audiences/app/controllers/audiences/contexts_controller.rb index cef31842..50831e05 100644 --- a/audiences/app/controllers/audiences/contexts_controller.rb +++ b/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 diff --git a/audiences/app/models/audiences/context_users.rb b/audiences/app/models/audiences/context_users.rb index 277d675c..b1feb351 100644 --- a/audiences/app/models/audiences/context_users.rb +++ b/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 diff --git a/audiences/app/models/audiences/criterion_users.rb b/audiences/app/models/audiences/criterion_users.rb index 3e6e76f1..53dd57a5 100644 --- a/audiences/app/models/audiences/criterion_users.rb +++ b/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 diff --git a/audiences/app/models/audiences/external_user.rb b/audiences/app/models/audiences/external_user.rb index 75c3eec2..86e0577b 100644 --- a/audiences/app/models/audiences/external_user.rb +++ b/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? diff --git a/audiences/docs/CHANGELOG.md b/audiences/docs/CHANGELOG.md index 302922b5..7411660d 100644 --- a/audiences/docs/CHANGELOG.md +++ b/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) diff --git a/audiences/gemfiles/rails_6_1.gemfile.lock b/audiences/gemfiles/rails_6_1.gemfile.lock index f52bd0a2..a92c458d 100644 --- a/audiences/gemfiles/rails_6_1.gemfile.lock +++ b/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 diff --git a/audiences/gemfiles/rails_7_0.gemfile.lock b/audiences/gemfiles/rails_7_0.gemfile.lock index 12556a6f..c40d852a 100644 --- a/audiences/gemfiles/rails_7_0.gemfile.lock +++ b/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 diff --git a/audiences/gemfiles/rails_7_1.gemfile.lock b/audiences/gemfiles/rails_7_1.gemfile.lock index 80f4a2bf..ae1da9a9 100644 --- a/audiences/gemfiles/rails_7_1.gemfile.lock +++ b/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 diff --git a/audiences/lib/audiences.rb b/audiences/lib/audiences.rb index b75cc6ab..2bff4562 100644 --- a/audiences/lib/audiences.rb +++ b/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 diff --git a/audiences/lib/audiences/scim/resource.rb b/audiences/lib/audiences/scim/resource.rb index bce2193f..65c12949 100644 --- a/audiences/lib/audiences/scim/resource.rb +++ b/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 diff --git a/audiences/lib/audiences/version.rb b/audiences/lib/audiences/version.rb index e1024771..73b839c9 100644 --- a/audiences/lib/audiences/version.rb +++ b/audiences/lib/audiences/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module Audiences - VERSION = "1.3.0" + VERSION = "1.3.1" end diff --git a/audiences/spec/lib/audiences_spec.rb b/audiences/spec/lib/audiences_spec.rb index 32bead25..e82d3ca0 100644 --- a/audiences/spec/lib/audiences_spec.rb +++ b/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 diff --git a/audiences/spec/requests/contexts_api_spec.rb b/audiences/spec/requests/contexts_api_spec.rb index 94885758..bec63ae9 100644 --- a/audiences/spec/requests/contexts_api_spec.rb +++ b/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" => [ {