fix: Should fix the deletion of permissions when resource name is upp…

…ercase (#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 6d4bcfc) Co-authored-by: ovidiupopa07 <105648914+ovidiupopa07@users.noreply.github.com>
spinnaker · Feb 7, 2023 · 6d9f505 · 6d9f505
1 parent 0282fe2
commit 6d9f505
Show file tree

Hide file tree

Showing 2 changed files with 41 additions and 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
@@ -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/...l/src/test/kotlin/com/netflix/spinnaker/fiat/permissions/SqlPermissionsRepositoryTests.kt b/...l/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 {