Skip to content

Commit e9ab9f8

Browse files
MrSerthMrSerth
committed
Enforce uniqueness constraint on external_id of external_users
Previously, we lacked such a constraint (cf. #2705), causing duplicated external users in production. Probably, they were caused by a double-click on the "Launch exercise", starting a race condition in two tabs to launch the exercise. I assume that both requests succeeded and each created a new external user. Obviously, an external ID should be unique per consumer, so that this error must not happen. We fix it by adding the respective database constraint, and a model validation in Rails (preventing similar issues in the future). For the existing issues, we provide a script, allowing to merge two external user accounts into a single user.
1 parent a16c9e0 commit e9ab9f8

File tree

5 files changed

+164
-4
lines changed

5 files changed

+164
-4
lines changed

‎app/controllers/concerns/lti.rb‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -187,8 +187,10 @@ def send_score_for(submission, user, score)
187187
end
188188

189189
def set_user
190-
@user = ExternalUser.find_or_create_by(consumer_id: @consumer.id, external_id: @provider.user_id)
190+
@user = ExternalUser.find_or_create_by!(consumer_id: @consumer.id, external_id: @provider.user_id)
191191
@user.update(email: external_user_email(@provider), name: external_user_name(@provider))
192+
rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid
193+
retry
192194
end
193195

194196
def set_study_group_membership

‎app/models/external_user.rb‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
# frozen_string_literal: true
22

33
class ExternalUser < User
4-
validates :external_id, presence: true
4+
validates :external_id, presence: true, uniqueness: {scope: :consumer_id}
55
has_many :lti_parameters, dependent: :destroy
66

77
def displayname

‎db/migrate/20250502125320_add_uniqueness_constraint_to_external_user_external_id.rb‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
# frozen_string_literal: true
2+
3+
class AddUniquenessConstraintToExternalUserExternalId < ActiveRecord::Migration[8.0]
4+
def change
5+
Rails.logger.info 'Adding a uniqueness constraint for external IDs associated with external users (and their consumer). ' \
6+
'If this fail, resolve the conflicts using `db/scripts/migrate_external_user.sql`'
7+
8+
change_column_null :external_users, :external_id, false
9+
add_index :external_users, %i[external_id consumer_id], unique: true
10+
end
11+
end

‎db/schema.rb‎

Lines changed: 3 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎db/scripts/migrate_external_user.sql‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,146 @@
1+
CREATE OR REPLACE FUNCTION migrate_external_user(target_user int, duplicated_user int)
2+
RETURNS VOID
3+
LANGUAGE plpgsql
4+
AS
5+
$$
6+
DECLARE
7+
8+
BEGIN
9+
IF target_user = duplicated_user THEN
10+
RETURN;
11+
END IF;
12+
13+
IF EXISTS(SELECT 1
14+
FROM programming_group_memberships a
15+
JOIN programming_group_memberships b ON a.programming_group_id = b.programming_group_id AND a.user_id != b.user_id
16+
WHERE (a.user_id = target_user AND b.user_id = duplicated_user)
17+
OR (a.user_id = duplicated_user AND b.user_id = target_user)) THEN
18+
RAISE NOTICE 'User % is already in the same programming group as user %', target_user, duplicated_user;
19+
RETURN;
20+
END IF;
21+
22+
WITH existing_anomaly_notifications AS (SELECT CONCAT(exercise_id, '_', exercise_collection_id) as existing
23+
FROM anomaly_notifications
24+
WHERE contributor_id = target_user AND contributor_type = 'ExternalUser')
25+
DELETE FROM anomaly_notifications WHERE contributor_id = duplicated_user AND contributor_type = 'ExternalUser'
26+
AND CONCAT(exercise_id, '_', exercise_collection_id) IN (SELECT existing FROM existing_anomaly_notifications);
27+
28+
UPDATE anomaly_notifications SET contributor_id = target_user WHERE contributor_id = duplicated_user AND contributor_type = 'ExternalUser';
29+
UPDATE authentication_tokens SET user_id = target_user WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
30+
31+
WITH existing_codeharbor_link AS (SELECT TRUE as existing
32+
FROM codeharbor_links
33+
WHERE user_id = target_user AND user_type = 'ExternalUser')
34+
DELETE FROM codeharbor_links
35+
WHERE user_id = duplicated_user AND user_type = 'ExternalUser'
36+
AND TRUE IN (SELECT existing FROM existing_codeharbor_link);
37+
38+
UPDATE codeharbor_links SET user_id = target_user WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
39+
UPDATE comments SET user_id = target_user WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
40+
UPDATE community_solution_contributions SET user_id = target_user WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
41+
UPDATE community_solution_locks SET user_id = target_user WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
42+
UPDATE events SET user_id = target_user WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
43+
UPDATE events_synchronized_editor SET user_id = target_user WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
44+
UPDATE execution_environments SET user_id = target_user WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
45+
UPDATE exercise_collections SET user_id = target_user WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
46+
UPDATE exercises SET user_id = target_user WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
47+
UPDATE file_types SET user_id = target_user WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
48+
49+
WITH existing_lti_parameters AS (SELECT CONCAT(exercise_id, '_', study_group_id) as existing
50+
FROM lti_parameters
51+
WHERE external_user_id = target_user)
52+
DELETE FROM lti_parameters
53+
WHERE external_user_id = duplicated_user
54+
AND CONCAT(exercise_id, '_', study_group_id) IN (SELECT existing FROM existing_lti_parameters);
55+
56+
UPDATE lti_parameters SET external_user_id = target_user WHERE external_user_id = duplicated_user;
57+
58+
WITH existing_pair_programming_exercise_feedbacks AS (SELECT CONCAT(exercise_id, '_', study_group_id, '_', programming_group_id) as existing
59+
FROM pair_programming_exercise_feedbacks
60+
WHERE user_id = target_user AND user_type = 'ExternalUser')
61+
DELETE FROM pair_programming_exercise_feedbacks
62+
WHERE user_id = duplicated_user AND user_type = 'ExternalUser'
63+
AND CONCAT(exercise_id, '_', study_group_id, '_', programming_group_id) IN
64+
(SELECT existing FROM existing_pair_programming_exercise_feedbacks);
65+
66+
UPDATE pair_programming_exercise_feedbacks SET user_id = target_user WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
67+
68+
WITH existing_pair_programming_waiting_users AS (SELECT exercise_id as existing
69+
FROM pair_programming_waiting_users
70+
WHERE user_id = target_user AND user_type = 'ExternalUser')
71+
DELETE FROM pair_programming_waiting_users
72+
WHERE user_id = duplicated_user AND user_type = 'ExternalUser'
73+
AND exercise_id IN (SELECT existing FROM existing_pair_programming_waiting_users);
74+
75+
UPDATE pair_programming_waiting_users SET user_id = target_user WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
76+
UPDATE programming_group_memberships SET user_id = target_user WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
77+
UPDATE proxy_exercises SET user_id = target_user WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
78+
UPDATE remote_evaluation_mappings SET user_id = target_user WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
79+
UPDATE request_for_comments SET user_id = target_user WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
80+
81+
WITH existing_runners AS (SELECT execution_environment_id as existing
82+
FROM runners
83+
WHERE contributor_id = target_user AND contributor_type = 'ExternalUser')
84+
DELETE
85+
FROM runners
86+
WHERE contributor_id = duplicated_user AND contributor_type = 'ExternalUser'
87+
AND execution_environment_id IN (SELECT existing FROM existing_runners);
88+
89+
UPDATE runners SET contributor_id = target_user WHERE contributor_id = duplicated_user AND contributor_type = 'ExternalUser';
90+
UPDATE searches SET user_id = target_user WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
91+
92+
WITH existing_study_group_memberships AS (SELECT study_group_id as existing
93+
FROM study_group_memberships
94+
WHERE user_id = target_user AND user_type = 'ExternalUser')
95+
DELETE FROM study_group_memberships
96+
WHERE user_id = duplicated_user AND user_type = 'ExternalUser'
97+
AND study_group_id IN (SELECT existing FROM existing_study_group_memberships);
98+
99+
UPDATE study_group_memberships SET user_id = target_user WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
100+
UPDATE submissions SET contributor_id = target_user WHERE contributor_id = duplicated_user AND contributor_type = 'ExternalUser';
101+
102+
WITH existing_subscriptions AS (SELECT request_for_comment_id as existing
103+
FROM subscriptions
104+
WHERE user_id = target_user AND user_type = 'ExternalUser')
105+
DELETE FROM subscriptions
106+
WHERE user_id = duplicated_user AND user_type = 'ExternalUser'
107+
AND request_for_comment_id IN (SELECT existing FROM existing_subscriptions);
108+
109+
-- We don't care about the subscription_type nor deleted flag here.
110+
UPDATE subscriptions SET user_id = target_user WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
111+
UPDATE testruns SET user_id = target_user WHERE user_id = duplicated_user;
112+
UPDATE tips SET user_id = target_user WHERE user_id = duplicated_user;
113+
114+
WITH existing_user_exercise_feedbacks AS (SELECT exercise_id as existing
115+
FROM user_exercise_feedbacks
116+
WHERE user_id = target_user AND user_type = 'ExternalUser')
117+
DELETE FROM user_exercise_feedbacks
118+
WHERE user_id = duplicated_user AND user_type = 'ExternalUser'
119+
AND exercise_id IN (SELECT existing FROM existing_user_exercise_feedbacks);
120+
121+
UPDATE user_exercise_feedbacks SET user_id = target_user WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
122+
UPDATE user_exercise_interventions SET contributor_id = target_user WHERE contributor_id = duplicated_user AND contributor_type = 'ExternalUser';
123+
124+
WITH existing_user_proxy_exercise_exercises AS (SELECT proxy_exercise_id as existing
125+
FROM user_proxy_exercise_exercises
126+
WHERE user_id = target_user AND user_type = 'ExternalUser')
127+
DELETE FROM user_proxy_exercise_exercises
128+
WHERE user_id = duplicated_user AND user_type = 'ExternalUser'
129+
AND proxy_exercise_id IN (SELECT existing FROM existing_user_proxy_exercise_exercises);
130+
131+
UPDATE user_proxy_exercise_exercises SET user_id = target_user WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
132+
133+
-- Passkeys need to be set up again.
134+
DELETE FROM webauthn_credentials WHERE user_id = duplicated_user AND user_type = 'ExternalUser';
135+
136+
DELETE FROM external_users WHERE ID = duplicated_user;
137+
END;
138+
$$;
139+
140+
/* Execute migration
141+
do $$
142+
begin
143+
perform migrate_external_user(target_user := 63783, duplicated_user := 63784);
144+
end
145+
$$;
146+
*/

0 commit comments

Comments
 (0)