-
Notifications
You must be signed in to change notification settings - Fork 0
[CMCSMACD-4695] Add a function for syncing users and roles #10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,126 @@ | ||
| package migrations | ||
|
|
||
| import ( | ||
| "database/sql" | ||
| "fmt" | ||
| "log" | ||
|
|
||
| "github.com/jmoiron/sqlx" | ||
| "github.com/lib/pq" | ||
| ) | ||
|
|
||
| type PostgreSQLUser struct { | ||
| Username string | ||
| GrantRoles []string | ||
| } | ||
|
|
||
| type UserAuthenticationType string | ||
|
|
||
| const ( | ||
| UserAuthenticationTypeIAM UserAuthenticationType = "iam" | ||
| UserAuthenticationTypePassword UserAuthenticationType = "password" | ||
| ) | ||
|
|
||
| // Make sure that the given users exist in database cluster and have only the | ||
| // role memberships specified. If authType is UserAuthenticationTypePassword, | ||
| // set each user's password to its username. Otherwise remove each user's password | ||
| // and also add the rds_iam role for each user. | ||
| // All operations are done in a single transaction. | ||
| func EnsureUsersWithRoles(db *sqlx.DB, users []PostgreSQLUser, authType UserAuthenticationType) error { | ||
| tx, err := db.Begin() | ||
| if err != nil { | ||
| return fmt.Errorf("Error starting transaction: %w", err) | ||
| } | ||
| defer func() { | ||
| err := tx.Rollback() | ||
| if err != nil && err != sql.ErrTxDone { | ||
| log.Printf("Error rolling back: %s", err) | ||
| } | ||
| }() | ||
|
|
||
| for _, user := range users { | ||
| createUserSQL := fmt.Sprintf(` | ||
| DO $$ | ||
| DECLARE | ||
| username text := %s; | ||
| BEGIN | ||
| IF NOT EXISTS ( | ||
| SELECT FROM pg_catalog.pg_user WHERE usename = username | ||
| ) THEN | ||
| EXECUTE format('CREATE USER %%I', username); | ||
| END IF; | ||
| END | ||
| $$`, pq.QuoteLiteral(user.Username)) | ||
| _, err := tx.Exec(createUserSQL) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to create user %q: %w", user.Username, err) | ||
| } | ||
|
|
||
| // Drop all existing roles | ||
| dropRolesSQL := fmt.Sprintf(` | ||
| DO $$ | ||
| DECLARE | ||
| r RECORD; | ||
| BEGIN | ||
| FOR r IN | ||
| SELECT roleid::regrole AS granted_role | ||
| FROM pg_catalog.pg_auth_members | ||
| WHERE member = %s::regrole | ||
| LOOP | ||
| EXECUTE format('REVOKE %%I FROM %s', r.granted_role); | ||
| END LOOP; | ||
| END | ||
| $$;`, pq.QuoteLiteral(user.Username), pq.QuoteIdentifier(user.Username)) | ||
| _, err = tx.Exec(dropRolesSQL) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to drop roles for user %q: %w", user.Username, err) | ||
| } | ||
|
|
||
| // There could be privileges on a variety of different objects. | ||
| // See https://www.postgresql.org/docs/current/sql-revoke.html | ||
| // But we will just worry about roles. | ||
|
|
||
| // Add roles | ||
| roles := user.GrantRoles | ||
| if authType == UserAuthenticationTypeIAM { | ||
| roles = append(roles, "rds_iam") | ||
| } | ||
| for _, role := range roles { | ||
| grantSQL := fmt.Sprintf("GRANT %s TO %s", pq.QuoteIdentifier(role), pq.QuoteIdentifier(user.Username)) | ||
| _, err = tx.Exec(grantSQL) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to give role %q to user %q: %w", role, user.Username, err) | ||
| } | ||
| } | ||
|
|
||
| // Set or remove password | ||
| switch authType { | ||
| case UserAuthenticationTypePassword: | ||
| _, err = tx.Exec( | ||
| fmt.Sprintf("ALTER USER %s WITH PASSWORD %s", | ||
| pq.QuoteIdentifier(user.Username), | ||
| pq.QuoteLiteral(user.Username)), | ||
| ) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to set password for user %q: %w", user.Username, err) | ||
| } | ||
| case UserAuthenticationTypeIAM: | ||
| _, err = tx.Exec( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think here we will also need to grant the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the caller will pass
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here's how it looks on my branch. UsePasswordAuth is false if we detect the Lambda runtime func ensureUsersAndMigrate(usePasswordAuth bool) error {
users := []migrations.PostgreSQLUser{
{
Username: "reader",
GrantRoles: []string{
postgresReadAllRole,
},
},
{
Username: "writer",
GrantRoles: []string{
postgresReadAllRole,
postgresWriteAllRole,
},
},
}
// When using RDS IAM auth instead of passwords, users also need the rds_iam role
if !usePasswordAuth {
for i := range users {
users[i].GrantRoles = append(users[i].GrantRoles, rdsIAMRole)
}
}
err := migrations.EnsureUsersWithRoles(db, users, usePasswordAuth)
if err != nil {
return fmt.Errorf("error ensuring users with roles: %w", err)
}
return migrations.Migrate(db, schema.AllMigrations())
}There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, okay, so that logic is in the caller — makes sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, on second thought, maybe we do want to include that in the function since everyone will want it and it simplifies the caller.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would work too
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check it out now and let me know what you all think. |
||
| fmt.Sprintf("ALTER USER %s WITH PASSWORD NULL", | ||
| pq.QuoteIdentifier(user.Username)), | ||
| ) | ||
| if err != nil { | ||
| return fmt.Errorf("Failed to remove password for user %q: %w", user.Username, err) | ||
| } | ||
| default: | ||
| return fmt.Errorf("Invalid authType %q", authType) | ||
| } | ||
| } | ||
|
|
||
| err = tx.Commit() | ||
| if err != nil { | ||
| return fmt.Errorf("Error committing transaction: %w", err) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option would be to create optimistically and catch an exception (static, less interpolation):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do kind of like that, but also reading the docs about exception handling makes me a little nervous. They recommend not doing it if you can avoid it (although I think just for performance reasons?), and there are some semantics regarding transactions that I don't think would matter here but I'm not 100% confident. I'm inclined to leave it as is.