Skip to content

Add missing ON DELETE FK constraints referencing users table

Drew Blessing requested to merge dblessing-add-missing-on-delete-fk-constraints into master

What does this MR do?

Fixes #195699 (closed).

The following tables were missing FK constraints referencing the users table (user_id column) with cascading ON DELETE:

  • protected_tag_create_access_levels
  • protected_branch_merge_access_levels
  • path_locks
  • protected_branch_push_access_levels
  • u2f_registrations

These migrations utilize what I understand to be the 'safe' way based on the discussion in the issue. The add_concurrent_foreign_key does a two-step process where it adds the foreign key with NOT VALID first and then subsequently validates the foreign key. This results in two different types of locks on the table and should reduce contention.

== 20200407171133 AddProtectedTagCreateAccessLevelsUserIdForeignKey: migrating
-- add_foreign_key(:protected_tag_create_access_levels, :users, {:on_delete=>:cascade, :validate=>false, :name=>"fk_protected_tag_create_access_levels_user_id"})
   -> 0.0023s
-- foreign_keys(:protected_tag_create_access_levels)
   -> 0.0032s
-- remove_foreign_key(:protected_tag_create_access_levels, {:column=>:user_id, :on_delete=>nil})
   -> 0.0033s
== 20200407171133 AddProtectedTagCreateAccessLevelsUserIdForeignKey: migrated (0.0159s)

== 20200407171417 ValidateProtectedTagCreateAccessLevelsUserIdForeignKey: migrating
-- foreign_keys(:protected_tag_create_access_levels)
   -> 0.0025s
-- execute("SET statement_timeout TO 0")
   -> 0.0002s
-- execute("ALTER TABLE protected_tag_create_access_levels VALIDATE CONSTRAINT fk_protected_tag_create_access_levels_user_id;")
   -> 0.0027s
-- execute("RESET ALL")
   -> 0.0002s
== 20200407171417 ValidateProtectedTagCreateAccessLevelsUserIdForeignKey: migrated (0.0058s)

== 20200408154331 AddProtectedBranchMergeAccessLevelsUserIdForeignKey: migrating
-- add_foreign_key(:protected_branch_merge_access_levels, :users, {:on_delete=>:cascade, :validate=>false, :name=>"fk_protected_branch_merge_access_levels_user_id"})
   -> 0.0010s
-- foreign_keys(:protected_branch_merge_access_levels)
   -> 0.0020s
-- remove_foreign_key(:protected_branch_merge_access_levels, {:column=>:user_id, :on_delete=>nil})
   -> 0.0024s
== 20200408154331 AddProtectedBranchMergeAccessLevelsUserIdForeignKey: migrated (0.0071s)

== 20200408154349 ValidateProtectedBranchMergeAccessLevelsUserIdForeignKey: migrating
-- foreign_keys(:protected_branch_merge_access_levels)
   -> 0.0028s
-- execute("SET statement_timeout TO 0")
   -> 0.0002s
-- execute("ALTER TABLE protected_branch_merge_access_levels VALIDATE CONSTRAINT fk_protected_branch_merge_access_levels_user_id;")
   -> 0.0011s
-- execute("RESET ALL")
   -> 0.0002s
== 20200408154349 ValidateProtectedBranchMergeAccessLevelsUserIdForeignKey: migrated (0.0045s)

== 20200408154411 AddPathLocksUserIdForeignKey: migrating =====================
-- add_foreign_key(:path_locks, :users, {:on_delete=>:cascade, :validate=>false, :name=>"fk_path_locks_user_id"})
   -> 0.0009s
-- foreign_keys(:path_locks)
   -> 0.0019s
-- remove_foreign_key(:path_locks, {:column=>:user_id, :on_delete=>nil})
   -> 0.0024s
== 20200408154411 AddPathLocksUserIdForeignKey: migrated (0.0067s) ============

== 20200408154428 ValidatePathLocksUserIdForeignKey: migrating ================
-- foreign_keys(:path_locks)
   -> 0.0023s
-- execute("SET statement_timeout TO 0")
   -> 0.0002s
-- execute("ALTER TABLE path_locks VALIDATE CONSTRAINT fk_path_locks_user_id;")
   -> 0.0010s
-- execute("RESET ALL")
   -> 0.0002s
== 20200408154428 ValidatePathLocksUserIdForeignKey: migrated (0.0038s) =======

== 20200408154455 AddProtectedBranchPushAccessLevelsUserIdForeignKey: migrating
-- add_foreign_key(:protected_branch_push_access_levels, :users, {:on_delete=>:cascade, :validate=>false, :name=>"fk_protected_branch_push_access_levels_user_id"})
   -> 0.0009s
-- foreign_keys(:protected_branch_push_access_levels)
   -> 0.0019s
-- remove_foreign_key(:protected_branch_push_access_levels, {:column=>:user_id, :on_delete=>nil})
   -> 0.0022s
== 20200408154455 AddProtectedBranchPushAccessLevelsUserIdForeignKey: migrated (0.0063s)

== 20200408154533 ValidateProtectedBranchPushAccessLevelsUserIdForeignKey: migrating
-- foreign_keys(:protected_branch_push_access_levels)
   -> 0.0019s
-- execute("SET statement_timeout TO 0")
   -> 0.0002s
-- execute("ALTER TABLE protected_branch_push_access_levels VALIDATE CONSTRAINT fk_protected_branch_push_access_levels_user_id;")
   -> 0.0010s
-- execute("RESET ALL")
   -> 0.0002s
== 20200408154533 ValidateProtectedBranchPushAccessLevelsUserIdForeignKey: migrated (0.0034s)

== 20200408154604 AddU2fRegistrationsUserIdForeignKey: migrating ==============
-- add_foreign_key(:u2f_registrations, :users, {:on_delete=>:cascade, :validate=>false, :name=>"fk_u2f_registrations_user_id"})
   -> 0.0009s
-- foreign_keys(:u2f_registrations)
   -> 0.0018s
-- remove_foreign_key(:u2f_registrations, {:column=>:user_id, :on_delete=>nil})
   -> 0.0023s
== 20200408154604 AddU2fRegistrationsUserIdForeignKey: migrated (0.0065s) =====

== 20200408154624 ValidateU2fRegistrationsUserIdForeignKey: migrating =========
-- foreign_keys(:u2f_registrations)
   -> 0.0018s
-- execute("SET statement_timeout TO 0")
   -> 0.0002s
-- execute("ALTER TABLE u2f_registrations VALIDATE CONSTRAINT fk_u2f_registrations_user_id;")
   -> 0.0010s
-- execute("RESET ALL")
   -> 0.0002s
== 20200408154624 ValidateU2fRegistrationsUserIdForeignKey: migrated (0.0033s)

Screenshots

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Drew Blessing

Merge request reports