From 6d9f5050840c48040edb6435c7a22640412092c7 Mon Sep 17 00:00:00 2001
From: "mergify[bot]" <37929162+mergify[bot]@users.noreply.github.com>
Date: Tue, 7 Feb 2023 18:46:08 +0000
Subject: [PATCH] fix: Should fix the deletion of permissions when resource
 name is uppercase (#1012) (#1014)

After the update to make all resource names lowercase, the user role sync process deletes the permission if the resource name (provided by the ResourceProvider) is in uppercase and the one in DB is in lowercase.

(cherry picked from commit 6d4bcfc02ea6b03b89f67df78bf739fff21ce907)

Co-authored-by: ovidiupopa07 <105648914+ovidiupopa07@users.noreply.github.com>
---
 .../permissions/SqlPermissionsRepository.kt   |  4 +-
 .../SqlPermissionsRepositoryTests.kt          | 41 ++++++++++++++++++-
 2 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/fiat-sql/src/main/kotlin/com/netflix/spinnaker/fiat/permissions/SqlPermissionsRepository.kt b/fiat-sql/src/main/kotlin/com/netflix/spinnaker/fiat/permissions/SqlPermissionsRepository.kt
index dbb2e2fde..f255ee265 100644
--- a/fiat-sql/src/main/kotlin/com/netflix/spinnaker/fiat/permissions/SqlPermissionsRepository.kt
+++ b/fiat-sql/src/main/kotlin/com/netflix/spinnaker/fiat/permissions/SqlPermissionsRepository.kt
@@ -293,7 +293,7 @@ class SqlPermissionsRepository(
         val toStore = mutableListOf<ResourceId>() // ids that are new or changed
 
         resources.forEach {
-            val resourceId = ResourceId(it.resourceType, it.name)
+            val resourceId = ResourceId(it.resourceType, it.name.toLowerCase())
             currentPermissions.add(resourceId)
 
             if (!existingPermissions.contains(resourceId)) {
@@ -372,7 +372,7 @@ class SqlPermissionsRepository(
         val hashes = mutableMapOf<ResourceId, String>() // id to sha256(body)
 
         resources.forEach {
-            val id = ResourceId(it.resourceType, it.name)
+            val id = ResourceId(it.resourceType, it.name.toLowerCase())
             currentIds.add(id)
 
             val body: String? = objectMapper.writeValueAsString(it)
diff --git a/fiat-sql/src/test/kotlin/com/netflix/spinnaker/fiat/permissions/SqlPermissionsRepositoryTests.kt b/fiat-sql/src/test/kotlin/com/netflix/spinnaker/fiat/permissions/SqlPermissionsRepositoryTests.kt
index f9183bfe4..4a754e403 100644
--- a/fiat-sql/src/test/kotlin/com/netflix/spinnaker/fiat/permissions/SqlPermissionsRepositoryTests.kt
+++ b/fiat-sql/src/test/kotlin/com/netflix/spinnaker/fiat/permissions/SqlPermissionsRepositoryTests.kt
@@ -21,7 +21,6 @@ import com.netflix.spinnaker.fiat.config.UnrestrictedResourceConfig.UNRESTRICTED
 import com.netflix.spinnaker.fiat.model.Authorization
 import com.netflix.spinnaker.fiat.model.UserPermission
 import com.netflix.spinnaker.fiat.model.resources.*
-import com.netflix.spinnaker.fiat.permissions.SqlPermissionsRepository
 import com.netflix.spinnaker.kork.sql.config.SqlRetryProperties
 import com.netflix.spinnaker.fiat.permissions.sql.tables.references.PERMISSION
 import com.netflix.spinnaker.fiat.permissions.sql.tables.references.RESOURCE
@@ -31,7 +30,6 @@ import com.netflix.spinnaker.kork.sql.test.SqlTestUtil
 import dev.minutest.ContextBuilder
 import dev.minutest.junit.JUnit5Minutests
 import dev.minutest.rootContext
-import kotlinx.coroutines.newSingleThreadContext
 import org.jooq.DSLContext
 import org.jooq.SQLDialect
 import org.jooq.impl.DSL.*
@@ -684,6 +682,45 @@ internal object SqlPermissionsRepositoryTests : JUnit5Minutests {
                     executor.shutdownNow()
                 }
             }
+
+            test("putAllById should not delete existing permissions when application name is uppercase") {
+                val abcRead = Permissions.Builder().add(Authorization.EXECUTE, "abc").build()
+                val account1 = Account().setName("account").setPermissions(abcRead)
+                val application1 = Application().setName("APP").setPermissions(abcRead)
+                val testUser = UserPermission()
+                        .setId("testUser")
+                        .setAccounts(mutableSetOf(account1))
+                        .setApplications(mutableSetOf(application1))
+                        .setServiceAccounts(mutableSetOf(ServiceAccount().setName("serviceAccount")))
+
+                sqlPermissionsRepository.put(testUser)
+
+                expectThat(
+                        jooq.select(USER.ADMIN).from(USER).where(USER.ID.eq("testuser")).fetchOne(USER.ADMIN)
+                ).isFalse()
+
+                sqlPermissionsRepository.putAllById(mutableMapOf("testUser" to testUser))
+
+
+                expectThat(
+                        jooq.selectCount().from(USER).fetchOne(count())
+                ).isEqualTo(1)
+
+
+                expectThat(
+                        resourceBody(jooq, "testuser", application1.resourceType, application1.name.toLowerCase()).get()
+                ).isEqualTo("""{"name":"APP","permissions":{"EXECUTE":["abc"]},"details":{}}""")
+
+                expectThat(
+                        jooq.select(PERMISSION.RESOURCE_TYPE).from(PERMISSION).where(PERMISSION.USER_ID.eq("testuser").and(PERMISSION.RESOURCE_NAME.eq("app"))).count()
+                ).isEqualTo(1)
+
+                sqlPermissionsRepository.putAllById(mutableMapOf("testUser" to testUser))
+                expectThat(
+                        jooq.select(PERMISSION.RESOURCE_TYPE).from(PERMISSION).where(PERMISSION.USER_ID.eq("testuser").and(PERMISSION.RESOURCE_NAME.eq("app"))).count()
+                ).isEqualTo(1)
+
+            }
         }
 
         after {