diff --git a/internal/api/roles.go b/internal/api/roles.go index f0c93755a..50166100c 100644 --- a/internal/api/roles.go +++ b/internal/api/roles.go @@ -1,6 +1,7 @@ package api import ( + "errors" "net/http" "time" @@ -8,6 +9,8 @@ import ( "go.infratographer.com/x/gidx" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" + + "go.infratographer.com/permissions-api/internal/query" ) const ( @@ -60,7 +63,8 @@ func (r *Router) roleCreate(c echo.Context) error { Name: role.Name, Actions: role.Actions, ResourceID: role.ResourceID, - Creator: role.CreatorID, + CreatedBy: role.CreatedBy, + UpdatedBy: role.UpdatedBy, CreatedAt: role.CreatedAt.Format(time.RFC3339), UpdatedAt: role.UpdatedAt.Format(time.RFC3339), } @@ -100,7 +104,11 @@ func (r *Router) roleUpdate(c echo.Context) error { // check on the role resource. resource, err := r.engine.GetRoleResource(ctx, roleResource) if err != nil { - return echo.NewHTTPError(http.StatusBadRequest, "error getting resource").SetInternal(err) + if errors.Is(err, query.ErrRoleNotFound) { + return echo.NewHTTPError(http.StatusNotFound, "resource not found").SetInternal(err) + } + + return echo.NewHTTPError(http.StatusInternalServerError, "error getting resource").SetInternal(err) } if err := r.checkActionWithResponse(ctx, subjectResource, actionRoleUpdate, resource); err != nil { @@ -117,7 +125,8 @@ func (r *Router) roleUpdate(c echo.Context) error { Name: role.Name, Actions: role.Actions, ResourceID: role.ResourceID, - Creator: role.CreatorID, + CreatedBy: role.CreatedBy, + UpdatedBy: role.UpdatedBy, CreatedAt: role.CreatedAt.Format(time.RFC3339), UpdatedAt: role.UpdatedAt.Format(time.RFC3339), } @@ -169,7 +178,8 @@ func (r *Router) roleGet(c echo.Context) error { Name: role.Name, Actions: role.Actions, ResourceID: role.ResourceID, - Creator: role.CreatorID, + CreatedBy: role.CreatedBy, + UpdatedBy: role.UpdatedBy, CreatedAt: role.CreatedAt.Format(time.RFC3339), UpdatedAt: role.UpdatedAt.Format(time.RFC3339), } @@ -216,7 +226,8 @@ func (r *Router) rolesList(c echo.Context) error { ID: role.ID, Name: role.Name, Actions: role.Actions, - Creator: role.CreatorID, + CreatedBy: role.CreatedBy, + UpdatedBy: role.UpdatedBy, CreatedAt: role.CreatedAt.Format(time.RFC3339), UpdatedAt: role.UpdatedAt.Format(time.RFC3339), } diff --git a/internal/api/types.go b/internal/api/types.go index 71825d067..e02e96c71 100644 --- a/internal/api/types.go +++ b/internal/api/types.go @@ -20,7 +20,8 @@ type roleResponse struct { Actions []string `json:"actions"` ResourceID gidx.PrefixedID `json:"resource_id,omitempty"` - Creator gidx.PrefixedID `json:"creator"` + CreatedBy gidx.PrefixedID `json:"created_by"` + UpdatedBy gidx.PrefixedID `json:"updated_by"` CreatedAt string `json:"created_at"` UpdatedAt string `json:"updated_at"` } diff --git a/internal/query/mock/mock.go b/internal/query/mock/mock.go index d7c5d6439..c9e9a5897 100644 --- a/internal/query/mock/mock.go +++ b/internal/query/mock/mock.go @@ -54,7 +54,8 @@ func (e *Engine) CreateRole(ctx context.Context, actor, res types.Resource, name ID: gidx.MustNewID(query.ApplicationPrefix), Name: name, Actions: outActions, - CreatorID: actor.ID, + CreatedBy: actor.ID, + UpdatedBy: actor.ID, CreatedAt: time.Now(), UpdatedAt: time.Now(), } @@ -62,9 +63,13 @@ func (e *Engine) CreateRole(ctx context.Context, actor, res types.Resource, name return role, nil } -// UpdateRole returns nothing but satisfies the Engine interface. +// UpdateRole returns the provided mock results. func (e *Engine) UpdateRole(ctx context.Context, actor, roleResource types.Resource, newName string, newActions []string) (types.Role, error) { - return types.Role{}, nil + args := e.Called(actor, roleResource, newName, newActions) + + retRole := args.Get(0).(types.Role) + + return retRole, args.Error(1) } // GetRole returns nothing but satisfies the Engine interface. diff --git a/internal/query/relations.go b/internal/query/relations.go index 9f495d89f..927856249 100644 --- a/internal/query/relations.go +++ b/internal/query/relations.go @@ -306,10 +306,16 @@ func (e *engine) CreateRole(ctx context.Context, actor, res types.Resource, role logRollbackErr(e.logger, e.store.RollbackContext(dbCtx)) + // No rollback of spicedb relations are done here. + // This does result in dangling unused entries in spicedb, + // however there are no assignments to these newly created + // and now discarded roles and so they won't be used. + return types.Role{}, err } - role.CreatorID = dbRole.CreatorID + role.CreatedBy = dbRole.CreatedBy + role.UpdatedBy = dbRole.UpdatedBy role.ResourceID = dbRole.ResourceID role.CreatedAt = dbRole.CreatedAt role.UpdatedAt = dbRole.UpdatedAt @@ -360,48 +366,35 @@ func (e *engine) UpdateRole(ctx context.Context, actor, roleResource types.Resou defer span.End() - role, err := e.GetRole(ctx, roleResource) + dbCtx, err := e.store.BeginContext(ctx) + if err != nil { + return types.Role{}, err + } + + role, err := e.GetRole(dbCtx, roleResource) if err != nil { return types.Role{}, err } newName = strings.TrimSpace(newName) - // If the name is the same, then clear the changed name. - if role.Name == newName { - newName = "" + if newName == "" { + newName = role.Name } addActions, remActions := actionsDiff(role.Actions, newActions) // If no changes, return existing role with no changes. - if newName == "" && len(addActions) == 0 && len(remActions) == 0 { + if newName == role.Name && len(addActions) == 0 && len(remActions) == 0 { return role, nil } - resourceID := role.ResourceID - - // If the resource id is not found in the permissions database, then we must locate it in the spicedb database. - if resourceID == gidx.NullPrefixedID { - resource, err := e.GetRoleResource(ctx, roleResource) - if err != nil { - return types.Role{}, fmt.Errorf("failed to locate roles associated resource: %s: %w", roleResource.ID.String(), err) - } - - resourceID = resource.ID - } - - resource, err := e.NewResourceFromID(resourceID) + resource, err := e.NewResourceFromID(role.ResourceID) if err != nil { return types.Role{}, err } - dbCtx, err := e.store.BeginContext(ctx) - if err != nil { - return types.Role{}, err - } - - dbRole, err := e.store.UpdateRole(dbCtx, role.ID, newName) + dbRole, err := e.store.UpdateRole(dbCtx, actor.ID, role.ID, newName) if err != nil { logRollbackErr(e.logger, e.store.RollbackContext(dbCtx)) @@ -432,11 +425,14 @@ func (e *engine) UpdateRole(ctx context.Context, actor, roleResource types.Resou logRollbackErr(e.logger, e.store.RollbackContext(dbCtx)) + // TODO: add spicedb rollback logic. + return types.Role{}, err } role.Name = dbRole.Name - role.CreatorID = dbRole.CreatorID + role.CreatedBy = dbRole.CreatedBy + role.UpdatedBy = dbRole.UpdatedBy role.ResourceID = dbRole.ResourceID role.CreatedAt = dbRole.CreatedAt role.UpdatedAt = dbRole.UpdatedAt @@ -858,7 +854,8 @@ func (e *engine) ListRoles(ctx context.Context, resource types.Resource) ([]type for i, role := range out { if dbRole, ok := rolesByID[role.ID.String()]; ok { role.Name = dbRole.Name - role.CreatorID = dbRole.CreatorID + role.CreatedBy = dbRole.CreatedBy + role.UpdatedBy = dbRole.UpdatedBy role.CreatedAt = dbRole.CreatedAt role.UpdatedAt = dbRole.UpdatedAt @@ -956,7 +953,8 @@ func (e *engine) GetRole(ctx context.Context, roleResource types.Resource) (type Actions: actions, ResourceID: dbRole.ResourceID, - CreatorID: dbRole.CreatorID, + CreatedBy: dbRole.CreatedBy, + UpdatedBy: dbRole.UpdatedBy, CreatedAt: dbRole.CreatedAt, UpdatedAt: dbRole.UpdatedAt, }, nil diff --git a/internal/storage/context.go b/internal/storage/context.go index a7437dd3e..48fc67ef9 100644 --- a/internal/storage/context.go +++ b/internal/storage/context.go @@ -39,6 +39,19 @@ func getContextTx(ctx context.Context) (*sql.Tx, error) { } } +func getContextDBQuery(ctx context.Context, def DBQuery) (DBQuery, error) { + tx, err := getContextTx(ctx) + + switch err { + case nil: + return tx, nil + case ErrorMissingContextTx: + return def, nil + default: + return nil, err + } +} + func commitContextTx(ctx context.Context) error { tx, err := getContextTx(ctx) if err != nil { diff --git a/internal/storage/errors.go b/internal/storage/errors.go index a04d8e44b..c264eb909 100644 --- a/internal/storage/errors.go +++ b/internal/storage/errors.go @@ -27,17 +27,30 @@ var ( ErrorInvalidContextTx = errors.New("invalid type for transaction context") ) +const ( + pqIndexRolesPrimaryKey = "roles_pkey" + pqIndexRolesResourceIDName = "roles_resource_id_name" +) + +// pqIsRoleAlreadyExistsError checks that the provided error is a postgres error. +// If so, checks if postgres threw a unique_violation error on the roles primary key index. +// If postgres has raised a unique violation error on this index it means a record already exists +// with a matching primary key (role id). func pqIsRoleAlreadyExistsError(err error) bool { if pqErr, ok := err.(*pq.Error); ok { - return pqErr.Code.Name() == "unique_violation" && pqErr.Constraint == "roles_pkey" + return pqErr.Code.Name() == "unique_violation" && pqErr.Constraint == pqIndexRolesPrimaryKey } return false } +// pqIsRoleNameTakenError checks that the provided error is a postgres error. +// If so, checks if postgres threw a unique_violation error on the roles resource id name index. +// If postgres has raised a unique violation error on this index it means a record already exists +// with the same resource id and role name combination. func pqIsRoleNameTakenError(err error) bool { if pqErr, ok := err.(*pq.Error); ok { - return pqErr.Code.Name() == "unique_violation" && pqErr.Constraint == "roles_resource_id_name" + return pqErr.Code.Name() == "unique_violation" && pqErr.Constraint == pqIndexRolesResourceIDName } return false diff --git a/internal/storage/migrations/20231122000000_initial_schema.sql b/internal/storage/migrations/20231122000000_initial_schema.sql index a73352f95..058fcdd25 100644 --- a/internal/storage/migrations/20231122000000_initial_schema.sql +++ b/internal/storage/migrations/20231122000000_initial_schema.sql @@ -4,13 +4,16 @@ CREATE TABLE "roles" ( "id" character varying NOT NULL, "name" character varying(64) NOT NULL, "resource_id" character varying NOT NULL, - "creator_id" character varying NOT NULL, + "created_by" character varying NOT NULL, + "updated_by" character varying NOT NULL, "created_at" timestamptz NOT NULL, "updated_at" timestamptz NOT NULL, PRIMARY KEY ("id") ); --- create index "roles_creator_id" to table: "roles" -CREATE INDEX "roles_creator_id" ON "roles" ("creator_id"); +-- create index "roles_created_by" to table: "roles" +CREATE INDEX "roles_created_by" ON "roles" ("created_by"); +-- create index "roles_created_by" to table: "roles" +CREATE INDEX "roles_updated_by" ON "roles" ("updated_by"); -- create index "roles_created_at" to table: "roles" CREATE INDEX "roles_created_at" ON "roles" ("created_at"); -- create index "roles_updated_at" to table: "roles" @@ -24,7 +27,9 @@ DROP INDEX "roles_resource_id_name"; DROP INDEX "roles_updated_at"; -- reverse: create index "roles_created_at" to table: "roles" DROP INDEX "roles_created_at"; --- reverse: create index "roles_creator_id" to table: "roles" -DROP INDEX "roles_creator_id"; +-- reverse: create index "roles_updated_by" to table: "roles" +DROP INDEX "roles_updated_by"; +-- reverse: create index "roles_created_by" to table: "roles" +DROP INDEX "roles_created_by"; -- reverse: create "roles" table DROP TABLE "roles"; diff --git a/internal/storage/roles.go b/internal/storage/roles.go index 51858d348..d3cd24fc6 100644 --- a/internal/storage/roles.go +++ b/internal/storage/roles.go @@ -16,7 +16,7 @@ type RoleService interface { GetResourceRoleByName(ctx context.Context, resourceID gidx.PrefixedID, name string) (Role, error) ListResourceRoles(ctx context.Context, resourceID gidx.PrefixedID) ([]Role, error) CreateRole(ctx context.Context, actorID gidx.PrefixedID, roleID gidx.PrefixedID, name string, resourceID gidx.PrefixedID) (Role, error) - UpdateRole(ctx context.Context, roleID gidx.PrefixedID, name string) (Role, error) + UpdateRole(ctx context.Context, actorID, roleID gidx.PrefixedID, name string) (Role, error) DeleteRole(ctx context.Context, roleID gidx.PrefixedID) (Role, error) } @@ -25,7 +25,8 @@ type Role struct { ID gidx.PrefixedID Name string ResourceID gidx.PrefixedID - CreatorID gidx.PrefixedID + CreatedBy gidx.PrefixedID + UpdatedBy gidx.PrefixedID CreatedAt time.Time UpdatedAt time.Time } @@ -33,14 +34,20 @@ type Role struct { // GetRoleByID retrieves a role from the database by the provided prefixed ID. // If no role exists an ErrRoleNotFound error is returned. func (e *engine) GetRoleByID(ctx context.Context, id gidx.PrefixedID) (Role, error) { + db, err := getContextDBQuery(ctx, e) + if err != nil { + return Role{}, err + } + var role Role - err := e.QueryRowContext(ctx, ` + err = db.QueryRowContext(ctx, ` SELECT id, name, resource_id, - creator_id, + created_by, + updated_by, created_at, updated_at FROM roles @@ -50,7 +57,8 @@ func (e *engine) GetRoleByID(ctx context.Context, id gidx.PrefixedID) (Role, err &role.ID, &role.Name, &role.ResourceID, - &role.CreatorID, + &role.CreatedBy, + &role.UpdatedBy, &role.CreatedAt, &role.UpdatedAt, ) @@ -69,14 +77,20 @@ func (e *engine) GetRoleByID(ctx context.Context, id gidx.PrefixedID) (Role, err // GetResourceRoleByName retrieves a role from the database by the provided resource ID and role name. // If no role exists an ErrRoleNotFound error is returned. func (e *engine) GetResourceRoleByName(ctx context.Context, resourceID gidx.PrefixedID, name string) (Role, error) { + db, err := getContextDBQuery(ctx, e) + if err != nil { + return Role{}, err + } + var role Role - err := e.QueryRowContext(ctx, ` + err = db.QueryRowContext(ctx, ` SELECT id, name, resource_id, - creator_id, + created_by, + updated_by, created_at, updated_at FROM roles @@ -90,7 +104,8 @@ func (e *engine) GetResourceRoleByName(ctx context.Context, resourceID gidx.Pref &role.ID, &role.Name, &role.ResourceID, - &role.CreatorID, + &role.CreatedBy, + &role.UpdatedBy, &role.CreatedAt, &role.UpdatedAt, ) @@ -109,12 +124,18 @@ func (e *engine) GetResourceRoleByName(ctx context.Context, resourceID gidx.Pref // ListResourceRoles retrieves all roles associated with the provided resource ID. // If no roles are found an empty slice is returned. func (e *engine) ListResourceRoles(ctx context.Context, resourceID gidx.PrefixedID) ([]Role, error) { - rows, err := e.QueryContext(ctx, ` + db, err := getContextDBQuery(ctx, e) + if err != nil { + return nil, err + } + + rows, err := db.QueryContext(ctx, ` SELECT id, name, resource_id, - creator_id, + created_by, + updated_by, created_at, updated_at FROM roles @@ -133,7 +154,7 @@ func (e *engine) ListResourceRoles(ctx context.Context, resourceID gidx.Prefixed for rows.Next() { var role Role - if err := rows.Scan(&role.ID, &role.Name, &role.ResourceID, &role.CreatorID, &role.CreatedAt, &role.UpdatedAt); err != nil { + if err := rows.Scan(&role.ID, &role.Name, &role.ResourceID, &role.CreatedBy, &role.UpdatedBy, &role.CreatedAt, &role.UpdatedAt); err != nil { return nil, err } @@ -159,15 +180,16 @@ func (e *engine) CreateRole(ctx context.Context, actorID, roleID gidx.PrefixedID err = tx.QueryRowContext(ctx, ` INSERT - INTO roles (id, name, resource_id, creator_id, created_at, updated_at) - VALUES ($1, $2, $3, $4, now(), now()) - RETURNING id, name, resource_id, creator_id, created_at, updated_at + INTO roles (id, name, resource_id, created_by, updated_by, created_at, updated_at) + VALUES ($1, $2, $3, $4, $4, now(), now()) + RETURNING id, name, resource_id, created_by, updated_by, created_at, updated_at `, roleID.String(), name, resourceID.String(), actorID.String(), ).Scan( &role.ID, &role.Name, &role.ResourceID, - &role.CreatorID, + &role.CreatedBy, + &role.UpdatedBy, &role.CreatedAt, &role.UpdatedAt, ) @@ -187,41 +209,29 @@ func (e *engine) CreateRole(ctx context.Context, actorID, roleID gidx.PrefixedID return role, nil } -// UpdateRole updates an existing role if one exists. -// If no role already exists, a new role is created in the same way as CreateRole. +// UpdateRole updates an existing role. // If changing the name and the new name results in a duplicate name error, an ErrRoleNameTaken error is returned. // // This method must be called with a context returned from BeginContext. // CommitContext or RollbackContext must be called afterwards if this method returns no error. -func (e *engine) UpdateRole(ctx context.Context, roleID gidx.PrefixedID, name string) (Role, error) { +func (e *engine) UpdateRole(ctx context.Context, actorID, roleID gidx.PrefixedID, name string) (Role, error) { tx, err := getContextTx(ctx) if err != nil { return Role{}, err } - var row *sql.Row - - // If no name is provided, only update the updated_at timestamp. - if name == "" { - row = tx.QueryRowContext(ctx, ` - UPDATE roles SET updated_at = now() WHERE id = $1 - RETURNING id, name, resource_id, creator_id, created_at, updated_at - `, roleID.String()) - } else { - row = tx.QueryRowContext(ctx, ` - UPDATE roles SET name = $1, updated_at = now() WHERE id = $2 - RETURNING id, name, resource_id, creator_id, created_at, updated_at - `, name, roleID.String(), - ) - } - var role Role - err = row.Scan( + err = tx.QueryRowContext(ctx, ` + UPDATE roles SET name = $1, updated_by = $2, updated_at = now() WHERE id = $3 + RETURNING id, name, resource_id, created_by, updated_by, created_at, updated_at + `, name, actorID.String(), roleID.String(), + ).Scan( &role.ID, &role.Name, &role.ResourceID, - &role.CreatorID, + &role.CreatedBy, + &role.UpdatedBy, &role.CreatedAt, &role.UpdatedAt, ) diff --git a/internal/storage/roles_test.go b/internal/storage/roles_test.go index 14771e64a..1e62a9fb6 100644 --- a/internal/storage/roles_test.go +++ b/internal/storage/roles_test.go @@ -54,7 +54,7 @@ func TestGetRoleByID(t *testing.T) { assert.Equal(t, roleID, res.Success.ID) assert.Equal(t, roleName, res.Success.Name) assert.Equal(t, resourceID, res.Success.ResourceID) - assert.Equal(t, actorID, res.Success.CreatorID) + assert.Equal(t, actorID, res.Success.CreatedBy) assert.Equal(t, createdRole.CreatedAt, res.Success.CreatedAt) assert.Equal(t, createdRole.UpdatedAt, res.Success.UpdatedAt) }, @@ -124,7 +124,7 @@ func TestListResourceRoles(t *testing.T) { require.NotEmpty(t, role.Name) assert.Equal(t, groups[role.Name], role.ID) assert.Equal(t, resourceID, role.ResourceID) - assert.Equal(t, actorID, role.CreatorID) + assert.Equal(t, actorID, role.CreatedBy) assert.False(t, role.CreatedAt.IsZero()) assert.False(t, role.UpdatedAt.IsZero()) } @@ -172,7 +172,7 @@ func TestCreateRole(t *testing.T) { assert.Equal(t, "permrol-abc123", res.Success.ID.String()) assert.Equal(t, "admins", res.Success.Name) assert.Equal(t, resourceID, res.Success.ResourceID) - assert.Equal(t, actorID, res.Success.CreatorID) + assert.Equal(t, actorID, res.Success.CreatedBy) assert.False(t, res.Success.CreatedAt.IsZero()) assert.False(t, res.Success.UpdatedAt.IsZero()) }, @@ -290,7 +290,7 @@ func TestUpdateRole(t *testing.T) { assert.Equal(t, role1ID, res.Success.ID) assert.Equal(t, "root-admins", res.Success.Name) - assert.Equal(t, actorID, res.Success.CreatorID) + assert.Equal(t, actorID, res.Success.CreatedBy) assert.Equal(t, createdDBRole1.CreatedAt, res.Success.CreatedAt) assert.NotEqual(t, createdDBRole1.UpdatedAt, res.Success.UpdatedAt) }, @@ -320,7 +320,7 @@ func TestUpdateRole(t *testing.T) { return result } - result.Success, result.Err = store.UpdateRole(dbCtx, input.id, input.name) + result.Success, result.Err = store.UpdateRole(dbCtx, actorID, input.id, input.name) if result.Err != nil { store.RollbackContext(dbCtx) //nolint:errcheck // skip check in test diff --git a/internal/storage/storage.go b/internal/storage/storage.go index ba050002f..ea1d64dc2 100644 --- a/internal/storage/storage.go +++ b/internal/storage/storage.go @@ -20,9 +20,16 @@ type Storage interface { // *sql.DB implements these methods. type DB interface { BeginTx(ctx context.Context, opts *sql.TxOptions) (*sql.Tx, error) + PingContext(ctx context.Context) error + + DBQuery +} + +// DBQuery are required methods for querying the database. +type DBQuery interface { QueryContext(ctx context.Context, query string, args ...any) (*sql.Rows, error) QueryRowContext(ctx context.Context, query string, args ...any) *sql.Row - PingContext(ctx context.Context) error + ExecContext(ctx context.Context, query string, args ...any) (sql.Result, error) } type engine struct { diff --git a/internal/types/types.go b/internal/types/types.go index 99548b687..347043507 100644 --- a/internal/types/types.go +++ b/internal/types/types.go @@ -14,7 +14,8 @@ type Role struct { Actions []string ResourceID gidx.PrefixedID - CreatorID gidx.PrefixedID + CreatedBy gidx.PrefixedID + UpdatedBy gidx.PrefixedID CreatedAt time.Time UpdatedAt time.Time }