From e2b9bf9cd26c2b7934df61e656489d977da08761 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivan=20Verg=C3=A9s?= Date: Thu, 17 Aug 2023 20:49:59 +0200 Subject: [PATCH] clear forbidden chars in names comming from oauth --- lib/decidim/civicrm.rb | 1 + lib/omniauth/strategies/civicrm.rb | 5 +++-- spec/fixtures/files/find_user_valid_response.json | 2 +- spec/forms/admin/event_meeting_form_spec.rb | 2 +- spec/forms/verifications/civicrm_groups_spec.rb | 2 +- .../verifications/civicrm_membership_types_spec.rb | 2 +- spec/forms/verifications/civicrm_spec.rb | 2 +- spec/jobs/auto_verification_job_spec.rb | 2 +- spec/jobs/event_sync_job_spec.rb | 2 +- spec/jobs/rebuild_verifications_job_spec.rb | 2 +- spec/jobs/sync_all_events_job_spec.rb | 2 +- spec/jobs/sync_all_groups_job_spec.rb | 2 +- spec/jobs/sync_event_registrations_job_spec.rb | 2 +- spec/jobs/sync_group_members_job_spec.rb | 2 +- spec/jobs/sync_membership_types_job_spec.rb | 2 +- spec/lib/civicrm/api/base/request_spec.rb | 2 +- spec/lib/civicrm/api/find_contact_spec.rb | 2 +- spec/lib/civicrm/api/find_event_spec.rb | 2 +- spec/lib/civicrm/api/find_group_spec.rb | 2 +- spec/lib/civicrm/api/find_user_spec.rb | 2 +- spec/lib/civicrm/api/list_contact_groups_spec.rb | 2 +- spec/lib/civicrm/api/list_contact_memberships_spec.rb | 2 +- spec/lib/civicrm/api/list_contacts_in_group_spec.rb | 2 +- spec/lib/civicrm/api/list_events_spec.rb | 2 +- spec/lib/civicrm/api/list_groups_spec.rb | 2 +- spec/lib/civicrm/api/list_membership_types_spec.rb | 2 +- spec/lib/civicrm_spec.rb | 8 ++++++++ spec/lib/event_parsers/event_meeting_parser_spec.rb | 2 +- .../event_parsers/event_registration_parser_spec.rb | 2 +- spec/omni_auth/strategies/civicrm_spec.rb | 10 +++++++++- .../civicrm/test => spec/shared}/shared_contexts.rb | 0 spec/system/login_data_spec.rb | 3 ++- 32 files changed, 49 insertions(+), 30 deletions(-) rename {lib/decidim/civicrm/test => spec/shared}/shared_contexts.rb (100%) diff --git a/lib/decidim/civicrm.rb b/lib/decidim/civicrm.rb index aaf7260..e73ddf5 100644 --- a/lib/decidim/civicrm.rb +++ b/lib/decidim/civicrm.rb @@ -119,6 +119,7 @@ def self.allow_unauthorized_path?(path) end def self.unauthorized_url + return Civicrm.unauthorized_redirect_url if Civicrm.unauthorized_redirect_url&.starts_with?("http") return Civicrm.unauthorized_redirect_url if Civicrm.allow_unauthorized_path?(Civicrm.unauthorized_redirect_url) "/authorizations/first_login" diff --git a/lib/omniauth/strategies/civicrm.rb b/lib/omniauth/strategies/civicrm.rb index 0ed4347..72fcbd8 100644 --- a/lib/omniauth/strategies/civicrm.rb +++ b/lib/omniauth/strategies/civicrm.rb @@ -6,6 +6,7 @@ module OmniAuth module Strategies class Civicrm < OmniAuth::Strategies::OAuth2 + REGEXP_SANITIZER = /[<>?%&\^*#@()\[\]=+:;"{}\\|]/ args [:client_id, :client_secret, :site] option :name, "civicrm" # do not symbolize this @@ -20,7 +21,8 @@ class Civicrm < OmniAuth::Strategies::OAuth2 info do { - name: extra[:contact][:display_name], + # sometimes the name can have special characters that are not allowed in the name validator + name: extra[:contact][:display_name].gsub(REGEXP_SANITIZER, ""), nickname: sanitized_nickname, email: raw_info["email"], image: raw_info["picture"] @@ -57,7 +59,6 @@ def civicrm_info end def sanitized_nickname - # TODO: restrict nicknamize to the current organization Decidim::UserBaseEntity.nicknamize(raw_info["preferred_username"] || raw_info["email"]) end end diff --git a/spec/fixtures/files/find_user_valid_response.json b/spec/fixtures/files/find_user_valid_response.json index 0f8841e..5977046 100644 --- a/spec/fixtures/files/find_user_valid_response.json +++ b/spec/fixtures/files/find_user_valid_response.json @@ -16,7 +16,7 @@ "id": 9999, "values": [ { - "display_name": "Arthur Dent" + "display_name": "(Arthur) Dent" } ] }, diff --git a/spec/forms/admin/event_meeting_form_spec.rb b/spec/forms/admin/event_meeting_form_spec.rb index 8b58f72..4e296c9 100644 --- a/spec/forms/admin/event_meeting_form_spec.rb +++ b/spec/forms/admin/event_meeting_form_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim::Civicrm module Admin diff --git a/spec/forms/verifications/civicrm_groups_spec.rb b/spec/forms/verifications/civicrm_groups_spec.rb index 0fd0b21..7ecf8db 100644 --- a/spec/forms/verifications/civicrm_groups_spec.rb +++ b/spec/forms/verifications/civicrm_groups_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim::Civicrm module Verifications diff --git a/spec/forms/verifications/civicrm_membership_types_spec.rb b/spec/forms/verifications/civicrm_membership_types_spec.rb index 2984bb5..ce9335a 100644 --- a/spec/forms/verifications/civicrm_membership_types_spec.rb +++ b/spec/forms/verifications/civicrm_membership_types_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim::Civicrm module Verifications diff --git a/spec/forms/verifications/civicrm_spec.rb b/spec/forms/verifications/civicrm_spec.rb index eedca37..6eef654 100644 --- a/spec/forms/verifications/civicrm_spec.rb +++ b/spec/forms/verifications/civicrm_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim::Civicrm module Verifications diff --git a/spec/jobs/auto_verification_job_spec.rb b/spec/jobs/auto_verification_job_spec.rb index 1083ad8..54c5adb 100644 --- a/spec/jobs/auto_verification_job_spec.rb +++ b/spec/jobs/auto_verification_job_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim::Civicrm describe AutoVerificationJob do diff --git a/spec/jobs/event_sync_job_spec.rb b/spec/jobs/event_sync_job_spec.rb index e778d71..1202e29 100644 --- a/spec/jobs/event_sync_job_spec.rb +++ b/spec/jobs/event_sync_job_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim::Civicrm describe EventSyncJob do diff --git a/spec/jobs/rebuild_verifications_job_spec.rb b/spec/jobs/rebuild_verifications_job_spec.rb index c17e3de..9059336 100644 --- a/spec/jobs/rebuild_verifications_job_spec.rb +++ b/spec/jobs/rebuild_verifications_job_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim::Civicrm describe RebuildVerificationsJob do diff --git a/spec/jobs/sync_all_events_job_spec.rb b/spec/jobs/sync_all_events_job_spec.rb index ca29e31..5290d20 100644 --- a/spec/jobs/sync_all_events_job_spec.rb +++ b/spec/jobs/sync_all_events_job_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim::Civicrm describe SyncAllEventsJob do diff --git a/spec/jobs/sync_all_groups_job_spec.rb b/spec/jobs/sync_all_groups_job_spec.rb index 1201bb3..574b30d 100644 --- a/spec/jobs/sync_all_groups_job_spec.rb +++ b/spec/jobs/sync_all_groups_job_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim::Civicrm describe SyncAllGroupsJob do diff --git a/spec/jobs/sync_event_registrations_job_spec.rb b/spec/jobs/sync_event_registrations_job_spec.rb index b60836d..38032ed 100644 --- a/spec/jobs/sync_event_registrations_job_spec.rb +++ b/spec/jobs/sync_event_registrations_job_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim::Civicrm describe SyncEventRegistrationsJob do diff --git a/spec/jobs/sync_group_members_job_spec.rb b/spec/jobs/sync_group_members_job_spec.rb index 8027aa7..27fdd86 100644 --- a/spec/jobs/sync_group_members_job_spec.rb +++ b/spec/jobs/sync_group_members_job_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim::Civicrm describe SyncGroupMembersJob do diff --git a/spec/jobs/sync_membership_types_job_spec.rb b/spec/jobs/sync_membership_types_job_spec.rb index 5086d14..81f6969 100644 --- a/spec/jobs/sync_membership_types_job_spec.rb +++ b/spec/jobs/sync_membership_types_job_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim::Civicrm describe SyncMembershipTypesJob do diff --git a/spec/lib/civicrm/api/base/request_spec.rb b/spec/lib/civicrm/api/base/request_spec.rb index 2a802af..735cf1e 100644 --- a/spec/lib/civicrm/api/base/request_spec.rb +++ b/spec/lib/civicrm/api/base/request_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim describe Civicrm::Api::Base::Request, type: :class do diff --git a/spec/lib/civicrm/api/find_contact_spec.rb b/spec/lib/civicrm/api/find_contact_spec.rb index b6ed30e..617855a 100644 --- a/spec/lib/civicrm/api/find_contact_spec.rb +++ b/spec/lib/civicrm/api/find_contact_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim describe Civicrm::Api::FindContact, type: :class do diff --git a/spec/lib/civicrm/api/find_event_spec.rb b/spec/lib/civicrm/api/find_event_spec.rb index f43622e..1083dc6 100644 --- a/spec/lib/civicrm/api/find_event_spec.rb +++ b/spec/lib/civicrm/api/find_event_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim describe Civicrm::Api::FindEvent, type: :class do diff --git a/spec/lib/civicrm/api/find_group_spec.rb b/spec/lib/civicrm/api/find_group_spec.rb index a8b0dac..e6b65bf 100644 --- a/spec/lib/civicrm/api/find_group_spec.rb +++ b/spec/lib/civicrm/api/find_group_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim describe Civicrm::Api::FindGroup, type: :class do diff --git a/spec/lib/civicrm/api/find_user_spec.rb b/spec/lib/civicrm/api/find_user_spec.rb index c275ab7..7e92564 100644 --- a/spec/lib/civicrm/api/find_user_spec.rb +++ b/spec/lib/civicrm/api/find_user_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim describe Civicrm::Api::FindUser, type: :class do diff --git a/spec/lib/civicrm/api/list_contact_groups_spec.rb b/spec/lib/civicrm/api/list_contact_groups_spec.rb index f7c319a..d5f45fe 100644 --- a/spec/lib/civicrm/api/list_contact_groups_spec.rb +++ b/spec/lib/civicrm/api/list_contact_groups_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim describe Civicrm::Api::ListContactGroups, type: :class do diff --git a/spec/lib/civicrm/api/list_contact_memberships_spec.rb b/spec/lib/civicrm/api/list_contact_memberships_spec.rb index e741bb7..958675c 100644 --- a/spec/lib/civicrm/api/list_contact_memberships_spec.rb +++ b/spec/lib/civicrm/api/list_contact_memberships_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim describe Civicrm::Api::ListContactMemberships, type: :class do diff --git a/spec/lib/civicrm/api/list_contacts_in_group_spec.rb b/spec/lib/civicrm/api/list_contacts_in_group_spec.rb index c17390b..3bb6190 100644 --- a/spec/lib/civicrm/api/list_contacts_in_group_spec.rb +++ b/spec/lib/civicrm/api/list_contacts_in_group_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim describe Civicrm::Api::ListContactsInGroup, type: :class do diff --git a/spec/lib/civicrm/api/list_events_spec.rb b/spec/lib/civicrm/api/list_events_spec.rb index 0576da9..b67101c 100644 --- a/spec/lib/civicrm/api/list_events_spec.rb +++ b/spec/lib/civicrm/api/list_events_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim describe Civicrm::Api::ListEvents, type: :class do diff --git a/spec/lib/civicrm/api/list_groups_spec.rb b/spec/lib/civicrm/api/list_groups_spec.rb index 4eb27e0..dc80792 100644 --- a/spec/lib/civicrm/api/list_groups_spec.rb +++ b/spec/lib/civicrm/api/list_groups_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim describe Civicrm::Api::ListGroups, type: :class do diff --git a/spec/lib/civicrm/api/list_membership_types_spec.rb b/spec/lib/civicrm/api/list_membership_types_spec.rb index 340c8c6..6194643 100644 --- a/spec/lib/civicrm/api/list_membership_types_spec.rb +++ b/spec/lib/civicrm/api/list_membership_types_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim describe Civicrm::Api::ListMembershipTypes, type: :class do diff --git a/spec/lib/civicrm_spec.rb b/spec/lib/civicrm_spec.rb index 8f8dc9e..3828f7b 100644 --- a/spec/lib/civicrm_spec.rb +++ b/spec/lib/civicrm_spec.rb @@ -39,5 +39,13 @@ module Decidim end end end + + context "when using a full url" do + let(:unauthorized_redirect_url) { "https://example.com/authorizations/first_login" } + + it "uses the specified url" do + expect(Civicrm.unauthorized_url).to eq(unauthorized_redirect_url) + end + end end end diff --git a/spec/lib/event_parsers/event_meeting_parser_spec.rb b/spec/lib/event_parsers/event_meeting_parser_spec.rb index 69f8e58..ca362d0 100644 --- a/spec/lib/event_parsers/event_meeting_parser_spec.rb +++ b/spec/lib/event_parsers/event_meeting_parser_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim::Civicrm describe EventParsers::EventMeetingParser, type: :class do diff --git a/spec/lib/event_parsers/event_registration_parser_spec.rb b/spec/lib/event_parsers/event_registration_parser_spec.rb index 3c03be2..1ecbdf8 100644 --- a/spec/lib/event_parsers/event_registration_parser_spec.rb +++ b/spec/lib/event_parsers/event_registration_parser_spec.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "spec_helper" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" module Decidim::Civicrm describe EventParsers::EventRegistrationParser, type: :class do diff --git a/spec/omni_auth/strategies/civicrm_spec.rb b/spec/omni_auth/strategies/civicrm_spec.rb index fcb254c..6b8d63c 100644 --- a/spec/omni_auth/strategies/civicrm_spec.rb +++ b/spec/omni_auth/strategies/civicrm_spec.rb @@ -3,7 +3,7 @@ require "spec_helper" require "omniauth" require "omniauth/test" -require "decidim/civicrm/test/shared_contexts" +require "shared/shared_contexts" RSpec.configure do |config| config.extend OmniAuth::Test::StrategyMacros, type: :strategy @@ -98,5 +98,13 @@ expect(subject.info[:nickname]).to eq("foo_bar_2") end end + + context "when the same user exists" do + let!(:existing_user) { create :user, nickname: "foo_bar", email: "bar@example.com" } + + it "returns a new valid nickname" do + expect(subject.info[:nickname]).to eq("foo_bar_2") + end + end end end diff --git a/lib/decidim/civicrm/test/shared_contexts.rb b/spec/shared/shared_contexts.rb similarity index 100% rename from lib/decidim/civicrm/test/shared_contexts.rb rename to spec/shared/shared_contexts.rb diff --git a/spec/system/login_data_spec.rb b/spec/system/login_data_spec.rb index c3e1638..5880a94 100644 --- a/spec/system/login_data_spec.rb +++ b/spec/system/login_data_spec.rb @@ -32,7 +32,8 @@ let(:last_user) { Decidim::User.last } let(:authorization) { Decidim::Authorization.find_by(name: handler) } - let(:user) { create(:user, :confirmed, name: "My Name", email: "my-email@example.org", organization: organization) } + let!(:existing_user) { create(:user, :confirmed, name: "Existing Name", nickname: "civicrm_user", email: "existing-email@example.org", organization: organization) } + let(:user) { create(:user, :confirmed, name: "My Name", nickname: "civicrm_user", email: "my-email@example.org", organization: organization) } let!(:identity) { create(:identity, user: user, provider: Decidim::Civicrm::OMNIAUTH_PROVIDER_NAME, uid: "12345") } let!(:group) { create :civicrm_group, organization: organization, civicrm_group_id: 3 } let!(:membership_type) { create :civicrm_membership_type, organization: organization, civicrm_membership_type_id: 3 }