diff --git a/enterprise/server/backends/authdb/authdb.go b/enterprise/server/backends/authdb/authdb.go index f2c1ffe68b0..1736b2308b6 100644 --- a/enterprise/server/backends/authdb/authdb.go +++ b/enterprise/server/backends/authdb/authdb.go @@ -5,6 +5,7 @@ import ( "encoding/base64" "encoding/hex" "fmt" + "slices" "strings" "sync" "time" @@ -696,12 +697,15 @@ func (d *AuthDB) authorizeNewAPIKeyCapabilities(ctx context.Context, userID, gro return d.authorizeGroupAdminRole(ctx, groupID) } -func (d *AuthDB) CreateUserAPIKey(ctx context.Context, groupID, label string, caps []akpb.ApiKey_Capability) (*tables.APIKey, error) { +func (d *AuthDB) CreateUserAPIKey(ctx context.Context, groupID, userID, label string, caps []akpb.ApiKey_Capability) (*tables.APIKey, error) { if !*userOwnedKeysEnabled { return nil, status.UnimplementedError("not implemented") } if groupID == "" { - return nil, status.InvalidArgumentError("Group ID cannot be nil.") + return nil, status.InvalidArgumentError("missing group ID") + } + if userID == "" { + return nil, status.InvalidArgumentError("missing user ID") } if err := authutil.AuthorizeGroupAccess(ctx, d.env, groupID); err != nil { @@ -713,7 +717,30 @@ func (d *AuthDB) CreateUserAPIKey(ctx context.Context, groupID, label string, ca return nil, err } - if err := d.authorizeNewAPIKeyCapabilities(ctx, u.GetUserID(), groupID, caps); err != nil { + if userID != u.GetUserID() { + // ORG_ADMIN is required to create keys for a user other than the + // authenticated user. + caps, err := capabilities.ForAuthenticatedUserGroup(ctx, d.env, groupID) + if err != nil { + return nil, status.WrapError(err, "get capabilities") + } + if !slices.Contains(caps, akpb.ApiKey_ORG_ADMIN_CAPABILITY) { + return nil, status.PermissionDeniedError("org admin permission is required to create an API key for the requested user") + } + + // When creating a key for the non-authenticated user, validate that + // they are a member of the requested group. + ok, err := d.isGroupMember(ctx, groupID, userID) + if err != nil { + log.CtxWarningf(ctx, "Failed to query group memberships: %s", err) + return nil, status.InternalError("failed to query group memberships") + } + if !ok { + return nil, status.PermissionDeniedError("user is not a member of the requested group") + } + } + + if err := d.authorizeNewAPIKeyCapabilities(ctx, userID, groupID, caps); err != nil { return nil, err } @@ -731,7 +758,7 @@ func (d *AuthDB) CreateUserAPIKey(ctx context.Context, groupID, label string, ca } ak := tables.APIKey{ - UserID: u.GetUserID(), + UserID: userID, GroupID: u.GetGroupID(), Label: label, Capabilities: capabilities.ToInt(caps), @@ -739,6 +766,24 @@ func (d *AuthDB) CreateUserAPIKey(ctx context.Context, groupID, label string, ca return d.createAPIKey(ctx, d.h, ak) } +func (d *AuthDB) isGroupMember(ctx context.Context, groupID, userID string) (bool, error) { + q := d.env.GetDBHandle().NewQuery(ctx, "authdb_check_group_membership").Raw(` + SELECT * + FROM "UserGroups" + WHERE group_group_id = ? + AND user_user_id = ? + AND membership_status = ? + `, groupID, userID, grpb.GroupMembershipStatus_MEMBER) + ug := &tables.UserGroup{} + if err := q.Take(ug); err != nil { + if db.IsRecordNotFound(err) { + return false, nil + } + return false, err + } + return true, nil +} + func (d *AuthDB) getAPIKey(ctx context.Context, h interfaces.DB, apiKeyID string) (*tables.APIKey, error) { if apiKeyID == "" { return nil, status.InvalidArgumentError("API key ID cannot be empty.") diff --git a/enterprise/server/backends/userdb/BUILD b/enterprise/server/backends/userdb/BUILD index 319770d17e0..4776548f62e 100644 --- a/enterprise/server/backends/userdb/BUILD +++ b/enterprise/server/backends/userdb/BUILD @@ -51,6 +51,8 @@ go_test( "//server/util/testing/flags", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", + "@org_golang_google_grpc//codes", + "@org_golang_google_grpc//status", ], ) @@ -74,7 +76,6 @@ go_test( tags = ["docker"], deps = [ ":userdb", - "//enterprise/server/auditlog", "//enterprise/server/testutil/enterprise_testauth", "//enterprise/server/testutil/enterprise_testenv", "//proto:api_key_go_proto", @@ -90,12 +91,13 @@ go_test( "//server/testutil/testenv", "//server/util/capabilities", "//server/util/claims", - "//server/util/perms", "//server/util/role", "//server/util/status", "//server/util/testing/flags", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", + "@org_golang_google_grpc//codes", + "@org_golang_google_grpc//status", ], ) @@ -119,7 +121,6 @@ go_test( tags = ["docker"], deps = [ ":userdb", - "//enterprise/server/auditlog", "//enterprise/server/testutil/enterprise_testauth", "//enterprise/server/testutil/enterprise_testenv", "//proto:api_key_go_proto", @@ -135,11 +136,12 @@ go_test( "//server/testutil/testenv", "//server/util/capabilities", "//server/util/claims", - "//server/util/perms", "//server/util/role", "//server/util/status", "//server/util/testing/flags", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", + "@org_golang_google_grpc//codes", + "@org_golang_google_grpc//status", ], ) diff --git a/enterprise/server/backends/userdb/userdb_test.go b/enterprise/server/backends/userdb/userdb_test.go index 56539404b65..a6e04b3b442 100644 --- a/enterprise/server/backends/userdb/userdb_test.go +++ b/enterprise/server/backends/userdb/userdb_test.go @@ -22,12 +22,14 @@ import ( "github.com/buildbuddy-io/buildbuddy/server/util/testing/flags" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "google.golang.org/grpc/codes" akpb "github.com/buildbuddy-io/buildbuddy/proto/api_key" alpb "github.com/buildbuddy-io/buildbuddy/proto/auditlog" ctxpb "github.com/buildbuddy-io/buildbuddy/proto/context" grpb "github.com/buildbuddy-io/buildbuddy/proto/group" uidpb "github.com/buildbuddy-io/buildbuddy/proto/user_id" + gstatus "google.golang.org/grpc/status" ) func newTestEnv(t *testing.T) *testenv.TestEnv { @@ -901,7 +903,7 @@ func TestDeleteAPIKey(t *testing.T) { setUserOwnedKeysEnabled(t, ctx1, env, gr1.Group.GroupID, true) uk3, err := adb.CreateUserAPIKey( - ctx3, gr1.Group.GroupID, "US3's Key", + ctx3, gr1.Group.GroupID, "US3", "US3's Key", []akpb.ApiKey_Capability{akpb.ApiKey_CAS_WRITE_CAPABILITY}) require.NoError(t, err, "create a US3-owned key in org1") @@ -960,7 +962,7 @@ func TestUserOwnedKeys_GetUpdateDeletePermissions(t *testing.T) { // Create a key owned by test.Owner ownerGroup := getGroup(t, ownerCtx, env).Group ownerKey, err := adb.CreateUserAPIKey( - ownerCtx, ownerGroup.GroupID, test.Owner+"'s key", + ownerCtx, ownerGroup.GroupID, test.Owner, test.Owner+"'s key", []akpb.ApiKey_Capability{akpb.ApiKey_CAS_WRITE_CAPABILITY}, ) require.NoError(t, err) @@ -1043,7 +1045,7 @@ func TestUserOwnedKeys_RespectsEnabledSetting(t *testing.T) { // Try to create a user-owned key; should fail by default. _, err := adb.CreateUserAPIKey( - ctx1, gr1.GroupID, "US1's key", + ctx1, gr1.GroupID, "US1", "US1's key", []akpb.ApiKey_Capability{akpb.ApiKey_CAS_WRITE_CAPABILITY}) require.Truef( t, status.IsPermissionDeniedError(err), @@ -1054,7 +1056,7 @@ func TestUserOwnedKeys_RespectsEnabledSetting(t *testing.T) { setUserOwnedKeysEnabled(t, ctx1, env, gr1.GroupID, true) key1, err := adb.CreateUserAPIKey( - ctx1, gr1.GroupID, "US1's key", + ctx1, gr1.GroupID, "US1", "US1's key", []akpb.ApiKey_Capability{akpb.ApiKey_CAS_WRITE_CAPABILITY}) require.NoError( t, err, @@ -1122,7 +1124,7 @@ func TestUserOwnedKeys_RemoveUserFromGroup_KeyNoLongerWorks(t *testing.T) { require.NoError(t, err) us2Key, err := adb.CreateUserAPIKey( - ctx2, gr1.GroupID, "US2's key", + ctx2, gr1.GroupID, "US2", "US2's key", []akpb.ApiKey_Capability{akpb.ApiKey_CAS_WRITE_CAPABILITY}) require.NoError(t, err, "US2 should be able to create a user-owned key") @@ -1161,7 +1163,7 @@ func TestUserOwnedKeys_ChangeRole_UpdatesCapabilities(t *testing.T) { }) require.NoError(t, err) us1Key, err := adb.CreateUserAPIKey( - ctx1, gr1.GroupID, "", + ctx1, gr1.GroupID, "US1", "", []akpb.ApiKey_Capability{akpb.ApiKey_CACHE_WRITE_CAPABILITY}) require.NoError(t, err, "US1 should be able to create a user-owned key") @@ -1231,7 +1233,7 @@ func TestUserOwnedKeys_CreateAndUpdateCapabilities(t *testing.T) { // Test create with capabilities key, err := adb.CreateUserAPIKey( - ctx1, g.GroupID, "US1's key", test.Capabilities) + ctx1, g.GroupID, "US1", "US1's key", test.Capabilities) if test.OK { require.NoError(t, err) // Read back the capabilities, make sure they took effect. @@ -1248,7 +1250,7 @@ func TestUserOwnedKeys_CreateAndUpdateCapabilities(t *testing.T) { // Test update existing key capabilities key, err = adb.CreateUserAPIKey( - ctx1, g.GroupID, "US1's key", + ctx1, g.GroupID, "US1", "US1's key", []akpb.ApiKey_Capability{}) require.NoError(t, err) key.Capabilities = capabilities.ToInt(test.Capabilities) @@ -1288,7 +1290,7 @@ func TestUserOwnedKeys_NotReturnedByGroupLevelAPIs(t *testing.T) { } // Create a user-level key. - _, err = adb.CreateUserAPIKey(ctx1, g.GroupID, "test-personal-key", nil /*=capabilities*/) + _, err = adb.CreateUserAPIKey(ctx1, g.GroupID, "US1", "test-personal-key", nil /*=capabilities*/) require.NoError(t, err) // Test all group-level APIs; none should return the user-level key we @@ -1302,6 +1304,112 @@ func TestUserOwnedKeys_NotReturnedByGroupLevelAPIs(t *testing.T) { require.Empty(t, keys) } +func TestUserOwnedKeys_CreateForOtherUser(t *testing.T) { + flags.Set(t, "app.add_user_to_domain_group", true) + + for _, test := range []struct { + Name string + AuthUserID string // set if authenticating as user + AuthKeyCaps []akpb.ApiKey_Capability // set if authenticating as API key + AuthGroupID string + KeyGroupID string + KeyUserID string + Code codes.Code + }{ + { + Name: "AuthAsOrgAdminAPIKey_CreateForUserInAuthenticatedGroup_OK", + AuthGroupID: "GR1", + AuthKeyCaps: []akpb.ApiKey_Capability{akpb.ApiKey_ORG_ADMIN_CAPABILITY}, + KeyGroupID: "GR1", + KeyUserID: "US1", + Code: codes.OK, + }, + { + Name: "AuthAsRegularAPIKey_CreateForUserInAuthenticatedGroup_PermissionDenied", + AuthGroupID: "GR1", + AuthKeyCaps: []akpb.ApiKey_Capability{akpb.ApiKey_CAS_WRITE_CAPABILITY}, + KeyGroupID: "GR1", + KeyUserID: "US1", + Code: codes.PermissionDenied, + }, + { + Name: "AuthAsOrgAdminAPIKey_CreateForUserInOtherGroup_PermissionDenied", + AuthGroupID: "GR1", + AuthKeyCaps: []akpb.ApiKey_Capability{akpb.ApiKey_ORG_ADMIN_CAPABILITY}, + KeyGroupID: "GR1", + KeyUserID: "US2", + Code: codes.PermissionDenied, + }, + { + Name: "AuthAsOrgAdminUser_CreateForUserInAuthenticatedGroup_OK", + AuthUserID: "US2", + AuthGroupID: "GR2", + KeyGroupID: "GR2", + KeyUserID: "US3", + Code: codes.OK, + }, + { + Name: "AuthAsOrgDeveloper_CreateForUserInAuthenticatedGroup_PermissionDenied", + AuthUserID: "US3", + AuthGroupID: "GR2", + KeyGroupID: "GR2", + KeyUserID: "US2", + Code: codes.PermissionDenied, + }, + } { + t.Run(test.Name, func(t *testing.T) { + // Setup: + // - GR1: + // - Has user US1 (Admin) + // - Has org API key with test.AuthCaps + // - GR2: + // - Has user US2 (Admin) + // - Has user US3 (Developer) + // - Has org API key with test.AuthCaps + + ctx := context.Background() + env := newTestEnv(t) + adb := env.GetAuthDB() + keys := map[string]*tables.APIKey{} + // GR1 setup + { + createUser(t, ctx, env, "US1", "org1.io") + ctx1 := authUserCtx(ctx, env, t, "US1") + g := getGroup(t, ctx1, env).Group + setUserOwnedKeysEnabled(t, ctx1, env, g.GroupID, true) + k1, err := adb.CreateAPIKey(ctx1, "GR1", "", test.AuthKeyCaps, false) + require.NoError(t, err) + keys["GR1"] = k1 + } + // GR2 setup + { + createUser(t, ctx, env, "US2", "org2.io") + ctx2 := authUserCtx(ctx, env, t, "US2") + g := getGroup(t, ctx2, env).Group + setUserOwnedKeysEnabled(t, ctx2, env, g.GroupID, true) + k2, err := adb.CreateAPIKey(ctx2, "GR2", "", test.AuthKeyCaps, false) + require.NoError(t, err) + keys["GR2"] = k2 + takeOwnershipOfDomain(t, ctx2, env, "US2") + createUser(t, ctx, env, "US3", "org2.io") + } + + // Try creating a personal key + var authCtx context.Context + if test.AuthUserID == "" { + authCtx = env.GetAuthenticator().AuthContextFromAPIKey(ctx, keys[test.AuthGroupID].Value) + } else { + authCtx = authUserCtx(ctx, env, t, test.AuthUserID) + } + k, err := adb.CreateUserAPIKey(authCtx, test.KeyGroupID, test.KeyUserID, "" /*=label*/, nil /*=caps*/) + assert.Equal(t, test.Code.String(), gstatus.Code(err).String(), "%s", err) + if err == nil { + assert.Equal(t, test.KeyUserID, k.UserID) + } + }) + } +} + func TestRequestToJoinGroup_DomainNonMember_CreatesRequest(t *testing.T) { ctx := context.Background() env := newTestEnv(t) diff --git a/proto/api_key.proto b/proto/api_key.proto index 8a6195ab5e6..6a0b9b1ed84 100644 --- a/proto/api_key.proto +++ b/proto/api_key.proto @@ -52,6 +52,11 @@ message CreateApiKeyRequest { reserved 2; // group_id + // The ID of the user to create the API key for. + // Requires ORG_ADMIN capability if different than the authenticated + // user. + string user_id = 6; + // Optional. The user-specified label of this API key that helps them // remember what it's for. string label = 3; diff --git a/server/buildbuddy_server/buildbuddy_server.go b/server/buildbuddy_server/buildbuddy_server.go index f0d8fd691b0..f722d8bb2f6 100644 --- a/server/buildbuddy_server/buildbuddy_server.go +++ b/server/buildbuddy_server/buildbuddy_server.go @@ -812,7 +812,18 @@ func (s *BuildBuddyServer) CreateUserApiKey(ctx context.Context, req *akpb.Creat if authDB == nil || !authDB.GetUserOwnedKeysEnabled() { return nil, status.UnimplementedError("Not Implemented") } - k, err := authDB.CreateUserAPIKey(ctx, req.GetRequestContext().GetGroupId(), req.GetLabel(), req.GetCapability()) + + u, err := s.env.GetAuthenticator().AuthenticatedUser(ctx) + if err != nil { + return nil, err + } + + userID := req.GetUserId() + // TODO: make user_id a required parameter and remove this fallback. + if userID == "" { + userID = u.GetUserID() + } + k, err := authDB.CreateUserAPIKey(ctx, req.GetRequestContext().GetGroupId(), userID, req.GetLabel(), req.GetCapability()) if err != nil { return nil, err } diff --git a/server/interfaces/interfaces.go b/server/interfaces/interfaces.go index e15926d1c0c..dc4071b1945 100644 --- a/server/interfaces/interfaces.go +++ b/server/interfaces/interfaces.go @@ -478,8 +478,11 @@ type AuthDB interface { // GetUserAPIKeys returns all user-owned API keys within a group. GetUserAPIKeys(ctx context.Context, groupID string) ([]*tables.APIKey, error) - // CreateUserAPIKey creates a user-owned API key within the group. - CreateUserAPIKey(ctx context.Context, groupID, label string, capabilities []akpb.ApiKey_Capability) (*tables.APIKey, error) + // CreateUserAPIKey creates a user-owned API key within the group. The given + // user must be a member of the group. If the request is not authenticated + // as the given user, then the authenticated user or API key must have + // ORG_ADMIN capability. + CreateUserAPIKey(ctx context.Context, groupID, userID, label string, capabilities []akpb.ApiKey_Capability) (*tables.APIKey, error) // GetAPIKey returns an API key by ID. The key may be user-owned or // group-owned.