[bugfix] Service Account Permissions being wiped (#1168)

* [bugfix] Service Account Permissions being wiped Fixes an issue where the service account permissions were being wiped for other applications. The backend didn't know to keep it within the scope of a single application for the updates as it has to figure out which permissions were deleted for environments. Fixes #1132 * fix tests * always recalc the list of permissions as it isn't detecting when the env changes --------- Co-authored-by: Irina Southwell <[email protected]>
featurehub-io · Jul 20, 2024 · 674f5e2 · 674f5e2
1 parent 9a5b28d
commit 674f5e2
Show file tree

Hide file tree

Showing 11 changed files with 6,241 additions and 31 deletions.
diff --git a/admin-frontend/build-frontend.sh b/admin-frontend/build-frontend.sh
@@ -9,11 +9,11 @@ cp build/build.sh target
 chmod ugo+x target/build.sh
 cd target
 tar xvf *.tar
-if which flutter >/dev/null; then
-  sh build.sh
-else
+#if which flutter >/dev/null; then
+#  sh build.sh
+#else
   docker run --rm -e BUILD_VERSION="$1" -v $PWD:/opt/app --entrypoint /bin/bash featurehub/flutter_web:1.12 -c "cd /opt/app && ./build.sh"
-fi
+#fi
 
 #docker run -it -v $PWD/open_admin_app:/opt/app/app -v $PWD/app_mr_layer:/opt/app/app_mr_layer -v $PWD/build:/opt/build featurehub/flutter_web:1.1 /bin/sh /opt/build/build.sh
 
diff --git a/admin-frontend/open_admin_app/lib/widgets/apps/manage_app_bloc.dart b/admin-frontend/open_admin_app/lib/widgets/apps/manage_app_bloc.dart
@@ -305,13 +305,14 @@ class ManageAppBloc implements Bloc, ManagementRepositoryAwareBloc {
   }
 
   Future<ServiceAccount?> updateServiceAccountPermissions(
-      String sid, ServiceAccount serviceAccount) async {
+      String sid, ServiceAccount serviceAccount, String? appId) async {
     try {
       final updatedServiceAccount =
           await _serviceAccountServiceApi.updateServiceAccountOnPortfolio(
         portfolio!.id,
         serviceAccount,
         includePermissions: true,
+        appId: appId
       );
 
       _serviceAccountPS.add(updatedServiceAccount);

diff --git a/admin-frontend/open_admin_app/lib/widgets/apps/service_account_permissions_widget.dart b/admin-frontend/open_admin_app/lib/widgets/apps/service_account_permissions_widget.dart
@@ -212,12 +212,9 @@ class _ServiceAccountPermissionDetailState
                   );
                 }
 
-                if (currentServiceAccount == null ||
-                    currentServiceAccount!.id != saSnapshot.data!.id) {
-                  newServiceAccountPermission =
-                      createMap(envSnapshot.data!, saSnapshot.data!);
-                  currentServiceAccount = saSnapshot.data;
-                }
+                newServiceAccountPermission =
+                    createMap(envSnapshot.data!, saSnapshot.data!);
+                currentServiceAccount = saSnapshot.data;
 
                 final rows = <TableRow>[];
                 rows.add(getHeader());
@@ -277,7 +274,8 @@ class _ServiceAccountPermissionDetailState
                             newSa.permissions = newList;
                             widget.bloc
                                 .updateServiceAccountPermissions(
-                                    newSa.id, saSnapshot.data!)
+                                    newSa.id, saSnapshot.data!,
+                                    (envSnapshot.data?.isNotEmpty == true) ? envSnapshot.data?.first.applicationId : null)
                                 .then((serviceAccount) => widget.bloc.mrClient
                                     .addSnackbar(Text(
                                         "Service account '${serviceAccount?.name ?? '<unknown>'}' updated!")))

diff --git a/backend/mr-api/mr-api.yaml b/backend/mr-api/mr-api.yaml
@@ -2,7 +2,7 @@ openapi: 3.0.1
 info:
   title: ManagementResourceApi
   description: This describes the API clients use for accessing features. This reflects the API from 1.6.1 onwards.
-  version: "1.2.1"
+  version: "1.2.2"
 #  CRUD for portfolios, environments, features, service account, people, and groups (edited)
 #  roles are fixed
 #  then people<->group association
@@ -2395,6 +2395,13 @@ paths:
           in: query
           schema:
             type: boolean
+        - name: appId
+          description: "limit removals to this application id"
+          in: query
+          required: false
+          schema:
+            type: string
+            format: uuid
       tags:
         - ServiceAccountService
       security:

diff --git a/backend/mr-db-api/src/main/kotlin/io/featurehub/db/api/ServiceAccountApi.kt b/backend/mr-db-api/src/main/kotlin/io/featurehub/db/api/ServiceAccountApi.kt
@@ -9,9 +9,9 @@ interface ServiceAccountApi {
   operator fun get(id: UUID, opts: Opts): ServiceAccount?
 
   @Throws(OptimisticLockingException::class)
-  fun update(serviceAccountId: UUID, updater: Person, serviceAccount: ServiceAccount, opts: Opts): ServiceAccount?
+  fun update(serviceAccountId: UUID, updater: Person, serviceAccount: ServiceAccount, appId: UUID?, opts: Opts): ServiceAccount?
   @Throws(OptimisticLockingException::class)
-  fun update(portfolioId: UUID, personId: UUID, serviceAccount: ServiceAccount, opts: Opts): ServiceAccount?
+  fun update(portfolioId: UUID, personId: UUID, serviceAccount: ServiceAccount, appId: UUID?, opts: Opts): ServiceAccount?
 
   /**
    * This has to determine if this user has access based on what they are asking for. If they have any access to the

diff --git a/backend/mr-db-sql/src/main/kotlin/io/featurehub/db/services/ServiceAccountSqlApi.kt b/backend/mr-db-sql/src/main/kotlin/io/featurehub/db/services/ServiceAccountSqlApi.kt
@@ -1,6 +1,5 @@
 package io.featurehub.db.services
 
-import io.ebean.Database
 import io.ebean.DuplicateKeyException
 import io.ebean.annotation.Transactional
 import io.ebean.annotation.TxType
@@ -48,22 +47,23 @@ class ServiceAccountSqlApi @Inject constructor(
   }
 
   @Throws(OptimisticLockingException::class)
-  override fun update(portfolioId: UUID, personId: UUID, serviceAccount: ServiceAccount, opts: Opts): ServiceAccount? {
+  override fun update(portfolioId: UUID, personId: UUID, serviceAccount: ServiceAccount, appId: UUID?, opts: Opts): ServiceAccount? {
     val sa = QDbServiceAccount().id.eq(serviceAccount.id).whenArchived.isNull.portfolio.id.eq(portfolioId).findOne() ?: return null
 
     if (serviceAccount.version == null || serviceAccount.version != sa.version) {
       throw OptimisticLockingException()
     }
 
     val whoUpdated = convertUtils.byPerson(personId) ?: return null
-    return update(sa, whoUpdated, serviceAccount, opts)
+    return update(sa, whoUpdated, serviceAccount, appId, opts)
   }
 
   @Throws(OptimisticLockingException::class)
   override fun update(
     serviceAccountId: UUID,
     updater: Person,
     serviceAccount: ServiceAccount,
+    appId: UUID?,
     opts: Opts
   ): ServiceAccount? {
     val sa = QDbServiceAccount().id.eq(serviceAccountId).whenArchived.isNull.findOne() ?: return null
@@ -72,10 +72,10 @@ class ServiceAccountSqlApi @Inject constructor(
       throw OptimisticLockingException()
     }
     val whoUpdated = convertUtils.byPerson(updater) ?: return null
-    return update(sa, whoUpdated, serviceAccount, opts)
+    return update(sa, whoUpdated, serviceAccount, appId, opts)
   }
 
-  private fun update(sa: DbServiceAccount, whoUpdated: DbPerson, serviceAccount: ServiceAccount, opts: Opts): ServiceAccount {
+  private fun update(sa: DbServiceAccount, whoUpdated: DbPerson, serviceAccount: ServiceAccount, appId: UUID?, opts: Opts): ServiceAccount {
     val updatedEnvironments: MutableMap<UUID, ServiceAccountPermission> = HashMap()
     val newEnvironments: MutableList<UUID> = ArrayList()
     serviceAccount
@@ -88,9 +88,16 @@ class ServiceAccountSqlApi @Inject constructor(
     val createPerms = mutableListOf<DbServiceAccountEnvironment>()
 
     // we drop out of this knowing which perms to delete and update
-    QDbServiceAccountEnvironment().environment.id
+    var finder = QDbServiceAccountEnvironment().environment.id
       .`in`(updatedEnvironments.keys).serviceAccount
       .eq(sa)
+
+    // limit our area of interest
+    if (appId != null) {
+      finder = finder.environment.parentApplication.id.eq(appId)
+    }
+
+    finder
       .findEach { upd: DbServiceAccountEnvironment ->
         val envId = upd.environment.id
         val perm = updatedEnvironments[envId]
@@ -106,7 +113,13 @@ class ServiceAccountSqlApi @Inject constructor(
         }
       }
 
-    QDbServiceAccountEnvironment().environment.id.notIn(updatedEnvironments.keys).serviceAccount.eq(sa).findEach { toDelete ->
+    var deleteFinder = QDbServiceAccountEnvironment().environment.id.notIn(updatedEnvironments.keys).serviceAccount.eq(sa)
+
+    if (appId != null) {
+      deleteFinder = deleteFinder.environment.parentApplication.id.eq(appId)
+    }
+
+    deleteFinder.findEach { toDelete ->
       deletePerms.add(toDelete)
     }
 
@@ -313,7 +326,7 @@ class ServiceAccountSqlApi @Inject constructor(
           .permissions(convertPermissionsToString(sap.permissions))
           .build()
       }
-    }.toMutableSet() ?: mutableSetOf()
+    }.toMutableSet()
 
     val sdkPerson = internalPersonApi.createSdkServiceAccountUser(serviceAccount.name, who, false)
     // now create the SA and attach the perms to form the links
@@ -338,8 +351,8 @@ class ServiceAccountSqlApi @Inject constructor(
   }
 
   private fun environmentMap(serviceAccount: CreateServiceAccount): Map<UUID, DbEnvironment> {
-    // find all of the UUIDs in the environment list
-    val envIds = serviceAccount.permissions.map { obj: ServiceAccountPermission -> obj.environmentId } ?: listOf()
+    // find all the UUIDs in the environment list
+    val envIds = serviceAccount.permissions.map { obj: ServiceAccountPermission -> obj.environmentId }
 
     // now find them in the db in one swoop using "in" syntax
     return QDbEnvironment().id.`in`(envIds).whenArchived.isNull.findList().associateBy { e -> e.id }
@@ -410,7 +423,7 @@ class ServiceAccountSqlApi @Inject constructor(
   }
 
   /**
-   * This is a transitional job that assumes that all of the attached people to the sdkUser are invalid and created
+   * This is a transitional job that assumes that all the attached people to the sdkUser are invalid and created
    * new SDK style user accounts for them. After the migration that inserts the job, this is in fact TRUE.
    */
   @Transactional
@@ -433,6 +446,6 @@ class ServiceAccountSqlApi @Inject constructor(
 
   // allow us to identify the user who created a feature change for instance
   override fun findServiceAccountByUserId(personId: UUID): UUID? {
-    return QDbServiceAccount().sdkPerson.id.eq(personId).findOne()?.let { it.id }
+    return QDbServiceAccount().sdkPerson.id.eq(personId).findOne()?.id
   }
 }
diff --git a/backend/mr-db-sql/src/test/groovy/io/featurehub/db/services/Base2Spec.groovy b/backend/mr-db-sql/src/test/groovy/io/featurehub/db/services/Base2Spec.groovy
@@ -11,6 +11,7 @@ import io.featurehub.db.test.DbSpecification
 import io.featurehub.mr.model.Group
 import io.featurehub.mr.model.Organization
 import io.featurehub.mr.model.Person
+import org.apache.commons.lang3.RandomStringUtils
 
 class Base2Spec extends DbSpecification {
   ConvertUtils convertUtils
@@ -22,6 +23,10 @@ class Base2Spec extends DbSpecification {
   Organization org
   WebhookEncryptionService encryptionService
 
+  String ranName() {
+    return RandomStringUtils.randomAlphabetic(10)
+  }
+
   def setup() {
     System.setProperty("webhooks.encryption.password", "foof")
     encryptionService = Mock()

diff --git a/backend/mr-db-sql/src/test/groovy/io/featurehub/db/services/ServiceAccount2Spec.groovy b/backend/mr-db-sql/src/test/groovy/io/featurehub/db/services/ServiceAccount2Spec.groovy
@@ -304,7 +304,7 @@ class ServiceAccount2Spec extends Base2Spec {
           ]
         )
 
-      def secondUpdate = sapi.update(createdServiceAccount.id, superPerson, updated, Opts.opts(FillOpts.Permissions))
+      def secondUpdate = sapi.update(createdServiceAccount.id, superPerson, updated, app1.id, Opts.opts(FillOpts.Permissions))
     and: "search for the result"
       def updatedResult = sapi.search(portfolio1Id, "sa-1", application1.id, superPerson, Opts.opts(FillOpts.Permissions)).find({it.id == createdServiceAccount.id})
 
@@ -334,14 +334,14 @@ class ServiceAccount2Spec extends Base2Spec {
       newEnv1.serviceAccountPermission.find({ it.serviceAccount.name == 'sa-1'}).sdkUrlClientEval.contains("/" + newEnv1.serviceAccountPermission.find({ it.serviceAccount.name == 'sa-1'}).serviceAccount.apiKeyClientSide)
       newEnv1.serviceAccountPermission.find({ it.serviceAccount.name == 'sa-1'}).sdkUrlServerEval.contains("/" + newEnv1.serviceAccountPermission.find({ it.serviceAccount.name == 'sa-1'}).serviceAccount.apiKeyServerSide)
     when: "we update a second time using the new API"
-      def thirdUpdate = sapi.update(portfolio1Id, superuser, secondUpdate.permissions([]), Opts.opts(FillOpts.Permissions))
+      def thirdUpdate = sapi.update(portfolio1Id, superuser, secondUpdate.permissions([]), app1.id, Opts.opts(FillOpts.Permissions))
     then:
       thirdUpdate.permissions.isEmpty()
   }
 
   def "I cannot request or update an unknown service account"() {
     when:
-      def x = sapi.update(UUID.randomUUID(), superPerson, new ServiceAccount(), Opts.empty())
+      def x = sapi.update(UUID.randomUUID(), superPerson, new ServiceAccount(), null, Opts.empty())
       def y = sapi.get(UUID.randomUUID(), Opts.empty())
     then:
       x == null

diff --git a/backend/mr/src/main/kotlin/io/featurehub/mr/resources/ServiceAccountResource.kt b/backend/mr/src/main/kotlin/io/featurehub/mr/resources/ServiceAccountResource.kt
@@ -134,6 +134,7 @@ class ServiceAccountResource @Inject constructor(
           id,
           person,
           serviceAccount,
+          holder.appId,
           Opts().add(FillOpts.Permissions, holder.includePermissions)
         ) ?: throw NotFoundException()
       } catch (e: OptimisticLockingException) {
@@ -163,6 +164,7 @@ class ServiceAccountResource @Inject constructor(
           serviceAccountId,
           person,
           serviceAccount,
+          null,
           Opts().add(FillOpts.Permissions, holder.includePermissions)
         ) ?: throw NotFoundException()
       } catch (e: OptimisticLockingException) {