From ffc3da35a6e6c1ec06bbd378d4cc64dfdae3acbf Mon Sep 17 00:00:00 2001 From: Sam Date: Tue, 10 Nov 2020 21:40:41 +1100 Subject: [PATCH] FIX: handle rapid concurrent SSO attempts more gracefully (#11180) Rapid concurrent SSO attempts is something that happens quite frequently in the wild at large enough scale. When this happens conditions such as adding a user to a group could possibly fire concurrently causing a user to be added to the same group twice and erroring out. To avoid all concurrency issues here we protect with a coarse distributed mutex. This heavily mitigates the risk around concurrent group additions and concurrent updates to user related records. --- app/models/discourse_single_sign_on.rb | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/app/models/discourse_single_sign_on.rb b/app/models/discourse_single_sign_on.rb index ab8c6c6a254..ab43a49f79e 100644 --- a/app/models/discourse_single_sign_on.rb +++ b/app/models/discourse_single_sign_on.rb @@ -64,6 +64,19 @@ class DiscourseSingleSignOn < SingleSignOn raise BannedExternalId, external_id end + # we protect here to ensure there is no situation where the same external id + # concurrently attempts to create or update sso records + # + # we can get duplicate HTTP requests quite easily (client rapid refresh) and this path does stuff such + # as updating groups for a users and so on that can happen even after the sso record and user is there + DistributedMutex.synchronize("sso_lookup_or_create_user_#{external_id}") do + lookup_or_create_user_unsafe(ip_address) + end + end + + private + + def lookup_or_create_user_unsafe(ip_address) sso_record = SingleSignOnRecord.find_by(external_id: external_id) if sso_record && (user = sso_record.user) @@ -135,8 +148,6 @@ class DiscourseSingleSignOn < SingleSignOn sso_record && sso_record.user end - private - def synchronize_groups(user) names = (groups || "").split(",").map(&:downcase) ids = Group.where('LOWER(NAME) in (?) AND NOT automatic', names).pluck(:id) @@ -187,7 +198,7 @@ class DiscourseSingleSignOn < SingleSignOn end def match_email_or_create_user(ip_address) - # Use a mutex here to counter SSO requests that are sent at the same time w + # Use a mutex here to counter SSO requests that are sent at the same time with # the same email payload DistributedMutex.synchronize("discourse_single_sign_on_#{email}") do user = User.find_by_email(email) if !require_activation