From 5f8711684af6fc49525ff5685cdf3c1883372d28 Mon Sep 17 00:00:00 2001 From: Alex Ostrikov Date: Thu, 1 Jun 2023 16:58:17 +0400 Subject: [PATCH] own: add API for removing an assigned owner. (#52691) Test plan: GraphQL tests added. --- cmd/frontend/graphqlbackend/own.go | 1 + cmd/frontend/graphqlbackend/own.graphql | 6 +- .../internal/own/resolvers/resolvers.go | 36 +++- .../internal/own/resolvers/resolvers_test.go | 155 +++++++++++++++++- 4 files changed, 185 insertions(+), 13 deletions(-) diff --git a/cmd/frontend/graphqlbackend/own.go b/cmd/frontend/graphqlbackend/own.go index 40839ecc937..4b232f500b1 100644 --- a/cmd/frontend/graphqlbackend/own.go +++ b/cmd/frontend/graphqlbackend/own.go @@ -61,6 +61,7 @@ type OwnResolver interface { // Assigned ownership mutations. AssignOwner(context.Context, *AssignOwnerArgs) (*EmptyResponse, error) + RemoveAssignedOwner(context.Context, *AssignOwnerArgs) (*EmptyResponse, error) // Config. OwnSignalConfigurations(ctx context.Context) ([]SignalConfigurationResolver, error) diff --git a/cmd/frontend/graphqlbackend/own.graphql b/cmd/frontend/graphqlbackend/own.graphql index 35a18432b70..de096238b96 100644 --- a/cmd/frontend/graphqlbackend/own.graphql +++ b/cmd/frontend/graphqlbackend/own.graphql @@ -249,6 +249,10 @@ extend type Mutation { assignOwner creates a new assigned owner. """ assignOwner(input: AssignOwnerInput!): EmptyResponse + """ + removeAssignedOwner removes an assigned owner. + """ + removeAssignedOwner(input: AssignOwnerInput!): EmptyResponse } """ @@ -347,7 +351,7 @@ input UpdateSignalConfigurationsInput { } """ -AssignOwnerInput represents the input for assigning an owner. +AssignOwnerInput represents the input for assigning or deleting an owner. """ input AssignOwnerInput { """ diff --git a/enterprise/cmd/frontend/internal/own/resolvers/resolvers.go b/enterprise/cmd/frontend/internal/own/resolvers/resolvers.go index 4e909c660b5..eb612537cc8 100644 --- a/enterprise/cmd/frontend/internal/own/resolvers/resolvers.go +++ b/enterprise/cmd/frontend/internal/own/resolvers/resolvers.go @@ -777,6 +777,25 @@ func (r *ownResolver) AssignOwner(ctx context.Context, args *graphqlbackend.Assi if actor.FromContext(ctx).IsInternal() { return nil, nil } + user, err := r.checkAssignedOwnershipPermission(ctx) + if err != nil { + return nil, err + } + u, err := unmarshalAssignOwnerArgs(args.Input) + if err != nil { + return nil, err + } + whoAssignedUserID := user.ID + err = r.db.AssignedOwners().Insert(ctx, u.AssignedOwnerID, u.RepoID, u.AbsolutePath, whoAssignedUserID) + if err != nil { + return nil, errors.Wrap(err, "creating assigned owner") + } + return &graphqlbackend.EmptyResponse{}, nil +} + +// checkAssignedOwnershipPermission checks that the user from the context has +// `rbac.OwnershipAssignPermission` and returns the user if so. +func (r *ownResolver) checkAssignedOwnershipPermission(ctx context.Context) (*types.User, error) { // Extracting the user to run an RBAC check and then use their ID as an // `whoAssignedUserID`. user, err := auth.CurrentUser(ctx, r.db) @@ -790,14 +809,25 @@ func (r *ownResolver) AssignOwner(ctx context.Context, args *graphqlbackend.Assi if err := rbac.CheckGivenUserHasPermission(ctx, r.db, user, rbac.OwnershipAssignPermission); err != nil { return nil, err } + return user, nil +} + +func (r *ownResolver) RemoveAssignedOwner(ctx context.Context, args *graphqlbackend.AssignOwnerArgs) (*graphqlbackend.EmptyResponse, error) { + // Internal actor is a no-op, only a user can remove an assigned owner. + if actor.FromContext(ctx).IsInternal() { + return nil, nil + } + _, err := r.checkAssignedOwnershipPermission(ctx) + if err != nil { + return nil, err + } u, err := unmarshalAssignOwnerArgs(args.Input) if err != nil { return nil, err } - whoAssignedUserID := user.ID - err = r.db.AssignedOwners().Insert(ctx, u.AssignedOwnerID, u.RepoID, u.AbsolutePath, whoAssignedUserID) + err = r.db.AssignedOwners().DeleteOwner(ctx, u.AssignedOwnerID, u.RepoID, u.AbsolutePath) if err != nil { - return nil, errors.Wrap(err, "creating assigned owner") + return nil, errors.Wrap(err, "deleting assigned owner") } return &graphqlbackend.EmptyResponse{}, nil } diff --git a/enterprise/cmd/frontend/internal/own/resolvers/resolvers_test.go b/enterprise/cmd/frontend/internal/own/resolvers/resolvers_test.go index efdc5783112..41b23977543 100644 --- a/enterprise/cmd/frontend/internal/own/resolvers/resolvers_test.go +++ b/enterprise/cmd/frontend/internal/own/resolvers/resolvers_test.go @@ -1411,10 +1411,10 @@ func TestAssignOwner(t *testing.T) { repo := types.Repo{Name: "test-repo-1", ID: 101} err := db.Repos().Create(ctx, &repo) require.NoError(t, err) - // Creating admin and non-admin users, only admin has rights to assign owners. - admin, err := db.Users().Create(context.Background(), database.NewUser{Username: "admin"}) + // Creating 2 users, only "hasPermission" user has rights to assign owners. + hasPermission, err := db.Users().Create(context.Background(), database.NewUser{Username: "has-permission"}) require.NoError(t, err) - user, err := db.Users().Create(context.Background(), database.NewUser{Username: "non-admin"}) + noPermission, err := db.Users().Create(context.Background(), database.NewUser{Username: "no-permission"}) require.NoError(t, err) // RBAC stuff below. permission, err := db.Permissions().Create(ctx, database.CreatePermissionOpts{ @@ -1430,7 +1430,7 @@ func TestAssignOwner(t *testing.T) { }) require.NoError(t, err) err = db.UserRoles().Assign(ctx, database.AssignUserRoleOpts{ - UserID: admin.ID, + UserID: hasPermission.ID, RoleID: role.ID, }) require.NoError(t, err) @@ -1440,8 +1440,8 @@ func TestAssignOwner(t *testing.T) { t.Fatal(err) } - adminCtx := actor.WithActor(ctx, actor.FromUser(admin.ID)) - userCtx := actor.WithActor(ctx, actor.FromUser(user.ID)) + adminCtx := actor.WithActor(ctx, actor.FromUser(hasPermission.ID)) + userCtx := actor.WithActor(ctx, actor.FromUser(noPermission.ID)) getBaseTest := func() *graphqlbackend.Test { return &graphqlbackend.Test{ @@ -1454,7 +1454,7 @@ func TestAssignOwner(t *testing.T) { } }`, Variables: map[string]any{"input": map[string]any{ - "assignedOwnerID": string(graphqlbackend.MarshalUserID(user.ID)), + "assignedOwnerID": string(graphqlbackend.MarshalUserID(noPermission.ID)), "repoID": string(graphqlbackend.MarshalRepositoryID(repo.ID)), "absolutePath": "", }}, @@ -1512,7 +1512,7 @@ func TestAssignOwner(t *testing.T) { "assignedOwnerID": string(graphqlbackend.MarshalUserID(0)), "repoID": string(graphqlbackend.MarshalRepositoryID(repo.ID)), "absolutePath": "", - "whoAssignedUserID": string(graphqlbackend.MarshalUserID(admin.ID)), + "whoAssignedUserID": string(graphqlbackend.MarshalUserID(hasPermission.ID)), }} graphqlbackend.RunTest(t, baseTest) assertNoAssignedOwners(t, repo.ID) @@ -1524,6 +1524,143 @@ func TestAssignOwner(t *testing.T) { baseTest.Context = adminCtx baseTest.ExpectedResult = `{"assignOwner":{"alwaysNil": null}}` graphqlbackend.RunTest(t, baseTest) - assertAssignedOwner(t, user.ID, admin.ID, repo.ID, "") + assertAssignedOwner(t, noPermission.ID, hasPermission.ID, repo.ID, "") + }) +} + +func TestDeleteAssignedOwner(t *testing.T) { + logger := logtest.Scoped(t) + testDB := dbtest.NewDB(logger, t) + db := database.NewDB(logger, testDB) + git := fakeGitserver{} + own := fakeOwnService{} + ctx := context.Background() + repo := types.Repo{Name: "test-repo-1", ID: 101} + err := db.Repos().Create(ctx, &repo) + require.NoError(t, err) + // Creating 2 users, only "hasPermission" user has rights to assign owners. + hasPermission, err := db.Users().Create(context.Background(), database.NewUser{Username: "has-permission"}) + require.NoError(t, err) + noPermission, err := db.Users().Create(context.Background(), database.NewUser{Username: "non-permission"}) + require.NoError(t, err) + // Creating an existing assigned owner. + require.NoError(t, db.AssignedOwners().Insert(ctx, noPermission.ID, repo.ID, "", hasPermission.ID)) + // RBAC stuff below. + permission, err := db.Permissions().Create(ctx, database.CreatePermissionOpts{ + Namespace: rbactypes.OwnershipNamespace, + Action: rbactypes.OwnershipAssignAction, + }) + require.NoError(t, err) + role, err := db.Roles().Create(ctx, "Can assign owners", false) + require.NoError(t, err) + err = db.RolePermissions().Assign(ctx, database.AssignRolePermissionOpts{ + RoleID: role.ID, + PermissionID: permission.ID, + }) + require.NoError(t, err) + err = db.UserRoles().Assign(ctx, database.AssignUserRoleOpts{ + UserID: hasPermission.ID, + RoleID: role.ID, + }) + require.NoError(t, err) + // RBAC stuff finished. Creating a GraphQL schema. + schema, err := graphqlbackend.NewSchema(db, git, nil, graphqlbackend.OptionalResolver{OwnResolver: resolvers.NewWithService(db, git, own, logger)}) + if err != nil { + t.Fatal(err) + } + + adminCtx := actor.WithActor(ctx, actor.FromUser(hasPermission.ID)) + userCtx := actor.WithActor(ctx, actor.FromUser(noPermission.ID)) + + getBaseTest := func() *graphqlbackend.Test { + return &graphqlbackend.Test{ + Context: userCtx, + Schema: schema, + Query: ` + mutation removeAssignedOwner($input:AssignOwnerInput!) { + removeAssignedOwner(input:$input) { + alwaysNil + } + }`, + Variables: map[string]any{"input": map[string]any{ + "assignedOwnerID": string(graphqlbackend.MarshalUserID(noPermission.ID)), + "repoID": string(graphqlbackend.MarshalRepositoryID(repo.ID)), + "absolutePath": "", + }}, + } + } + + assertOwnerExists := func(t *testing.T) { + t.Helper() + owners, err := db.AssignedOwners().ListAssignedOwnersForRepo(ctx, repo.ID) + require.NoError(t, err) + require.Len(t, owners, 1) + owner := owners[0] + assert.Equal(t, noPermission.ID, owner.OwnerUserID) + assert.Equal(t, hasPermission.ID, owner.WhoAssignedUserID) + assert.Equal(t, "", owner.FilePath) + } + + assertNoAssignedOwners := func(t *testing.T) { + t.Helper() + owners, err := db.AssignedOwners().ListAssignedOwnersForRepo(ctx, repo.ID) + require.NoError(t, err) + require.Empty(t, owners) + } + + t.Run("cannot delete assigned owner without permission", func(t *testing.T) { + baseTest := getBaseTest() + expectedErrs := []*gqlerrors.QueryError{{ + Message: "user is missing permission OWNERSHIP#ASSIGN", + Path: []any{"removeAssignedOwner"}, + }} + baseTest.ExpectedErrors = expectedErrs + baseTest.ExpectedResult = `{"removeAssignedOwner":null}` + graphqlbackend.RunTest(t, baseTest) + assertOwnerExists(t) + }) + + t.Run("bad request", func(t *testing.T) { + baseTest := getBaseTest() + baseTest.Context = adminCtx + expectedErrs := []*gqlerrors.QueryError{{ + Message: "assigned user ID should not be 0", + Path: []any{"removeAssignedOwner"}, + }} + baseTest.ExpectedErrors = expectedErrs + baseTest.ExpectedResult = `{"removeAssignedOwner":null}` + baseTest.Variables = map[string]any{"input": map[string]any{ + "assignedOwnerID": string(graphqlbackend.MarshalUserID(0)), + "repoID": string(graphqlbackend.MarshalRepositoryID(repo.ID)), + "absolutePath": "", + }} + graphqlbackend.RunTest(t, baseTest) + assertOwnerExists(t) + }) + + t.Run("assigned owner not found", func(t *testing.T) { + baseTest := getBaseTest() + baseTest.Context = adminCtx + expectedErrs := []*gqlerrors.QueryError{{ + Message: `deleting assigned owner: cannot delete assigned owner with ID=1337 for "" path for repo with ID=1`, + Path: []any{"removeAssignedOwner"}, + }} + baseTest.ExpectedErrors = expectedErrs + baseTest.ExpectedResult = `{"removeAssignedOwner":null}` + baseTest.Variables = map[string]any{"input": map[string]any{ + "assignedOwnerID": string(graphqlbackend.MarshalUserID(1337)), + "repoID": string(graphqlbackend.MarshalRepositoryID(repo.ID)), + "absolutePath": "", + }} + graphqlbackend.RunTest(t, baseTest) + assertOwnerExists(t) + }) + + t.Run("assigned owner successfully deleted", func(t *testing.T) { + baseTest := getBaseTest() + baseTest.Context = adminCtx + baseTest.ExpectedResult = `{"removeAssignedOwner":{"alwaysNil": null}}` + graphqlbackend.RunTest(t, baseTest) + assertNoAssignedOwners(t) }) }