From b8977a312fb8d17bd0e07a50586562e4fa4cebb6 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Wed, 24 Jan 2024 14:20:56 +0200 Subject: [PATCH] Hook up authz tuple writing to Minder server (#2179) This hooks up the initial creation of tuples for authorization in the server. Tuples define the relationships between users and projects in minder via a role. The intent is for these to happen first, and roll them back if database operations fail. --- cmd/server/app/serve.go | 12 +++++- database/mock/store.go | 15 ++++++++ database/query/projects.sql | 10 +++++ internal/controlplane/default_project.go | 29 ++++++++++++-- internal/controlplane/handlers_user.go | 4 +- internal/controlplane/handlers_user_test.go | 11 +++++- internal/controlplane/identity_events.go | 33 +++++++++++++--- internal/controlplane/identity_events_test.go | 5 ++- internal/controlplane/server.go | 8 ++-- internal/db/projects.sql.go | 38 +++++++++++++++++++ internal/db/querier.go | 1 + 11 files changed, 146 insertions(+), 20 deletions(-) diff --git a/cmd/server/app/serve.go b/cmd/server/app/serve.go index f215003f28..af3cc26ce4 100644 --- a/cmd/server/app/serve.go +++ b/cmd/server/app/serve.go @@ -29,6 +29,7 @@ import ( "golang.org/x/sync/errgroup" "github.com/stacklok/minder/internal/auth" + "github.com/stacklok/minder/internal/authz/mock" serverconfig "github.com/stacklok/minder/internal/config/server" "github.com/stacklok/minder/internal/controlplane" "github.com/stacklok/minder/internal/db" @@ -104,7 +105,10 @@ var serveCmd = &cobra.Command{ return fmt.Errorf("failed to fetch and cache identity provider JWKS: %w\n", err) } - err = controlplane.SubscribeToIdentityEvents(ctx, store, cfg) + // TODO: Change this for an actual authz client + authzc := &mock.NoopClient{Authorized: true} + + err = controlplane.SubscribeToIdentityEvents(ctx, store, authzc, cfg) if err != nil { return fmt.Errorf("unable to subscribe to identity server events: %w", err) } @@ -112,7 +116,11 @@ var serveCmd = &cobra.Command{ serverMetrics := controlplane.NewMetrics() providerMetrics := provtelemetry.NewProviderMetrics() - s, err := controlplane.NewServer(store, evt, serverMetrics, cfg, vldtr, controlplane.WithProviderMetrics(providerMetrics)) + s, err := controlplane.NewServer( + store, evt, serverMetrics, cfg, vldtr, + controlplane.WithProviderMetrics(providerMetrics), + controlplane.WithAuthzClient(authzc), + ) if err != nil { return fmt.Errorf("unable to create server: %w", err) } diff --git a/database/mock/store.go b/database/mock/store.go index 55ee80ca6c..4176ee8c9d 100644 --- a/database/mock/store.go +++ b/database/mock/store.go @@ -276,6 +276,21 @@ func (mr *MockStoreMockRecorder) CreateProject(arg0, arg1 interface{}) *gomock.C return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateProject", reflect.TypeOf((*MockStore)(nil).CreateProject), arg0, arg1) } +// CreateProjectWithID mocks base method. +func (m *MockStore) CreateProjectWithID(arg0 context.Context, arg1 db.CreateProjectWithIDParams) (db.Project, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "CreateProjectWithID", arg0, arg1) + ret0, _ := ret[0].(db.Project) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// CreateProjectWithID indicates an expected call of CreateProjectWithID. +func (mr *MockStoreMockRecorder) CreateProjectWithID(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CreateProjectWithID", reflect.TypeOf((*MockStore)(nil).CreateProjectWithID), arg0, arg1) +} + // CreateProvider mocks base method. func (m *MockStore) CreateProvider(arg0 context.Context, arg1 db.CreateProviderParams) (db.Provider, error) { m.ctrl.T.Helper() diff --git a/database/query/projects.sql b/database/query/projects.sql index 5c8254abb5..5334c9e078 100644 --- a/database/query/projects.sql +++ b/database/query/projects.sql @@ -7,6 +7,16 @@ INSERT INTO projects ( $1, $2, sqlc.arg(metadata)::jsonb ) RETURNING *; +-- name: CreateProjectWithID :one +INSERT INTO projects ( + id, + name, + parent_id, + metadata +) VALUES ( + $1, $2, $3, sqlc.arg(metadata)::jsonb +) RETURNING *; + -- name: GetRootProjects :many SELECT * FROM projects WHERE parent_id IS NULL; diff --git a/internal/controlplane/default_project.go b/internal/controlplane/default_project.go index e1fea057b3..b51acdf2fd 100644 --- a/internal/controlplane/default_project.go +++ b/internal/controlplane/default_project.go @@ -21,10 +21,12 @@ import ( "fmt" "github.com/google/uuid" + "github.com/rs/zerolog/log" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/timestamppb" + "github.com/stacklok/minder/internal/authz" "github.com/stacklok/minder/internal/db" github "github.com/stacklok/minder/internal/providers/github" pb "github.com/stacklok/minder/pkg/api/protobuf/go/minder/v1" @@ -41,8 +43,8 @@ type ProjectMeta struct { } // CreateDefaultRecordsForOrg creates the default records, such as projects, roles and provider for the organization -func CreateDefaultRecordsForOrg(ctx context.Context, qtx db.Querier, - org db.Project, projectName string) (*pb.Project, []int32, error) { +func (s *Server) CreateDefaultRecordsForOrg(ctx context.Context, qtx db.Querier, + org db.Project, projectName string, userSub string) (outproj *pb.Project, outroles []int32, projerr error) { projectmeta := &ProjectMeta{ Description: fmt.Sprintf("Default admin project for %s", org.Name), } @@ -52,12 +54,30 @@ func CreateDefaultRecordsForOrg(ctx context.Context, qtx db.Querier, return nil, nil, status.Errorf(codes.Internal, "failed to marshal meta: %v", err) } + projectID := uuid.New() + + // Create authorization tuple + // NOTE: This is only creating a tuple for the project, not the organization + // We currently have no use for the organization and it might be + // removed in the future. + if err := s.authzClient.Write(ctx, userSub, authz.AuthzRoleAdmin, projectID); err != nil { + return nil, nil, status.Errorf(codes.Internal, "failed to create authorization tuple: %v", err) + } + defer func() { + if outproj == nil && projerr != nil { + if err := s.authzClient.Delete(ctx, userSub, authz.AuthzRoleAdmin, projectID); err != nil { + log.Ctx(ctx).Error().Err(err).Msg("failed to delete authorization tuple") + } + } + }() + // we need to create the default records for the organization - project, err := qtx.CreateProject(ctx, db.CreateProjectParams{ + project, err := qtx.CreateProjectWithID(ctx, db.CreateProjectWithIDParams{ ParentID: uuid.NullUUID{ UUID: org.ID, Valid: true, }, + ID: projectID, Name: projectName, Metadata: jsonmeta, }) @@ -93,6 +113,9 @@ func CreateDefaultRecordsForOrg(ctx context.Context, qtx db.Querier, }) if err != nil { + if err := s.authzClient.Delete(ctx, userSub, authz.AuthzRoleAdmin, projectID); err != nil { + log.Ctx(ctx).Error().Err(err).Msg("failed to delete authorization tuple") + } return nil, nil, status.Errorf(codes.Internal, "failed to create default project role: %v", err) } diff --git a/internal/controlplane/handlers_user.go b/internal/controlplane/handlers_user.go index 041c35a7a3..b43355db3b 100644 --- a/internal/controlplane/handlers_user.go +++ b/internal/controlplane/handlers_user.go @@ -88,7 +88,7 @@ func (s *Server) CreateUser(ctx context.Context, if err != nil { return nil, status.Errorf(codes.Internal, "failed to create organization: %s", err) } - orgProject, userRoles, err := CreateDefaultRecordsForOrg(ctx, qtx, organization, baseName) + orgProject, userRoles, err := s.CreateDefaultRecordsForOrg(ctx, qtx, organization, baseName, subject) if err != nil { return nil, status.Errorf(codes.Internal, "failed to create default organization records: %s", err) } @@ -187,7 +187,7 @@ func (s *Server) DeleteUser(ctx context.Context, return nil, status.Errorf(codes.Internal, "unexpected status code when deleting account: %d", resp.StatusCode) } - err = DeleteUser(ctx, s.store, subject) + err = DeleteUser(ctx, s.store, s.authzClient, subject) if err != nil { return nil, status.Errorf(codes.Internal, "failed to delete user from database: %v", err) } diff --git a/internal/controlplane/handlers_user_test.go b/internal/controlplane/handlers_user_test.go index 38fb634ae3..5232ad08c8 100644 --- a/internal/controlplane/handlers_user_test.go +++ b/internal/controlplane/handlers_user_test.go @@ -35,6 +35,7 @@ import ( mockdb "github.com/stacklok/minder/database/mock" "github.com/stacklok/minder/internal/auth" mockjwt "github.com/stacklok/minder/internal/auth/mock" + "github.com/stacklok/minder/internal/authz/mock" serverconfig "github.com/stacklok/minder/internal/config/server" "github.com/stacklok/minder/internal/crypto" "github.com/stacklok/minder/internal/db" @@ -78,7 +79,7 @@ func TestCreateUserDBMock(t *testing.T) { CreateOrganization(gomock.Any(), gomock.Any()). Return(db.Project{ID: orgID}, nil) store.EXPECT(). - CreateProject(gomock.Any(), gomock.Any()). + CreateProjectWithID(gomock.Any(), gomock.Any()). Return(db.Project{ ID: projectID, ParentID: uuid.NullUUID{ @@ -151,6 +152,7 @@ func TestCreateUserDBMock(t *testing.T) { cfg: &serverconfig.Config{}, cryptoEngine: crypeng, vldtr: mockJwtValidator, + authzClient: &mock.NoopClient{Authorized: true}, } resp, err := server.CreateUser(ctx, tc.req) @@ -183,7 +185,7 @@ func TestCreateUser_gRPC(t *testing.T) { CreateOrganization(gomock.Any(), gomock.Any()). Return(db.Project{ID: orgID}, nil) store.EXPECT(). - CreateProject(gomock.Any(), gomock.Any()). + CreateProjectWithID(gomock.Any(), gomock.Any()). Return(db.Project{ ID: projectID, ParentID: uuid.NullUUID{ @@ -339,6 +341,8 @@ func TestDeleteUserDBMock(t *testing.T) { mockStore.EXPECT(). DeleteOrganization(gomock.Any(), orgID). Return(nil) + mockStore.EXPECT().GetUserProjects(gomock.Any(), gomock.Any()). + Return([]db.GetUserProjectsRow{}, nil) mockStore.EXPECT().Commit(gomock.Any()) mockStore.EXPECT().Rollback(gomock.Any()) @@ -357,6 +361,7 @@ func TestDeleteUserDBMock(t *testing.T) { }, vldtr: mockJwtValidator, cryptoEngine: crypeng, + authzClient: &mock.NoopClient{Authorized: true}, } response, err := server.DeleteUser(ctx, request) @@ -393,6 +398,8 @@ func TestDeleteUser_gRPC(t *testing.T) { Return(db.User{ OrganizationID: orgID, }, nil) + store.EXPECT().GetUserProjects(gomock.Any(), gomock.Any()). + Return([]db.GetUserProjectsRow{}, nil) store.EXPECT(). DeleteOrganization(gomock.Any(), orgID). Return(nil) diff --git a/internal/controlplane/identity_events.go b/internal/controlplane/identity_events.go index 36bf0986a9..8a59bb4773 100644 --- a/internal/controlplane/identity_events.go +++ b/internal/controlplane/identity_events.go @@ -30,6 +30,7 @@ import ( "golang.org/x/oauth2" "golang.org/x/oauth2/clientcredentials" + "github.com/stacklok/minder/internal/authz" serverconfig "github.com/stacklok/minder/internal/config/server" "github.com/stacklok/minder/internal/db" ) @@ -49,10 +50,15 @@ type AccountEvent struct { } // SubscribeToIdentityEvents starts a cron job that periodically fetches events from the identity provider -func SubscribeToIdentityEvents(ctx context.Context, store db.Store, cfg *serverconfig.Config) error { +func SubscribeToIdentityEvents( + ctx context.Context, + store db.Store, + authzClient authz.Client, + cfg *serverconfig.Config, +) error { c := cron.New() _, err := c.AddFunc(eventFetchInterval, func() { - HandleEvents(ctx, store, cfg) + HandleEvents(ctx, store, authzClient, cfg) }) if err != nil { return err @@ -62,7 +68,12 @@ func SubscribeToIdentityEvents(ctx context.Context, store db.Store, cfg *serverc } // HandleEvents fetches events from the identity provider and performs any related changes to the minder database -func HandleEvents(ctx context.Context, store db.Store, cfg *serverconfig.Config) { +func HandleEvents( + ctx context.Context, + store db.Store, + authzClient authz.Client, + cfg *serverconfig.Config, +) { d := time.Now().Add(time.Duration(10) * time.Minute) ctx, cancel := context.WithDeadline(ctx, d) defer cancel() @@ -122,14 +133,14 @@ func HandleEvents(ctx context.Context, store db.Store, cfg *serverconfig.Config) } for _, event := range events { if event.Type == deleteAccountEventType { - err := DeleteUser(ctx, store, event.UserId) + err := DeleteUser(ctx, store, authzClient, event.UserId) zerolog.Ctx(ctx).Error().Msgf("events chron: error deleting user account: %v", err) } } } // DeleteUser deletes a user and all their associated data from the minder database -func DeleteUser(ctx context.Context, store db.Store, userId string) error { +func DeleteUser(ctx context.Context, store db.Store, authzClient authz.Client, userId string) error { tx, err := store.BeginTransaction() if err != nil { return err @@ -145,6 +156,18 @@ func DeleteUser(ctx context.Context, store db.Store, userId string) error { } return fmt.Errorf("error retrieving user %v", err) } + + projs, err := store.GetUserProjects(ctx, user.ID) + if err != nil { + return fmt.Errorf("error retrieving user projects %v", err) + } + + for _, proj := range projs { + if err := authzClient.Delete(ctx, userId, authz.AuthzRoleAdmin, proj.ID); err != nil { + return fmt.Errorf("error deleting authorization tuple %v", err) + } + } + err = qtx.DeleteOrganization(ctx, user.OrganizationID) if err != nil { return fmt.Errorf("error deleting organization %w", err) diff --git a/internal/controlplane/identity_events_test.go b/internal/controlplane/identity_events_test.go index 8002876c34..9b521d7de5 100644 --- a/internal/controlplane/identity_events_test.go +++ b/internal/controlplane/identity_events_test.go @@ -27,6 +27,7 @@ import ( "golang.org/x/oauth2" mockdb "github.com/stacklok/minder/database/mock" + "github.com/stacklok/minder/internal/authz/mock" serverconfig "github.com/stacklok/minder/internal/config/server" "github.com/stacklok/minder/internal/db" ) @@ -72,6 +73,8 @@ func TestHandleEvents(t *testing.T) { mockStore.EXPECT(). GetUserBySubject(gomock.Any(), "alreadyDeletedUserId"). Return(db.User{}, sql.ErrNoRows) + mockStore.EXPECT().GetUserProjects(gomock.Any(), gomock.Any()). + Return([]db.GetUserProjectsRow{}, nil) mockStore.EXPECT().Rollback(gomock.Any()) c := serverconfig.Config{ @@ -83,7 +86,7 @@ func TestHandleEvents(t *testing.T) { }, }, } - HandleEvents(context.Background(), mockStore, &c) + HandleEvents(context.Background(), mockStore, &mock.NoopClient{Authorized: true}, &c) } func tokenHandler(t *testing.T, w http.ResponseWriter) { diff --git a/internal/controlplane/server.go b/internal/controlplane/server.go index 3ff5b6275e..0371308ac7 100644 --- a/internal/controlplane/server.go +++ b/internal/controlplane/server.go @@ -124,16 +124,14 @@ func NewServer( vldtr: vldtr, mt: cpm, provMt: provtelemetry.NewNoopMetrics(), + // TODO: this currently always returns authorized as a transitionary measure. + // When OpenFGA is fully rolled out, we may want to make this a hard error or set to false. + authzClient: &mock.NoopClient{Authorized: true}, } for _, opt := range opts { opt(s) } - if s.authzClient == nil { - // TODO: this currently always returns authorized as a transitionary measure. - // When OpenFGA is fully rolled out, we may want to make this a hard error or set to false. - s.authzClient = &mock.NoopClient{Authorized: true} - } return s, nil } diff --git a/internal/db/projects.sql.go b/internal/db/projects.sql.go index 2f741dd756..b0e9dc2916 100644 --- a/internal/db/projects.sql.go +++ b/internal/db/projects.sql.go @@ -44,6 +44,44 @@ func (q *Queries) CreateProject(ctx context.Context, arg CreateProjectParams) (P return i, err } +const createProjectWithID = `-- name: CreateProjectWithID :one +INSERT INTO projects ( + id, + name, + parent_id, + metadata +) VALUES ( + $1, $2, $3, $4::jsonb +) RETURNING id, name, is_organization, metadata, parent_id, created_at, updated_at +` + +type CreateProjectWithIDParams struct { + ID uuid.UUID `json:"id"` + Name string `json:"name"` + ParentID uuid.NullUUID `json:"parent_id"` + Metadata json.RawMessage `json:"metadata"` +} + +func (q *Queries) CreateProjectWithID(ctx context.Context, arg CreateProjectWithIDParams) (Project, error) { + row := q.db.QueryRowContext(ctx, createProjectWithID, + arg.ID, + arg.Name, + arg.ParentID, + arg.Metadata, + ) + var i Project + err := row.Scan( + &i.ID, + &i.Name, + &i.IsOrganization, + &i.Metadata, + &i.ParentID, + &i.CreatedAt, + &i.UpdatedAt, + ) + return i, err +} + const deleteProject = `-- name: DeleteProject :many WITH RECURSIVE get_children AS ( SELECT id, parent_id FROM projects diff --git a/internal/db/querier.go b/internal/db/querier.go index 6a4d11187a..f91a3699bb 100644 --- a/internal/db/querier.go +++ b/internal/db/querier.go @@ -25,6 +25,7 @@ type Querier interface { CreateProfile(ctx context.Context, arg CreateProfileParams) (Profile, error) CreateProfileForEntity(ctx context.Context, arg CreateProfileForEntityParams) (EntityProfile, error) CreateProject(ctx context.Context, arg CreateProjectParams) (Project, error) + CreateProjectWithID(ctx context.Context, arg CreateProjectWithIDParams) (Project, error) CreateProvider(ctx context.Context, arg CreateProviderParams) (Provider, error) CreatePullRequest(ctx context.Context, arg CreatePullRequestParams) (PullRequest, error) CreateRepository(ctx context.Context, arg CreateRepositoryParams) (Repository, error)