Remove defunct users when updating a database

.junie/guidelines.md

@@ -0,0 +1,92 @@
+### Project-specific development guidelines
+
+This document summarizes non-obvious, project-specific details to help advanced contributors build, test, and debug this repo reliably.
+
+#### Build and configuration
+- Toolchain
+  - Scala 3.7.4 (see `build.sbt`), JVM 17 (GitHub CI uses Temurin 17). Use Java 17 locally to match.
+  - Build tool: sbt (standard layout). Packaging uses `sbt-native-packager` (`JavaAppPackaging` and `UniversalPlugin`).
+  - Codegen: The `smithy` subproject is enabled with `com.disneystreaming.smithy4s` codegen. sbt will generate code for Smithy models before compiling dependents. No manual steps are needed beyond running sbt.
+
+- Subprojects
+  - Root project is named `postgresql-init-core` in `build.sbt` and depends on `:smithy` for generated Smithy4s code.
+  - Smithy models live under `smithy/src/main`.
+
+- Dependencies and noteworthy versions
+  - Skunk 0.6.4 for PostgreSQL access.
+  - Cats Effect 3 ecosystem, Natchez for tracing (including X-Ray), `log4cats` + `log4j2` for logging.
+  - Test stack: MUnit 1.2.x, `munit-scalacheck`, and `discipline-munit`.
+
+- Serverless packaging and deploy (optional, for AWS publication)
+  - `serverless.yml` is configured to deploy two Lambda artifacts. `sbt deploy` shells out to `serverless deploy`.
+  - Required environment for deployment (see README): `BUCKET`, `ACCOUNT`, `STAGE`, `SUBNET_ID`, `SECURITY_GROUP`.
+  - sbt will pass `DATABASE_ARTIFACT_PATH` to Serverless at deploy-time.
+  - Local deploy command string is configurable via `serverlessDeployCommand` in `build.sbt`.
+
+- Local manual invocation for development
+  - `src/test/scala/com/dwolla/postgres/init/LocalApp.scala` contains a small `IOApp` for running the handler locally against a real Postgres instance and real AWS Secrets Manager. This is for integration/manual testing; it requires valid AWS credentials, an accessible DB, and OTel environment if you want traces.
+
+- Network/SSL conventions for DB connections
+  - `CreateSkunkSession` chooses `SSL.None` for `localhost` and `SSL.System` otherwise, and picks `postgres` as the default database unless a specific `Database` is provided. This is relevant when simulating local connections.
+
+#### Testing: how to run, add, and scope tests
+- Running all tests
+  - From project root: `sbt test`. This builds the Smithy subproject if needed, compiles tests, and runs all MUnit suites under `src/test/scala`.
+
+- Running a single suite or test
+  - Single suite: `sbt "testOnly com.dwolla.postgres.init.ExtractRequestPropertiesSpec"`
+  - Single test within a suite: `sbt "testOnly com.dwolla.postgres.init.SqlStringRegexSpec -- *passwords*"` (MUnit supports test filters via `--` followed by a glob pattern matching the test name.)
+
+- Property-based and law checks
+  - The project uses ScalaCheck and Discipline. Example: `SqlStringRegexSpec` combines Arbitrary instances with `checkAll` law tests (`SemigroupTests[SqlIdentifier]`). If you add new `cats` typeclass instances, consider adding law checks here.
+
+- Example: adding and running a simple test (demonstrated and verified)
+  - A trivial MUnit suite named `DemoSanitySpec` was added temporarily to verify commands:
+    ```scala
+    package com.dwolla.postgres.init
+
+    class DemoSanitySpec extends munit.FunSuite {
+      test("demo sanity: 2 + 2 == 4") {
+        assertEquals(2 + 2, 4)
+      }
+    }
+    ```
+  - Run it directly: `sbt "testOnly com.dwolla.postgres.init.DemoSanitySpec"`.
+  - It passed locally; the file has been removed to keep the repo clean.
+
+- Adding new tests
+  - Place suites under `src/test/scala` using package `com.dwolla.postgres.init` (match sources if testing package-private APIs).
+  - Use `munit.FunSuite` for example-based tests, and add `munit-scalacheck` for generators/properties. For typeclass laws, add `DisciplineSuite` + `discipline-munit`.
+  - Prefer deterministic unit tests. For DB integration, use `LocalApp` or an ephemeral containerized Postgres with fixed schema/fixtures. Avoid hitting real AWS in automated tests.
+
+- Test selection pitfalls
+  - The test runner in this repo is MUnit; use `testOnly` with fully qualified suite names. Running a directory path (e.g., `testOnly src/test/scala`) will not work—use FQCN instead.
+
+#### Development conventions and useful internals
+- Functional style and effects
+  - Prefer tagless-final and `cats` typeclasses. Constructors are expressed via givens/context bounds where practical (e.g., `given [F[_] : {Temporal, Trace, Network, Console}]: CreateSkunkSession[F] = Session.single`).
+  - Encourage `Resource` + `Kleisli` patterns for DB sessions. See `CreateSkunkSession` extension methods like `inSession` and `recoverUndefinedAs` for ergonomics.
+
+- SQL identifiers and input validation
+  - See `SqlStringRegexSpec` for refined predicates: SQL identifiers follow `[A-Za-z][A-Za-z0-9_]*`, and generated passwords exclude dangerous punctuation. When adding features that touch SQL strings, validate via the existing newtypes/refinements to maintain safety.
+
+- Tracing & logging
+  - Natchez is integrated. Use `Trace[F]` in new effectful APIs where traces are relevant. For local runs, `LocalApp` wires `OpenTelemetryAtDwolla` and logs via SLF4J/Log4j2.
+
+- Build info and runtime metadata
+  - `BuildInfoPlugin` is enabled. Access via `com.dwolla.buildinfo.postgres.init.BuildInfo` (used in `LocalApp`) for name/version/etc. Keep it in sync when adding new runtime info.
+
+- CI nuances
+  - CI config is under `.dwollaci.yml` and GitHub Actions. CI uses JDK 17 and runs `sbt test`.
+  - If you adjust the plugin or Java version, update CI accordingly. Use `sbt githubWorkflowGenerate` to regenerate the GitHub Actions workflow YAML.
+
+#### Known quirks and fixes
+- `build.sbt` defines `serverlessDeployCommand` as `Seq[String]`. On some Scala 3 toolchains you may need an explicit `immutable.Seq` to satisfy `settingKey[Seq[String]]`. If you see a type mismatch like `found: scala.collection.Seq[String]`, change to: `serverlessDeployCommand := scala.collection.immutable.Seq.from("serverless deploy --verbose".split(' '))` or similar.
+- README JSON blocks are illustrative and may not be strict JSON; treat them as examples for CloudFormation.
+
+#### Quickstart commands (verified)
+- Full build + tests: `sbt test`
+- One suite: `sbt "testOnly com.dwolla.postgres.init.ExtractRequestPropertiesSpec"`
+- One test in a suite: `sbt "testOnly com.dwolla.postgres.init.SqlStringRegexSpec -- *passwords*"`
+- Package universal distribution: `sbt Universal/packageBin`
+- Deploy (requires env vars and Serverless): `sbt deploy`

src/main/scala/com/dwolla/postgres/init/PostgresqlDatabaseInitHandlerImpl.scala

@@ -52,6 +52,8 @@ object PostgresqlDatabaseInitHandlerImpl {
         _ <- userPasswords.traverse { userPassword =>
           userRepository.addOrUpdateUser(userPassword) >> roleRepository.addUserToRole(userPassword.user, userPassword.database)
         }
+        usersToRemove <- userRepository.findDefunctUsers(userPasswords.map(_.user), RoleRepository.roleNameForDatabase(input.name))
+        _ <- usersToRemove.traverse_(userRepository.removeUser)
       } yield ()
 
     private def createOrUpdate(userPasswords: List[UserConnectionInfo], input: DatabaseMetadata): F[PhysicalResourceId] =

src/main/scala/com/dwolla/postgres/init/aws/SecretsManagerAlg.scala

@@ -5,6 +5,7 @@ import cats.*
 import cats.effect.{Trace as _, *}
 import cats.syntax.all.*
 import cats.tagless.aop.*
+import cats.tagless.Derive
 import com.amazonaws.secretsmanager.*
 import com.dwolla.tagless.WeaveKnot
 import com.dwolla.tracing.syntax.*
@@ -15,46 +16,19 @@ import org.typelevel.log4cats.Logger
 
 trait SecretsManagerAlg[F[_]] {
   def getSecret(secretId: SecretIdType): F[Secret]
-  def getSecretAs[A : Decoder](secretId: SecretIdType): F[A]
+  def getSecretAs[A : {Decoder, TraceableValue}](secretId: SecretIdType): F[A]
 }
 
+@annotation.experimental
 object SecretsManagerAlg {
-  given Aspect[SecretsManagerAlg, TraceableValue, TraceableValue] = new Aspect[SecretsManagerAlg, TraceableValue, TraceableValue] {
-    override def weave[F[_]](af: SecretsManagerAlg[F]): SecretsManagerAlg[Aspect.Weave[F, TraceableValue, TraceableValue, *]] =
-      new SecretsManagerAlg[Aspect.Weave[F, TraceableValue, TraceableValue, *]] {
-        override def getSecret(secretId: SecretIdType): Aspect.Weave[F, TraceableValue, TraceableValue, Secret] =
-          Aspect.Weave(
-            "SecretsManagerAlg",
-            List(List(
-              Aspect.Advice.byValue("secretId", secretId),
-            )),
-            Aspect.Advice("getSecret", af.getSecret(secretId))
-          )
+  given Aspect[SecretsManagerAlg, TraceableValue, TraceableValue] = Derive.aspect
 
-        override def getSecretAs[A: Decoder](secretId: SecretIdType): Aspect.Weave[F, TraceableValue, TraceableValue, A] =
-          Aspect.Weave(
-            "SecretsManagerAlg",
-            List(
-              List(Aspect.Advice.byValue("secretId", secretId)),
-              List(Aspect.Advice.byValue("implicit decoder", Decoder[A].toString)),
-            ),
-            Aspect.Advice("getSecretAs", af.getSecretAs[A](secretId))(using TraceableValue[String].contramap[A](_ => "redacted successfully parsed and decoded secret"))
-          )
-      }
-
-    override def mapK[F[_], G[_]](af: SecretsManagerAlg[F])(fk: F ~> G): SecretsManagerAlg[G] =
-      new SecretsManagerAlg[G] {
-        override def getSecret(secretId: SecretIdType): G[Secret] = fk(af.getSecret(secretId))
-        override def getSecretAs[A: Decoder](secretId: SecretIdType): G[A] = fk(af.getSecretAs[A](secretId))
-      }
-  }
-
-  def apply[F[_] : Async : Logger : Trace](client: SecretsManager[F]): SecretsManagerAlg[F] =
+  def apply[F[_] : {Async, Logger, Trace}](client: SecretsManager[F]): SecretsManagerAlg[F] =
     WeaveKnot[SecretsManagerAlg, F](apply(client, _))(_.traceWithInputsAndOutputs)
 
-  private def apply[F[_] : Async : Logger](client: SecretsManager[F],
-                                   self: Eval[SecretsManagerAlg[F]],
-                                  ): SecretsManagerAlg[F] = new SecretsManagerAlg[F] {
+  private def apply[F[_] : {Async, Logger}](client: SecretsManager[F],
+                                            self: Eval[SecretsManagerAlg[F]],
+                                           ): SecretsManagerAlg[F] = new SecretsManagerAlg[F] {
     private val parser = new JawnParser
 
     override def getSecret(secretId: SecretIdType): F[Secret] =
@@ -71,7 +45,7 @@ object SecretsManagerAlg {
               NoSecretInResponseException(secretId).raiseError[F, Secret]
           }
 
-    override def getSecretAs[A: Decoder](secretId: SecretIdType): F[A] =
+    override def getSecretAs[A : {Decoder, TraceableValue}](secretId: SecretIdType): F[A] =
       self.value.getSecret(secretId)
         .flatMap {
           case SecretString(SecretStringType(value)) => parser.parse(value).liftTo[F]

src/main/scala/com/dwolla/postgres/init/repositories/CreateSkunkSession.scala

@@ -41,7 +41,7 @@ object CreateSkunkSession {
                   username: MasterDatabaseUsername,
                   password: MasterDatabasePassword,
                  )
-                 (using CreateSkunkSession[F], MonadCancelThrow[F]): F[A] =
+                 (using CreateSkunkSession[F], MonadCancelThrow[F], Trace[F]): F[A] =
       impl(host, port, username, password, none)
 
     def inSession(host: Host,
@@ -50,24 +50,27 @@ object CreateSkunkSession {
                   password: MasterDatabasePassword,
                   database: Database,
                  )
-                 (using CreateSkunkSession[F], MonadCancelThrow[F]): F[A] =
+                 (using CreateSkunkSession[F], MonadCancelThrow[F], Trace[F]): F[A] =
       impl(host, port, username, password, database.some)
 
     private def impl(host: Host,
-                  port: Port,
-                  username: MasterDatabaseUsername,
-                  password: MasterDatabasePassword,
-                  database: Option[Database],
-                 )
-                 (using CreateSkunkSession[F], MonadCancelThrow[F]): F[A] =
-      CreateSkunkSession[F].single(
-        host = host.show,
-        port = port.value,
-        user = username.value.value,
-        database = database.map(_.value).getOrElse(sqlIdentifier"postgres").value,
-        password = password.value.some,
-        ssl = if (host == host"localhost") SSL.None else SSL.System,
-      ).use(kleisli.run)
+                     port: Port,
+                     username: MasterDatabaseUsername,
+                     password: MasterDatabasePassword,
+                     database: Option[Database],
+                    )
+                    (using CreateSkunkSession[F], MonadCancelThrow[F], Trace[F]): F[A] =
+      Trace[F].span("database.session") {
+        Trace[F].put("database" -> database.map(_.value).getOrElse(sqlIdentifier"postgres").value) >>
+          CreateSkunkSession[F].single(
+            host = host.show,
+            port = port.value,
+            user = username.value.value,
+            database = database.map(_.value).getOrElse(sqlIdentifier"postgres").value,
+            password = password.value.some,
+            ssl = if (host == host"localhost") SSL.None else SSL.System,
+          ).use(kleisli.run)
+      }
 
   extension [F[_], A](fa: F[A])
     def recoverUndefinedAs(a: A)

src/main/scala/com/dwolla/postgres/init/repositories/UserRepository.scala

@@ -21,11 +21,16 @@ import scala.concurrent.duration.*
 trait UserRepository[F[_]] {
   def addOrUpdateUser(userConnectionInfo: UserConnectionInfo): F[Username]
   def removeUser(username: Username): F[Username]
+  def findDefunctUsers(usersThatShouldExist: List[Username], role: RoleName): F[List[Username]]
 }
 
 @annotation.experimental
 object UserRepository {
-  given Aspect[UserRepository, TraceableValue, TraceableValue] = Derive.aspect
+  given Aspect[UserRepository, TraceableValue, TraceableValue] = {
+    given TraceableValue[List[Username]] = TraceableValue[String].contramap(_.mkString("[", ", ", "]"))
+
+    Derive.aspect
+  }
 
   def usernameForDatabase(database: Database): Username =
     Username(database.value)
@@ -82,13 +87,23 @@ object UserRepository {
 
     override def removeUser(username: Username): Kleisli[F, Session[F], Username] =
       removeUser(username, 5)
+
+    override def findDefunctUsers(usersThatShouldExist: List[Username],
+                                  roleName: RoleName,
+                                 ): Kleisli[F, Session[F], List[Username]] =
+      Kleisli[F, Session[F], List[Username]] {
+        _.execute(UserQueries.findDefunctUsers(usersThatShouldExist))(usersThatShouldExist *: roleName *: EmptyTuple)
+      }
   }.traceWithInputsAndOutputs
 }
 
 object UserQueries {
   private val username: skunk.Codec[Username] =
     name.eimap[Username](refineV[SqlIdentifierPredicate](_).map(Username(_)))(_.value.value)
 
+  private val roleName: skunk.Codec[RoleName] =
+    name.eimap[RoleName](refineV[SqlIdentifierPredicate](_).map(RoleName(_)))(_.value.value)
+
   val checkUserExists: Query[Username, Username] =
     sql"SELECT u.usename FROM pg_catalog.pg_user u WHERE u.usename = $username"
       .query(username)
@@ -106,4 +121,20 @@ object UserQueries {
   def removeUser(username: Username): Command[Void] =
     sql"DROP USER IF EXISTS #${username.value.value}"
       .command
+
+  def findDefunctUsers(users: List[Username]): Query[users.type *: RoleName *: EmptyTuple, Username] =
+    sql"""
+      WITH expected(role_name) AS (VALUES (${username.list(users)}))
+      SELECT
+          m.rolname AS user_name
+      FROM pg_auth_members rm
+               JOIN pg_roles r ON r.oid = rm.roleid
+               JOIN pg_roles m ON m.oid = rm.member
+      WHERE r.rolname = $roleName
+        AND m.rolcanlogin = TRUE
+        AND m.rolname NOT IN (SELECT role_name FROM expected)
+      ORDER BY user_name;
+     """
+      .query(username)
+
 }