Clear conflicting connection impersonations before adding new ones (#32579)

enterprise/backend/src/metabase_enterprise/advanced_permissions/models/connection_impersonation.clj

@@ -29,14 +29,18 @@
              {}
              impersonations))))
 
-(defenterprise upsert-impersonations!
-  "Create new Connection Impersonation records or update existing ones, if they have an `:id`."
+(defenterprise insert-impersonations!
+  "Create new Connection Impersonation records. Deletes any existing Connection Impersonation records for the same
+  group and database before creating new ones."
   :feature :advanced-permissions
   [impersonations]
   (doall
    (for [impersonation impersonations]
-     (if-let [id (:id impersonation)]
-       (t2/update! :model/ConnectionImpersonation id impersonation)
+
+     (do
+       (t2/delete! :model/ConnectionImpersonation
+                   :group_id (:group_id impersonation)
+                   :db_id (:db_id impersonation))
        (-> (t2/insert-returning-instances! :model/ConnectionImpersonation impersonation)
            first)))))
 

enterprise/backend/test/metabase_enterprise/advanced_permissions/api/impersonation_test.clj

@@ -21,21 +21,24 @@
           (let [impersonation {:group_id  (u/the-id group)
                                :db_id     (mt/id)
                                :attribute "Attribute Name"}
-                graph         (assoc (perms/data-perms-graph) :impersonations [impersonation])
-                result        (mt/user-http-request :crowberto :put 200 "permissions/graph" graph)]
-            (is (= [(assoc impersonation :id (-> result :impersonations first :id))]
-                   (t2/select :model/ConnectionImpersonation :group_id (u/the-id group)))))
+                graph         (assoc (perms/data-perms-graph) :impersonations [impersonation])]
+            (mt/user-http-request :crowberto :put 200 "permissions/graph" graph)
+            (is (=? [impersonation]
+                    (t2/select :model/ConnectionImpersonation :group_id (u/the-id group)))))
 
           (testing "A connection impersonation policy can be updated via the permissions graph endpoint"
-            (let [impersonation (-> (t2/select :model/ConnectionImpersonation
-                                               :group_id (u/the-id group))
-                                    first
-                                    (assoc :attribute "New Attribute Name"))
+            (let [impersonation {:group_id  (u/the-id group)
+                                 :db_id     (mt/id)
+                                 :attribute "New Attribute Name"}
                   graph         (assoc (perms/data-perms-graph) :impersonations [impersonation])]
               (mt/user-http-request :crowberto :put 200 "permissions/graph" graph)
-              (is (= [impersonation]
-                     (t2/select :model/ConnectionImpersonation
-                                :group_id (u/the-id group)))))))))))
+              (is (=?
+                   [{:group_id  (u/the-id group)
+                     :db_id     (mt/id)
+                     :attribute "New Attribute Name"}]
+                   (t2/select :model/ConnectionImpersonation
+                              :group_id (u/the-id group))))
+              (is (= 1 (t2/count :model/ConnectionImpersonation :group_id (u/the-id group)))))))))))
 
 (deftest fetch-impersonation-policy-test
   (testing "GET /api/ee/advanced-permissions/impersonation"

resources/migrations/000_migrations.yaml

@@ -15922,6 +15922,104 @@ databaseChangeLog:
                   constraints:
                     nullable: false
+  - changeSet:
+      id: v47.00-051
+      author: noahmoss
+      comment: Added 0.47.0 - Drop foreign key constraint on connection_impersonations.db_id
+      changes:
+        - dropForeignKeyConstraint:
+            baseTableName: connection_impersonations
+            constraintName: fk_conn_impersonation_db_id
+      rollback:
+        - addForeignKeyConstraint:
+            baseTableName: connection_impersonations
+            baseColumnNames: db_id
+            referencedTableName: metabase_database
+            referencedColumnNames: id
+            constraintName: fk_conn_impersonation_db_id
+            onDelete: CASCADE
+
+  - changeSet:
+      id: v47.00-052
+      author: noahmoss
+      comment: Added 0.47.0 - Drop foreign key constraint on connection_impersonations.group_id
+      changes:
+        - dropForeignKeyConstraint:
+            baseTableName: connection_impersonations
+            constraintName: fk_conn_impersonation_group_id
+      rollback:
+        - addForeignKeyConstraint:
+            baseTableName: connection_impersonations
+            baseColumnNames: group_id
+            referencedTableName: permissions_group
+            referencedColumnNames: id
+            constraintName: fk_conn_impersonation_group_id
+            onDelete: CASCADE
+
+  - changeSet:
+      id: v47.00-053
+      author: noahmoss
+      comment: Added 0.47.0 -- connection_impersonations index for db_id column
+      changes:
+        - createIndex:
+            tableName: connection_impersonations
+            columns:
+              - column:
+                  name: db_id
+            indexName: idx_conn_impersonations_db_id
+
+  - changeSet:
+      id: v47.00-054
+      author: noahmoss
+      comment: Added 0.47.0 -- connection_impersonations index for group_id column
+      changes:
+        - createIndex:
+            tableName: connection_impersonations
+            columns:
+              - column:
+                  name: group_id
+            indexName: idx_conn_impersonations_group_id
+
+  - changeSet:
+      id: v47.00-055
+      author: noahmoss
+      comment: Added 0.47.0 - unique constraint for connection impersonations
+      changes:
+        - addUniqueConstraint:
+            tableName: connection_impersonations
+            columnNames: group_id, db_id
+            constraintName: conn_impersonation_unique_group_id_db_id
+      rollback:
+        - dropUniqueConstraint:
+            tableName: connection_impersonations
+            constraintName: conn_impersonation_unique_group_id_db_id
+
+  - changeSet:
+      id: v47.00-056
+      author: noahmoss
+      comment: Added 0.47.0 - re-add foreign key constraint on connection_impersonations.db_id
+      changes:
+        - addForeignKeyConstraint:
+            baseTableName: connection_impersonations
+            baseColumnNames: db_id
+            referencedTableName: metabase_database
+            referencedColumnNames: id
+            constraintName: fk_conn_impersonation_db_id
+            onDelete: CASCADE
+
+  - changeSet:
+      id: v47.00-057
+      author: noahmoss
+      comment: Added 0.47.0 - re-add foreign key constraint on connection_impersonations.group_id
+      changes:
+        - addForeignKeyConstraint:
+            baseTableName: connection_impersonations
+            baseColumnNames: group_id
+            referencedTableName: permissions_group
+            referencedColumnNames: id
+            constraintName: fk_conn_impersonation_group_id
+            onDelete: CASCADE
+
   - changeSet:
       id: v48.00-003
       author: qnkhuat

src/metabase/api/permissions.clj

@@ -53,8 +53,8 @@
   (throw (ex-info (tru "Sandboxes are an Enterprise feature. Please upgrade to a paid plan to use this feature.")
                   {:status-code 402})))
 
-(defenterprise upsert-impersonations!
-  "OSS implementation of `upsert-impersonations!`. Errors since this is an enterprise feature."
+(defenterprise insert-impersonations!
+  "OSS implementation of `insert-impersonations!`. Errors since this is an enterprise feature."
   metabase-enterprise.advanced-permissions.models.connection-impersonation
   [_impersonations]
   (throw (ex-info (tru "Connection impersonation is an Enterprise feature. Please upgrade to a paid plan to use this feature.")
@@ -97,7 +97,7 @@
                                      (upsert-sandboxes! sandbox-updates))
             impersonation-updates  (:impersonations graph)
             impersonations         (when impersonation-updates
-                                     (upsert-impersonations! impersonation-updates))]
+                                     (insert-impersonations! impersonation-updates))]
         (merge
          (perms/data-perms-graph)
          (when sandboxes {:sandboxes sandboxes})