From 74c8c161acaad9e3608a100d8710cb2ff374af68 Mon Sep 17 00:00:00 2001 From: Vadim Berezniker Date: Wed, 11 Oct 2023 14:15:58 -0700 Subject: [PATCH] Make impersonation play nice with subdomains. If the user is impersonating group X, but tries to access subdomain for group Y then don't enable impersonation on requests for subdomain Y so that subdomain Y can be accessed normally. This PR slightly changes the semantics of how the frontend communicates impersonation information to the backend. Prior to this change, the frontend would set an impersonation target and would assume that impersonation was in effect. Now, the frontend still sets the impersonation target but now the backend informs the frontend whether impersonation is actually in effect via the GetUser response. Fixes https://github.com/buildbuddy-io/buildbuddy-internal/issues/2779 --- app/auth/auth_service.ts | 5 +-- enterprise/server/backends/userdb/userdb.go | 3 ++ proto/buildbuddy_service.proto | 1 - proto/user.proto | 3 ++ server/buildbuddy_server/buildbuddy_server.go | 34 ++++++++----------- server/role_filter/role_filter.go | 1 - server/util/claims/BUILD | 1 + server/util/claims/claims.go | 15 +++++++- 8 files changed, 37 insertions(+), 26 deletions(-) diff --git a/app/auth/auth_service.ts b/app/auth/auth_service.ts index 8512e3b484a..1339039b1d4 100644 --- a/app/auth/auth_service.ts +++ b/app/auth/auth_service.ts @@ -133,9 +133,6 @@ export class AuthService { } private getUser(request: user.GetUserRequest) { - if (rpcService.requestContext.impersonatingGroupId) { - return rpcService.service.getImpersonatedUser(request); - } return rpcService.service.getUser(request); } @@ -178,7 +175,7 @@ export class AuthService { (name) => (name[0].toLowerCase() + name.substring(1)) as BuildBuddyServiceRpcName ) ), - isImpersonating: Boolean(rpcService.requestContext.impersonatingGroupId), + isImpersonating: response.isImpersonating, subdomainGroupID: response.subdomainGroupId, }); } diff --git a/enterprise/server/backends/userdb/userdb.go b/enterprise/server/backends/userdb/userdb.go index f126d91fbc2..36a515e968d 100644 --- a/enterprise/server/backends/userdb/userdb.go +++ b/enterprise/server/backends/userdb/userdb.go @@ -774,6 +774,9 @@ func (d *UserDB) GetUser(ctx context.Context) (*tables.User, error) { if err != nil { return nil, err } + if u.IsImpersonating() { + return d.GetImpersonatedUser(ctx) + } var user *tables.User err = d.h.Transaction(ctx, func(tx *db.DB) error { user, err = d.getUser(tx, u.GetUserID()) diff --git a/proto/buildbuddy_service.proto b/proto/buildbuddy_service.proto index 6a97394764a..ef450c9f1dd 100644 --- a/proto/buildbuddy_service.proto +++ b/proto/buildbuddy_service.proto @@ -64,7 +64,6 @@ service BuildBuddyService { // User API rpc CreateUser(user.CreateUserRequest) returns (user.CreateUserResponse); rpc GetUser(user.GetUserRequest) returns (user.GetUserResponse); - rpc GetImpersonatedUser(user.GetUserRequest) returns (user.GetUserResponse); // Groups API rpc GetGroup(grp.GetGroupRequest) returns (grp.GetGroupResponse); diff --git a/proto/user.proto b/proto/user.proto index 0ec0d63f662..19c2f6f6a97 100644 --- a/proto/user.proto +++ b/proto/user.proto @@ -58,6 +58,9 @@ message GetUserResponse { // group ID that owns the subdomain, but only if the user is allowed to // impersonate. string subdomain_group_id = 8; + + // True if the user is using impersonation to access a group. + bool is_impersonating = 10; } message CreateUserRequest { diff --git a/server/buildbuddy_server/buildbuddy_server.go b/server/buildbuddy_server/buildbuddy_server.go index 448ff667a99..e4ca9f3e68b 100644 --- a/server/buildbuddy_server/buildbuddy_server.go +++ b/server/buildbuddy_server/buildbuddy_server.go @@ -274,22 +274,6 @@ func makeGroups(groupRoles []*tables.GroupRole) []*grpb.Group { return r } -func (s *BuildBuddyServer) GetUser(ctx context.Context, req *uspb.GetUserRequest) (*uspb.GetUserResponse, error) { - userDB := s.env.GetUserDB() - if userDB == nil { - return nil, status.UnimplementedError("Not Implemented") - } - return s.getUser(ctx, req, userDB.GetUser) -} - -func (s *BuildBuddyServer) GetImpersonatedUser(ctx context.Context, req *uspb.GetUserRequest) (*uspb.GetUserResponse, error) { - userDB := s.env.GetUserDB() - if userDB == nil { - return nil, status.UnimplementedError("Not Implemented") - } - return s.getUser(ctx, req, userDB.GetImpersonatedUser) -} - func (s *BuildBuddyServer) getGroupIDForSubdomain(ctx context.Context) (string, error) { sd := subdomain.Get(ctx) if sd == "" { @@ -308,10 +292,21 @@ func (s *BuildBuddyServer) getGroupIDForSubdomain(ctx context.Context) (string, return g.GroupID, nil } -type userLookup func(ctx context.Context) (*tables.User, error) +func (s *BuildBuddyServer) GetUser(ctx context.Context, req *uspb.GetUserRequest) (*uspb.GetUserResponse, error) { + userDB := s.env.GetUserDB() + if userDB == nil { + return nil, status.UnimplementedError("Not Implemented") + } -func (s *BuildBuddyServer) getUser(ctx context.Context, req *uspb.GetUserRequest, dbLookup userLookup) (*uspb.GetUserResponse, error) { - tu, err := dbLookup(ctx) + auth := s.env.GetAuthenticator() + if auth == nil { + return nil, status.InternalError("No auth configured on this BuildBuddy instance") + } + u, err := s.env.GetAuthenticator().AuthenticatedUser(ctx) + if err != nil { + return nil, err + } + tu, err := s.env.GetUserDB().GetUser(ctx) if err != nil { return nil, err } @@ -370,6 +365,7 @@ func (s *BuildBuddyServer) getUser(ctx context.Context, req *uspb.GetUserRequest AllowedRpc: allowedRPCs, GithubLinked: tu.GithubToken != "", SubdomainGroupId: subdomainGroupID, + IsImpersonating: u.IsImpersonating(), }, nil } diff --git a/server/role_filter/role_filter.go b/server/role_filter/role_filter.go index 8e8ca591eff..eeb9faa198c 100644 --- a/server/role_filter/role_filter.go +++ b/server/role_filter/role_filter.go @@ -24,7 +24,6 @@ var ( roleIndependentRPCs = []string{ // RPCs that happen pre-login and don't require group membership. "GetUser", - "GetImpersonatedUser", "CreateUser", "GetGroup", // Invocations can be shared publicly, so authorization for these RPCs is diff --git a/server/util/claims/BUILD b/server/util/claims/BUILD index cb58cba17e3..b10e5b59b41 100644 --- a/server/util/claims/BUILD +++ b/server/util/claims/BUILD @@ -17,6 +17,7 @@ go_library( "//server/util/request_context", "//server/util/role", "//server/util/status", + "//server/util/subdomain", "@com_github_golang_jwt_jwt//:jwt", ], ) diff --git a/server/util/claims/claims.go b/server/util/claims/claims.go index 4a179ba0aa6..8fb9e1553d6 100644 --- a/server/util/claims/claims.go +++ b/server/util/claims/claims.go @@ -14,6 +14,7 @@ import ( "github.com/buildbuddy-io/buildbuddy/server/util/lru" "github.com/buildbuddy-io/buildbuddy/server/util/role" "github.com/buildbuddy-io/buildbuddy/server/util/status" + "github.com/buildbuddy-io/buildbuddy/server/util/subdomain" "github.com/golang-jwt/jwt" akpb "github.com/buildbuddy-io/buildbuddy/proto/api_key" @@ -161,8 +162,20 @@ func ClaimsFromSubID(ctx context.Context, env environment.Env, subID string) (*C if membership.GroupID != env.GetAuthenticator().AdminGroupID() || membership.Role != role.Admin { continue } + + ig, err := env.GetUserDB().GetGroupByID(ctx, c.GetImpersonatingGroupId()) + if err != nil { + return nil, err + } + + // If the user requested impersonation but the subdomain doesn't + // match the impersonation target then don't enable impersonation. + if sd := subdomain.Get(ctx); sd != "" && ig.URLIdentifier != nil && sd != *ig.URLIdentifier { + return claims, nil + } + u.Groups = []*tables.GroupRole{{ - Group: tables.Group{GroupID: c.GetImpersonatingGroupId()}, + Group: *ig, Role: uint32(role.Admin), }} claims := userClaims(u, c.GetImpersonatingGroupId())