From cb30e433f43c5c86e259b4faa9a23c152aa61fe0 Mon Sep 17 00:00:00 2001 From: Jakub Nyckowski Date: Mon, 24 Jul 2023 13:35:37 -0400 Subject: [PATCH 01/13] "Add authorization rules to the Assistant and UserPreferences service" This commit introduces authorization rules into the Assistant service to restrict operations based on the authenticated user's role permissions. Now each method in the Assistant service checks if the authenticated user has necessary permissions to perform the requested operation. The permissions are checked via defined RBAC rules. A user requires specific permissions to perform various operations such as creating a conversation, updating a conversation, fetching a user's conversations, deleting a conversation, and adding a message to a conversation. Also, even if a user has necessary permissions, they cannot perform operations for a different user. Each user can only access their own data. --- lib/auth/assist/assistv1/service.go | 63 ++- lib/auth/assist/assistv1/service_test.go | 368 ++++++++++++++++++ lib/auth/auth_with_roles.go | 59 +-- lib/auth/grpcserver.go | 3 +- .../userpreferencesv1/service.go | 36 +- .../userpreferencesv1/service_test.go | 262 +++++++++++++ 6 files changed, 735 insertions(+), 56 deletions(-) create mode 100644 lib/auth/assist/assistv1/service_test.go create mode 100644 lib/auth/userpreferences/userpreferencesv1/service_test.go diff --git a/lib/auth/assist/assistv1/service.go b/lib/auth/assist/assistv1/service.go index a46dda271bbe1..8f721d91e423f 100644 --- a/lib/auth/assist/assistv1/service.go +++ b/lib/auth/assist/assistv1/service.go @@ -93,13 +93,31 @@ func NewService(cfg *ServiceConfig) (*Service, error) { // CreateAssistantConversation creates a new conversation entry in the backend. func (a *Service) CreateAssistantConversation(ctx context.Context, req *assist.CreateAssistantConversationRequest) (*assist.CreateAssistantConversationResponse, error) { + authCtx, err := authz.AuthorizeWithVerbs(ctx, a.log, a.authorizer, true, types.KindAssistant, types.VerbCreate) + if err != nil { + return nil, trace.Wrap(err) + } + + if authCtx.User.GetName() != req.GetUsername() { + return nil, trace.AccessDenied("user %q is not allowed to create conversation for user %q", authCtx.User.GetName(), req.Username) + } + resp, err := a.backend.CreateAssistantConversation(ctx, req) return resp, trace.Wrap(err) } // UpdateAssistantConversationInfo updates the conversation info for a conversation. -func (a *Service) UpdateAssistantConversationInfo(ctx context.Context, request *assist.UpdateAssistantConversationInfoRequest) (*emptypb.Empty, error) { - err := a.backend.UpdateAssistantConversationInfo(ctx, request) +func (a *Service) UpdateAssistantConversationInfo(ctx context.Context, req *assist.UpdateAssistantConversationInfoRequest) (*emptypb.Empty, error) { + authCtx, err := authz.AuthorizeWithVerbs(ctx, a.log, a.authorizer, true, types.KindAssistant, types.VerbUpdate) + if err != nil { + return nil, trace.Wrap(err) + } + + if authCtx.User.GetName() != req.GetUsername() { + return nil, trace.AccessDenied("user %q is not allowed to update conversation for user %q", authCtx.User.GetName(), req.Username) + } + + err = a.backend.UpdateAssistantConversationInfo(ctx, req) if err != nil { return &emptypb.Empty{}, trace.Wrap(err) } @@ -109,28 +127,69 @@ func (a *Service) UpdateAssistantConversationInfo(ctx context.Context, request * // GetAssistantConversations returns all conversations started by a user. func (a *Service) GetAssistantConversations(ctx context.Context, req *assist.GetAssistantConversationsRequest) (*assist.GetAssistantConversationsResponse, error) { + authCtx, err := authz.AuthorizeWithVerbs(ctx, a.log, a.authorizer, true, types.KindAssistant, types.VerbRead, types.VerbList) + if err != nil { + return nil, trace.Wrap(err) + } + + if authCtx.User.GetName() != req.GetUsername() { + return nil, trace.AccessDenied("user %q is not allowed to list conversations for user %q", authCtx.User.GetName(), req.GetUsername()) + } + resp, err := a.backend.GetAssistantConversations(ctx, req) return resp, trace.Wrap(err) } // DeleteAssistantConversation deletes a conversation entry and associated messages from the backend. func (a *Service) DeleteAssistantConversation(ctx context.Context, req *assist.DeleteAssistantConversationRequest) (*emptypb.Empty, error) { + authCtx, err := authz.AuthorizeWithVerbs(ctx, a.log, a.authorizer, true, types.KindAssistant, types.VerbDelete) + if err != nil { + return nil, trace.Wrap(err) + } + + if authCtx.User.GetName() != req.GetUsername() { + return nil, trace.AccessDenied("user %q is not allowed to delete conversation for user %q", authCtx.User.GetName(), req.GetUsername()) + } + return &emptypb.Empty{}, trace.Wrap(a.backend.DeleteAssistantConversation(ctx, req)) } // GetAssistantMessages returns all messages with given conversation ID. func (a *Service) GetAssistantMessages(ctx context.Context, req *assist.GetAssistantMessagesRequest) (*assist.GetAssistantMessagesResponse, error) { + authCtx, err := authz.AuthorizeWithVerbs(ctx, a.log, a.authorizer, true, types.KindAssistant, types.VerbRead) + if err != nil { + return nil, trace.Wrap(err) + } + + if authCtx.User.GetName() != req.GetUsername() { + return nil, trace.AccessDenied("user %q is not allowed to get messages for user %q", authCtx.User.GetName(), req.GetUsername()) + } + resp, err := a.backend.GetAssistantMessages(ctx, req) return resp, trace.Wrap(err) } // CreateAssistantMessage adds the message to the backend. func (a *Service) CreateAssistantMessage(ctx context.Context, req *assist.CreateAssistantMessageRequest) (*emptypb.Empty, error) { + authCtx, err := authz.AuthorizeWithVerbs(ctx, a.log, a.authorizer, true, types.KindAssistant, types.VerbCreate) + if err != nil { + return nil, trace.Wrap(err) + } + + if authCtx.User.GetName() != req.GetUsername() { + return nil, trace.AccessDenied("user %q is not allowed to create message for user %q", authCtx.User.GetName(), req.GetUsername()) + } + return &emptypb.Empty{}, trace.Wrap(a.backend.CreateAssistantMessage(ctx, req)) } // IsAssistEnabled returns true if the assist is enabled or not on the auth level. func (a *Service) IsAssistEnabled(ctx context.Context, _ *assist.IsAssistEnabledRequest) (*assist.IsAssistEnabledResponse, error) { + _, err := authz.AuthorizeWithVerbs(ctx, a.log, a.authorizer, true, types.KindAssistant, types.VerbRead) + if err != nil { + return nil, trace.Wrap(err) + } + if a.embedder == nil { // If the embedder is not configured, the assist is not enabled as we cannot compute embeddings. return &assist.IsAssistEnabledResponse{Enabled: false}, nil diff --git a/lib/auth/assist/assistv1/service_test.go b/lib/auth/assist/assistv1/service_test.go new file mode 100644 index 0000000000000..ce06d8d38bbdf --- /dev/null +++ b/lib/auth/assist/assistv1/service_test.go @@ -0,0 +1,368 @@ +/* + * Copyright 2023 Gravitational, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package assistv1 + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/protobuf/types/known/timestamppb" + + assistpb "github.com/gravitational/teleport/api/gen/proto/go/assist/v1" + "github.com/gravitational/teleport/api/types" + "github.com/gravitational/teleport/lib/ai" + "github.com/gravitational/teleport/lib/assist" + "github.com/gravitational/teleport/lib/authz" + "github.com/gravitational/teleport/lib/backend/memory" + "github.com/gravitational/teleport/lib/services" + "github.com/gravitational/teleport/lib/services/local" + "github.com/gravitational/teleport/lib/tlsca" +) + +const ( + defaultUser = "test-user" + noAccessUser = "user-no-access" +) + +func TestService_CreateAssistantConversation(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + username string + req *assistpb.CreateAssistantConversationRequest + wantErr assert.ErrorAssertionFunc + assertResponse func(t *testing.T, resp *assistpb.CreateAssistantConversationResponse) + }{ + { + name: "success", + username: defaultUser, + req: &assistpb.CreateAssistantConversationRequest{ + Username: defaultUser, + CreatedTime: timestamppb.Now(), + }, + wantErr: assert.NoError, + assertResponse: func(t *testing.T, resp *assistpb.CreateAssistantConversationResponse) { + require.NotEmpty(t, resp.GetId()) + }, + }, + { + name: "access denies - RBAC", + username: noAccessUser, + req: &assistpb.CreateAssistantConversationRequest{ + Username: noAccessUser, + CreatedTime: timestamppb.Now(), + }, + wantErr: assert.Error, + }, + { + name: "access denied - different user", + username: defaultUser, + req: &assistpb.CreateAssistantConversationRequest{ + Username: noAccessUser, + CreatedTime: timestamppb.Now(), + }, + wantErr: assert.Error, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctxs, svc := initSvc(t) + + got, err := svc.CreateAssistantConversation(ctxs[tt.username], tt.req) + tt.wantErr(t, err) + + if tt.assertResponse != nil { + tt.assertResponse(t, got) + } + }) + } +} + +func TestService_GetAssistantConversations(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + username string + req *assistpb.GetAssistantConversationsRequest + wantErr assert.ErrorAssertionFunc + assertResponse func(t *testing.T, resp *assistpb.CreateAssistantConversationResponse) + }{ + { + name: "success", + username: defaultUser, + req: &assistpb.GetAssistantConversationsRequest{ + Username: defaultUser, + }, + wantErr: assert.NoError, + }, + { + name: "access denies - RBAC", + username: noAccessUser, + req: &assistpb.GetAssistantConversationsRequest{ + Username: noAccessUser, + }, + wantErr: assert.Error, + }, + { + name: "access denied - different user", + username: defaultUser, + req: &assistpb.GetAssistantConversationsRequest{ + Username: noAccessUser, + }, + wantErr: assert.Error, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctxs, svc := initSvc(t) + + _, err := svc.GetAssistantConversations(ctxs[tt.username], tt.req) + tt.wantErr(t, err) + }) + } +} + +func TestService_DeleteAssistantConversations(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + username string + req *assistpb.DeleteAssistantConversationRequest + wantErr assert.ErrorAssertionFunc + }{ + { + name: "success", + username: defaultUser, + req: &assistpb.DeleteAssistantConversationRequest{ + Username: defaultUser, + }, + wantErr: assert.NoError, + }, + { + name: "access denies - RBAC", + username: noAccessUser, + req: &assistpb.DeleteAssistantConversationRequest{ + Username: noAccessUser, + }, + wantErr: assert.Error, + }, + { + name: "access denied - different user", + username: defaultUser, + req: &assistpb.DeleteAssistantConversationRequest{ + Username: noAccessUser, + }, + wantErr: assert.Error, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctxs, svc := initSvc(t) + + // Create a conversation that we can remove, so we don't hit "conversation doesn't exist" error + convMsg, err := svc.backend.CreateAssistantConversation(ctxs[tt.username], &assistpb.CreateAssistantConversationRequest{ + Username: tt.username, + CreatedTime: timestamppb.Now(), + }) + require.NoError(t, err) + + conversationID := convMsg.GetId() + + tt.req.ConversationId = conversationID + + _, err = svc.DeleteAssistantConversation(ctxs[tt.username], tt.req) + tt.wantErr(t, err) + }) + } +} + +func TestService_InsertAssistantMessage(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + username string + req *assistpb.CreateAssistantMessageRequest + wantErr assert.ErrorAssertionFunc + }{ + { + name: "success", + username: defaultUser, + req: &assistpb.CreateAssistantMessageRequest{ + Username: defaultUser, + Message: &assistpb.AssistantMessage{ + Type: string(assist.MessageKindAssistantMessage), + CreatedTime: timestamppb.Now(), + Payload: "Blah", + }, + }, + wantErr: assert.NoError, + }, + { + name: "access denies - RBAC", + username: noAccessUser, + req: &assistpb.CreateAssistantMessageRequest{ + Username: noAccessUser, + }, + wantErr: assert.Error, + }, + { + name: "access denied - different user", + username: defaultUser, + req: &assistpb.CreateAssistantMessageRequest{ + Username: noAccessUser, + }, + wantErr: assert.Error, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctxs, svc := initSvc(t) + + // Create a conversation that we can remove, so we don't hit "conversation doesn't exist" error + convMsg, err := svc.backend.CreateAssistantConversation(ctxs[tt.username], &assistpb.CreateAssistantConversationRequest{ + Username: tt.username, + CreatedTime: timestamppb.Now(), + }) + require.NoError(t, err) + + conversationID := convMsg.GetId() + + tt.req.ConversationId = conversationID + + _, err = svc.CreateAssistantMessage(ctxs[tt.username], tt.req) + tt.wantErr(t, err) + }) + } +} + +func initSvc(t *testing.T) (map[string]context.Context, *Service) { + ctx := context.Background() + backend, err := memory.New(memory.Config{}) + require.NoError(t, err) + + clusterConfigSvc, err := local.NewClusterConfigurationService(backend) + require.NoError(t, err) + trustSvc := local.NewCAService(backend) + roleSvc := local.NewAccessService(backend) + userSvc := local.NewIdentityService(backend) + + require.NoError(t, clusterConfigSvc.SetAuthPreference(ctx, types.DefaultAuthPreference())) + require.NoError(t, clusterConfigSvc.SetClusterAuditConfig(ctx, types.DefaultClusterAuditConfig())) + require.NoError(t, clusterConfigSvc.SetClusterNetworkingConfig(ctx, types.DefaultClusterNetworkingConfig())) + require.NoError(t, clusterConfigSvc.SetSessionRecordingConfig(ctx, types.DefaultSessionRecordingConfig())) + + accessPoint := struct { + services.ClusterConfiguration + services.Trust + services.RoleGetter + services.UserGetter + }{ + ClusterConfiguration: clusterConfigSvc, + Trust: trustSvc, + RoleGetter: roleSvc, + UserGetter: userSvc, + } + + accessService := local.NewAccessService(backend) + eventService := local.NewEventsService(backend) + lockWatcher, err := services.NewLockWatcher(ctx, services.LockWatcherConfig{ + ResourceWatcherConfig: services.ResourceWatcherConfig{ + Client: eventService, + Component: "test", + }, + LockGetter: accessService, + }) + require.NoError(t, err) + + authorizer, err := authz.NewAuthorizer(authz.AuthorizerOpts{ + ClusterName: "test-cluster", + AccessPoint: accessPoint, + LockWatcher: lockWatcher, + }) + require.NoError(t, err) + + roles := map[string]types.Role{} + + role, err := types.NewRole("allow-rules", types.RoleSpecV6{ + Allow: types.RoleConditions{ + Rules: []types.Rule{ + { + Resources: []string{types.KindAssistant}, + Verbs: []string{types.VerbList, types.VerbRead, types.VerbUpdate, types.VerbCreate, types.VerbDelete}, + }, + }, + }, + }) + require.NoError(t, err) + + roles[defaultUser] = role + + roleNoAccess, err := types.NewRole("no-rules", types.RoleSpecV6{ + Allow: types.RoleConditions{}, + }) + require.NoError(t, err) + roles["user-no-access"] = roleNoAccess + + ctxs := make(map[string]context.Context, len(roles)) + for username, role := range roles { + err = roleSvc.CreateRole(ctx, role) + require.NoError(t, err) + + user, err := types.NewUser(username) + user.AddRole(role.GetName()) + require.NoError(t, err) + + err = userSvc.CreateUser(user) + require.NoError(t, err) + + ctx = authz.ContextWithUser(ctx, authz.LocalUser{ + Username: user.GetName(), + Identity: tlsca.Identity{ + Username: user.GetName(), + Groups: []string{role.GetName()}, + }, + }) + ctxs[user.GetName()] = ctx + } + + svc, err := NewService(&ServiceConfig{ + Backend: local.NewAssistService(backend), + Authorizer: authorizer, + Embeddings: &ai.SimpleRetriever{}, + ResourceGetter: &nodeGetterFake{}, + }) + require.NoError(t, err) + + return ctxs, svc +} + +type nodeGetterFake struct { +} + +func (g *nodeGetterFake) GetNode(ctx context.Context, namespace, name string) (types.Server, error) { + return nil, nil +} diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go index 5ee74890070d6..f6d939b8e380a 100644 --- a/lib/auth/auth_with_roles.go +++ b/lib/auth/auth_with_roles.go @@ -6438,88 +6438,47 @@ func (a *ServerWithRoles) WatchPendingHeadlessAuthentications(ctx context.Contex // CreateAssistantConversation creates a new conversation entry in the backend. func (a *ServerWithRoles) CreateAssistantConversation(ctx context.Context, req *assist.CreateAssistantConversationRequest) (*assist.CreateAssistantConversationResponse, error) { - if err := a.action(apidefaults.Namespace, types.KindAssistant, types.VerbCreate); err != nil { - return nil, trace.Wrap(err) - } - - return a.authServer.CreateAssistantConversation(ctx, req) + return nil, trace.NotImplemented("CreateAssistantConversation must not be called on auth.ServerWithRoles") } // GetAssistantConversations returns all conversations started by a user. func (a *ServerWithRoles) GetAssistantConversations(ctx context.Context, request *assist.GetAssistantConversationsRequest) (*assist.GetAssistantConversationsResponse, error) { - if err := a.action(apidefaults.Namespace, types.KindAssistant, types.VerbList); err != nil { - return nil, trace.Wrap(err) - } - - return a.authServer.GetAssistantConversations(ctx, request) + return nil, trace.NotImplemented("GetAssistantConversations must not be called on auth.ServerWithRoles") } // GetAssistantMessages returns all messages with given conversation ID. func (a *ServerWithRoles) GetAssistantMessages(ctx context.Context, req *assist.GetAssistantMessagesRequest) (*assist.GetAssistantMessagesResponse, error) { - if err := a.action(apidefaults.Namespace, types.KindAssistant, types.VerbRead); err != nil { - return nil, trace.Wrap(err) - } - - return a.authServer.GetAssistantMessages(ctx, req) + return nil, trace.NotImplemented("GetAssistantMessages must not be called on auth.ServerWithRoles") } // DeleteAssistantConversation deletes a conversation by ID. func (a *ServerWithRoles) DeleteAssistantConversation(ctx context.Context, req *assist.DeleteAssistantConversationRequest) error { - if err := a.action(apidefaults.Namespace, types.KindAssistant, types.VerbDelete); err != nil { - return trace.Wrap(err) - } - - return trace.Wrap(a.authServer.DeleteAssistantConversation(ctx, req)) + return trace.NotImplemented("DeleteAssistantConversation must not be called on auth.ServerWithRoles") } // IsAssistEnabled returns true if the assist is enabled or not on the auth level. func (a *ServerWithRoles) IsAssistEnabled(ctx context.Context) (*assist.IsAssistEnabledResponse, error) { - if err := a.action(apidefaults.Namespace, types.KindAssistant, types.VerbRead); err != nil { - return nil, trace.Wrap(err) - } - - return a.authServer.IsAssistEnabled(ctx) + return nil, trace.NotImplemented("IsAssistEnabled must not be called on auth.ServerWithRoles") } // CreateAssistantMessage adds the message to the backend. func (a *ServerWithRoles) CreateAssistantMessage(ctx context.Context, msg *assist.CreateAssistantMessageRequest) error { - if err := a.action(apidefaults.Namespace, types.KindAssistant, types.VerbUpdate); err != nil { - return trace.Wrap(err) - } - - return a.authServer.CreateAssistantMessage(ctx, msg) + return trace.NotImplemented("CreateAssistantMessage must not be called on auth.ServerWithRoles") } // UpdateAssistantConversationInfo updates the conversation info. func (a *ServerWithRoles) UpdateAssistantConversationInfo(ctx context.Context, msg *assist.UpdateAssistantConversationInfoRequest) error { - if err := a.action(apidefaults.Namespace, types.KindAssistant, types.VerbUpdate); err != nil { - return trace.Wrap(err) - } - - return a.authServer.UpdateAssistantConversationInfo(ctx, msg) + return trace.NotImplemented("UpdateAssistantConversationInfo must not be called on auth.ServerWithRoles") } // GetUserPreferences returns the user preferences for a given user. func (a *ServerWithRoles) GetUserPreferences(ctx context.Context, req *userpreferencespb.GetUserPreferencesRequest) (*userpreferencespb.GetUserPreferencesResponse, error) { - if err := a.currentUserAction(req.Username); err != nil { - return nil, trace.Wrap(err) - } - - preferences, err := a.authServer.GetUserPreferences(ctx, req) - if err != nil { - return nil, trace.Wrap(err) - } - - return preferences, nil + return nil, trace.NotImplemented("GetUserPreferences must not be called on auth.ServerWithRoles") } // UpsertUserPreferences creates or updates user preferences for a given username. func (a *ServerWithRoles) UpsertUserPreferences(ctx context.Context, req *userpreferencespb.UpsertUserPreferencesRequest) error { - if err := a.currentUserAction(req.Username); err != nil { - return trace.Wrap(err) - } - - return trace.Wrap(a.authServer.UpsertUserPreferences(ctx, req)) + return trace.NotImplemented("UpsertUserPreferences must not be called on auth.ServerWithRoles") } // CloneHTTPClient creates a new HTTP client with the same configuration. diff --git a/lib/auth/grpcserver.go b/lib/auth/grpcserver.go index 578e9033b8005..2e9d61d11f30b 100644 --- a/lib/auth/grpcserver.go +++ b/lib/auth/grpcserver.go @@ -5458,7 +5458,8 @@ func NewGRPCServer(cfg GRPCServerConfig) (*GRPCServer, error) { // Initialize and register the user preferences service. userPreferencesSrv, err := userpreferencesv1.NewService(&userpreferencesv1.ServiceConfig{ - Backend: cfg.AuthServer.Services, + Backend: cfg.AuthServer.Services, + Authorizer: cfg.Authorizer, }) if err != nil { return nil, trace.Wrap(err) diff --git a/lib/auth/userpreferences/userpreferencesv1/service.go b/lib/auth/userpreferences/userpreferencesv1/service.go index 933c3f9fe8d54..e6d643e915065 100644 --- a/lib/auth/userpreferences/userpreferencesv1/service.go +++ b/lib/auth/userpreferences/userpreferencesv1/service.go @@ -21,22 +21,29 @@ import ( "context" "github.com/gravitational/trace" + "github.com/sirupsen/logrus" "google.golang.org/protobuf/types/known/emptypb" userpreferences "github.com/gravitational/teleport/api/gen/proto/go/userpreferences/v1" + "github.com/gravitational/teleport/api/types" + "github.com/gravitational/teleport/lib/authz" "github.com/gravitational/teleport/lib/services" ) // ServiceConfig holds configuration options for the user preferences service. type ServiceConfig struct { - Backend services.UserPreferences + Backend services.UserPreferences + Authorizer authz.Authorizer + Logger *logrus.Entry } // Service implements the teleport.userpreferences.v1.UserPreferencesService RPC service. type Service struct { userpreferences.UnimplementedUserPreferencesServiceServer - backend services.UserPreferences + backend services.UserPreferences + authorizer authz.Authorizer + log *logrus.Entry } // NewService returns a new user preferences gRPC service. @@ -44,19 +51,42 @@ func NewService(cfg *ServiceConfig) (*Service, error) { switch { case cfg.Backend == nil: return nil, trace.BadParameter("backend is required") + case cfg.Authorizer == nil: + return nil, trace.BadParameter("authorizer is required") + case cfg.Logger == nil: + cfg.Logger = logrus.WithField(trace.Component, "userpreferences.service") } return &Service{ - backend: cfg.Backend, + backend: cfg.Backend, + authorizer: cfg.Authorizer, }, nil } // GetUserPreferences returns the user preferences for a given user. func (a *Service) GetUserPreferences(ctx context.Context, req *userpreferences.GetUserPreferencesRequest) (*userpreferences.GetUserPreferencesResponse, error) { + authCtx, err := authz.AuthorizeWithVerbs(ctx, a.log, a.authorizer, true, types.KindUser, types.VerbUpdate) + if err != nil { + return nil, trace.Wrap(err) + } + + if authCtx.User.GetName() != req.GetUsername() { + return nil, trace.AccessDenied("user %q is not allowed to access preferences for user %q", authCtx.User.GetName(), req.Username) + } + return a.backend.GetUserPreferences(ctx, req) } // UpsertUserPreferences creates or updates user preferences for a given username. func (a *Service) UpsertUserPreferences(ctx context.Context, req *userpreferences.UpsertUserPreferencesRequest) (*emptypb.Empty, error) { + authCtx, err := authz.AuthorizeWithVerbs(ctx, a.log, a.authorizer, true, types.KindUser, types.VerbUpdate) + if err != nil { + return nil, trace.Wrap(err) + } + + if authCtx.User.GetName() != req.GetUsername() { + return nil, trace.AccessDenied("user %q is not allowed to access preferences for user %q", authCtx.User.GetName(), req.Username) + } + return &emptypb.Empty{}, trace.Wrap(a.backend.UpsertUserPreferences(ctx, req)) } diff --git a/lib/auth/userpreferences/userpreferencesv1/service_test.go b/lib/auth/userpreferences/userpreferencesv1/service_test.go new file mode 100644 index 0000000000000..e553ad244f69d --- /dev/null +++ b/lib/auth/userpreferences/userpreferencesv1/service_test.go @@ -0,0 +1,262 @@ +/* + * Copyright 2023 Gravitational, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package userpreferencesv1 + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + userpreferencesv1 "github.com/gravitational/teleport/api/gen/proto/go/userpreferences/v1" + "github.com/gravitational/teleport/api/types" + "github.com/gravitational/teleport/lib/authz" + "github.com/gravitational/teleport/lib/backend/memory" + "github.com/gravitational/teleport/lib/services" + "github.com/gravitational/teleport/lib/services/local" + "github.com/gravitational/teleport/lib/tlsca" +) + +const ( + defaultUser = "test-user" + noAccessUser = "user-no-access" +) + +func TestService_GetUserPreferences(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + userName string + req *userpreferencesv1.GetUserPreferencesRequest + want *userpreferencesv1.GetUserPreferencesResponse + wantErr assert.ErrorAssertionFunc + }{ + { + name: "success", + userName: defaultUser, + req: &userpreferencesv1.GetUserPreferencesRequest{ + Username: defaultUser, + }, + want: &userpreferencesv1.GetUserPreferencesResponse{ + Preferences: &userpreferencesv1.UserPreferences{ + Assist: &userpreferencesv1.AssistUserPreferences{ + PreferredLogins: []string{}, + ViewMode: userpreferencesv1.AssistViewMode_ASSIST_VIEW_MODE_DOCKED, + }, + Theme: userpreferencesv1.Theme_THEME_LIGHT, + Onboard: &userpreferencesv1.OnboardUserPreferences{ + PreferredResources: []userpreferencesv1.Resource{}, + }, + }, + }, + wantErr: assert.NoError, + }, + { + name: "access denied - incorrect user", + userName: defaultUser, + req: &userpreferencesv1.GetUserPreferencesRequest{ + Username: "non-existent-user", + }, + want: nil, + wantErr: assert.Error, + }, + { + name: "access denied - RBAC rule", + userName: noAccessUser, + req: &userpreferencesv1.GetUserPreferencesRequest{ + Username: "test-user-no-access", + }, + want: nil, + wantErr: assert.Error, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctxs, svc := initSvc(t) + + got, err := svc.GetUserPreferences(ctxs[tt.userName], tt.req) + tt.wantErr(t, err) + + if tt.want != nil { + require.Equal(t, tt.want, got) + } + }) + } +} + +func TestService_UpsertUserPreferences(t *testing.T) { + t.Parallel() + + defaultPreferences := &userpreferencesv1.UserPreferences{ + Assist: &userpreferencesv1.AssistUserPreferences{ + PreferredLogins: []string{}, + ViewMode: userpreferencesv1.AssistViewMode_ASSIST_VIEW_MODE_DOCKED, + }, + Theme: userpreferencesv1.Theme_THEME_LIGHT, + Onboard: &userpreferencesv1.OnboardUserPreferences{ + PreferredResources: []userpreferencesv1.Resource{}, + }, + } + + tests := []struct { + name string + userName string + req *userpreferencesv1.UpsertUserPreferencesRequest + wantErr assert.ErrorAssertionFunc + }{ + { + name: "success", + userName: defaultUser, + req: &userpreferencesv1.UpsertUserPreferencesRequest{ + Username: defaultUser, + Preferences: defaultPreferences, + }, + wantErr: assert.NoError, + }, + { + name: "access denied - incorrect user", + userName: defaultUser, + req: &userpreferencesv1.UpsertUserPreferencesRequest{ + Username: "non-existent-user", + Preferences: defaultPreferences, + }, + wantErr: assert.Error, + }, + { + name: "access denied - RBAC rule", + userName: noAccessUser, + req: &userpreferencesv1.UpsertUserPreferencesRequest{ + Username: noAccessUser, + Preferences: defaultPreferences, + }, + wantErr: assert.Error, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctxs, svc := initSvc(t) + + _, err := svc.UpsertUserPreferences(ctxs[tt.userName], tt.req) + if tt.wantErr(t, err) { + return + } + }) + } +} + +func initSvc(t *testing.T) (map[string]context.Context, *Service) { + ctx := context.Background() + backend, err := memory.New(memory.Config{}) + require.NoError(t, err) + + clusterConfigSvc, err := local.NewClusterConfigurationService(backend) + require.NoError(t, err) + trustSvc := local.NewCAService(backend) + roleSvc := local.NewAccessService(backend) + userSvc := local.NewIdentityService(backend) + + require.NoError(t, clusterConfigSvc.SetAuthPreference(ctx, types.DefaultAuthPreference())) + require.NoError(t, clusterConfigSvc.SetClusterAuditConfig(ctx, types.DefaultClusterAuditConfig())) + require.NoError(t, clusterConfigSvc.SetClusterNetworkingConfig(ctx, types.DefaultClusterNetworkingConfig())) + require.NoError(t, clusterConfigSvc.SetSessionRecordingConfig(ctx, types.DefaultSessionRecordingConfig())) + + accessPoint := struct { + services.ClusterConfiguration + services.Trust + services.RoleGetter + services.UserGetter + }{ + ClusterConfiguration: clusterConfigSvc, + Trust: trustSvc, + RoleGetter: roleSvc, + UserGetter: userSvc, + } + + accessService := local.NewAccessService(backend) + eventService := local.NewEventsService(backend) + lockWatcher, err := services.NewLockWatcher(ctx, services.LockWatcherConfig{ + ResourceWatcherConfig: services.ResourceWatcherConfig{ + Client: eventService, + Component: "test", + }, + LockGetter: accessService, + }) + require.NoError(t, err) + + authorizer, err := authz.NewAuthorizer(authz.AuthorizerOpts{ + ClusterName: "test-cluster", + AccessPoint: accessPoint, + LockWatcher: lockWatcher, + }) + require.NoError(t, err) + + roles := map[string]types.Role{} + + role, err := types.NewRole("allow-rules", types.RoleSpecV6{ + Allow: types.RoleConditions{ + Rules: []types.Rule{ + { + Resources: []string{types.KindUser}, + Verbs: []string{types.VerbList, types.VerbRead, types.VerbUpdate, types.VerbCreate, types.VerbDelete}, + }, + }, + }, + }) + require.NoError(t, err) + + roles[defaultUser] = role + + roleNoAccess, err := types.NewRole("no-rules", types.RoleSpecV6{ + Allow: types.RoleConditions{}, + }) + require.NoError(t, err) + roles["user-no-access"] = roleNoAccess + + ctxs := make(map[string]context.Context, len(roles)) + for username, role := range roles { + err = roleSvc.CreateRole(ctx, role) + require.NoError(t, err) + + user, err := types.NewUser(username) + user.AddRole(role.GetName()) + require.NoError(t, err) + + err = userSvc.CreateUser(user) + require.NoError(t, err) + + ctx = authz.ContextWithUser(ctx, authz.LocalUser{ + Username: user.GetName(), + Identity: tlsca.Identity{ + Username: user.GetName(), + Groups: []string{role.GetName()}, + }, + }) + ctxs[user.GetName()] = ctx + } + + svc, err := NewService(&ServiceConfig{ + Backend: local.NewUserPreferencesService(backend), + Authorizer: authorizer, + }) + require.NoError(t, err) + + return ctxs, svc +} From 22211b2d9bbd4a104d99a90b2b7254c564a4da07 Mon Sep 17 00:00:00 2001 From: Jakub Nyckowski Date: Mon, 24 Jul 2023 15:36:01 -0400 Subject: [PATCH 02/13] Add missing logger --- lib/auth/userpreferences/userpreferencesv1/service.go | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/auth/userpreferences/userpreferencesv1/service.go b/lib/auth/userpreferences/userpreferencesv1/service.go index e6d643e915065..f99cc9b025f24 100644 --- a/lib/auth/userpreferences/userpreferencesv1/service.go +++ b/lib/auth/userpreferences/userpreferencesv1/service.go @@ -60,6 +60,7 @@ func NewService(cfg *ServiceConfig) (*Service, error) { return &Service{ backend: cfg.Backend, authorizer: cfg.Authorizer, + log: cfg.Logger, }, nil } From 5e92a87f50ddbccd0cfece5156856306c4a4e3e4 Mon Sep 17 00:00:00 2001 From: Jakub Nyckowski Date: Mon, 24 Jul 2023 16:54:50 -0400 Subject: [PATCH 03/13] "Refactor user preferences request handling" This commit refactors how GetUserPreferences and UpsertUserPreferences handle requests. The `username` field is removed from request parameters. Instead of having the client send the user's username in a request, the server now automatically uses the username of the authenticated user making the request. This change improves the security by preventing a user from attempting to fetch or manipulate another user's preferences. Removed tests were specifically testing the old, insecure behavior. --- .../userpreferences/v1/userpreferences.pb.go | 96 +++++++------------ .../userpreferences/v1/userpreferences.proto | 7 +- lib/auth/assist/assistv1/service.go | 27 +++++- lib/auth/auth.go | 8 +- lib/auth/init.go | 2 +- .../userpreferencesv1/service.go | 16 ++-- .../userpreferencesv1/service_test.go | 32 +------ lib/authz/permissions.go | 13 +++ lib/services/local/userpreferences.go | 12 +-- lib/services/local/userpreferences_test.go | 19 +--- lib/services/userpreferences.go | 8 ++ lib/web/userpreferences.go | 5 +- 12 files changed, 110 insertions(+), 135 deletions(-) diff --git a/api/gen/proto/go/userpreferences/v1/userpreferences.pb.go b/api/gen/proto/go/userpreferences/v1/userpreferences.pb.go index c7984c072d229..f470f0fdb08da 100644 --- a/api/gen/proto/go/userpreferences/v1/userpreferences.pb.go +++ b/api/gen/proto/go/userpreferences/v1/userpreferences.pb.go @@ -107,9 +107,6 @@ type GetUserPreferencesRequest struct { state protoimpl.MessageState sizeCache protoimpl.SizeCache unknownFields protoimpl.UnknownFields - - // username is the username of the owner of the user preferences to get. - Username string `protobuf:"bytes,1,opt,name=username,proto3" json:"username,omitempty"` } func (x *GetUserPreferencesRequest) Reset() { @@ -144,13 +141,6 @@ func (*GetUserPreferencesRequest) Descriptor() ([]byte, []int) { return file_teleport_userpreferences_v1_userpreferences_proto_rawDescGZIP(), []int{1} } -func (x *GetUserPreferencesRequest) GetUsername() string { - if x != nil { - return x.Username - } - return "" -} - // GetUserPreferencesResponse is a response to get the user preferences. type GetUserPreferencesResponse struct { state protoimpl.MessageState @@ -208,8 +198,6 @@ type UpsertUserPreferencesRequest struct { // preferences is the new user preferences to set. Preferences *UserPreferences `protobuf:"bytes,1,opt,name=preferences,proto3" json:"preferences,omitempty"` - // username is the username of the owner of the user preferences to update. - Username string `protobuf:"bytes,2,opt,name=username,proto3" json:"username,omitempty"` } func (x *UpsertUserPreferencesRequest) Reset() { @@ -251,13 +239,6 @@ func (x *UpsertUserPreferencesRequest) GetPreferences() *UserPreferences { return nil } -func (x *UpsertUserPreferencesRequest) GetUsername() string { - if x != nil { - return x.Username - } - return "" -} - var File_teleport_userpreferences_v1_userpreferences_proto protoreflect.FileDescriptor var file_teleport_userpreferences_v1_userpreferences_proto_rawDesc = []byte{ @@ -290,49 +271,46 @@ var file_teleport_userpreferences_v1_userpreferences_proto_rawDesc = []byte{ 0x74, 0x2e, 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x2e, 0x76, 0x31, 0x2e, 0x4f, 0x6e, 0x62, 0x6f, 0x61, 0x72, 0x64, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x07, 0x6f, 0x6e, 0x62, - 0x6f, 0x61, 0x72, 0x64, 0x22, 0x37, 0x0a, 0x19, 0x47, 0x65, 0x74, 0x55, 0x73, 0x65, 0x72, 0x50, + 0x6f, 0x61, 0x72, 0x64, 0x22, 0x21, 0x0a, 0x19, 0x47, 0x65, 0x74, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, - 0x74, 0x12, 0x1a, 0x0a, 0x08, 0x75, 0x73, 0x65, 0x72, 0x6e, 0x61, 0x6d, 0x65, 0x18, 0x01, 0x20, - 0x01, 0x28, 0x09, 0x52, 0x08, 0x75, 0x73, 0x65, 0x72, 0x6e, 0x61, 0x6d, 0x65, 0x22, 0x6c, 0x0a, - 0x1a, 0x47, 0x65, 0x74, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, - 0x63, 0x65, 0x73, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x12, 0x4e, 0x0a, 0x0b, 0x70, - 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x18, 0x01, 0x20, 0x01, 0x28, 0x0b, - 0x32, 0x2c, 0x2e, 0x74, 0x65, 0x6c, 0x65, 0x70, 0x6f, 0x72, 0x74, 0x2e, 0x75, 0x73, 0x65, 0x72, - 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x2e, 0x76, 0x31, 0x2e, 0x55, - 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x0b, - 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x22, 0x8a, 0x01, 0x0a, 0x1c, - 0x55, 0x70, 0x73, 0x65, 0x72, 0x74, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, - 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x12, 0x4e, 0x0a, 0x0b, - 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x18, 0x01, 0x20, 0x01, 0x28, - 0x0b, 0x32, 0x2c, 0x2e, 0x74, 0x65, 0x6c, 0x65, 0x70, 0x6f, 0x72, 0x74, 0x2e, 0x75, 0x73, 0x65, - 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x2e, 0x76, 0x31, 0x2e, - 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, - 0x0b, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x12, 0x1a, 0x0a, 0x08, - 0x75, 0x73, 0x65, 0x72, 0x6e, 0x61, 0x6d, 0x65, 0x18, 0x02, 0x20, 0x01, 0x28, 0x09, 0x52, 0x08, - 0x75, 0x73, 0x65, 0x72, 0x6e, 0x61, 0x6d, 0x65, 0x32, 0x8c, 0x02, 0x0a, 0x16, 0x55, 0x73, 0x65, - 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x53, 0x65, 0x72, 0x76, - 0x69, 0x63, 0x65, 0x12, 0x85, 0x01, 0x0a, 0x12, 0x47, 0x65, 0x74, 0x55, 0x73, 0x65, 0x72, 0x50, - 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x12, 0x36, 0x2e, 0x74, 0x65, 0x6c, + 0x74, 0x4a, 0x04, 0x08, 0x01, 0x10, 0x02, 0x22, 0x6c, 0x0a, 0x1a, 0x47, 0x65, 0x74, 0x55, 0x73, + 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x65, 0x73, + 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x12, 0x4e, 0x0a, 0x0b, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, + 0x6e, 0x63, 0x65, 0x73, 0x18, 0x01, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x2c, 0x2e, 0x74, 0x65, 0x6c, 0x65, 0x70, 0x6f, 0x72, 0x74, 0x2e, 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, - 0x65, 0x6e, 0x63, 0x65, 0x73, 0x2e, 0x76, 0x31, 0x2e, 0x47, 0x65, 0x74, 0x55, 0x73, 0x65, 0x72, + 0x65, 0x6e, 0x63, 0x65, 0x73, 0x2e, 0x76, 0x31, 0x2e, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, + 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x0b, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, + 0x65, 0x6e, 0x63, 0x65, 0x73, 0x22, 0x74, 0x0a, 0x1c, 0x55, 0x70, 0x73, 0x65, 0x72, 0x74, 0x55, + 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x65, + 0x71, 0x75, 0x65, 0x73, 0x74, 0x12, 0x4e, 0x0a, 0x0b, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, + 0x6e, 0x63, 0x65, 0x73, 0x18, 0x01, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x2c, 0x2e, 0x74, 0x65, 0x6c, + 0x65, 0x70, 0x6f, 0x72, 0x74, 0x2e, 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, + 0x65, 0x6e, 0x63, 0x65, 0x73, 0x2e, 0x76, 0x31, 0x2e, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, + 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x0b, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, + 0x65, 0x6e, 0x63, 0x65, 0x73, 0x4a, 0x04, 0x08, 0x02, 0x10, 0x03, 0x32, 0x8c, 0x02, 0x0a, 0x16, + 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x53, + 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0x12, 0x85, 0x01, 0x0a, 0x12, 0x47, 0x65, 0x74, 0x55, 0x73, + 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x12, 0x36, 0x2e, + 0x74, 0x65, 0x6c, 0x65, 0x70, 0x6f, 0x72, 0x74, 0x2e, 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, + 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x2e, 0x76, 0x31, 0x2e, 0x47, 0x65, 0x74, 0x55, + 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x65, + 0x71, 0x75, 0x65, 0x73, 0x74, 0x1a, 0x37, 0x2e, 0x74, 0x65, 0x6c, 0x65, 0x70, 0x6f, 0x72, 0x74, + 0x2e, 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, + 0x2e, 0x76, 0x31, 0x2e, 0x47, 0x65, 0x74, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, + 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x12, 0x6a, + 0x0a, 0x15, 0x55, 0x70, 0x73, 0x65, 0x72, 0x74, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, + 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x12, 0x39, 0x2e, 0x74, 0x65, 0x6c, 0x65, 0x70, 0x6f, + 0x72, 0x74, 0x2e, 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, + 0x65, 0x73, 0x2e, 0x76, 0x31, 0x2e, 0x55, 0x70, 0x73, 0x65, 0x72, 0x74, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x65, 0x71, 0x75, 0x65, - 0x73, 0x74, 0x1a, 0x37, 0x2e, 0x74, 0x65, 0x6c, 0x65, 0x70, 0x6f, 0x72, 0x74, 0x2e, 0x75, 0x73, - 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x2e, 0x76, 0x31, - 0x2e, 0x47, 0x65, 0x74, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, - 0x63, 0x65, 0x73, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x12, 0x6a, 0x0a, 0x15, 0x55, - 0x70, 0x73, 0x65, 0x72, 0x74, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, - 0x6e, 0x63, 0x65, 0x73, 0x12, 0x39, 0x2e, 0x74, 0x65, 0x6c, 0x65, 0x70, 0x6f, 0x72, 0x74, 0x2e, - 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x2e, - 0x76, 0x31, 0x2e, 0x55, 0x70, 0x73, 0x65, 0x72, 0x74, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, - 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x1a, - 0x16, 0x2e, 0x67, 0x6f, 0x6f, 0x67, 0x6c, 0x65, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x62, 0x75, - 0x66, 0x2e, 0x45, 0x6d, 0x70, 0x74, 0x79, 0x42, 0x59, 0x5a, 0x57, 0x67, 0x69, 0x74, 0x68, 0x75, - 0x62, 0x2e, 0x63, 0x6f, 0x6d, 0x2f, 0x67, 0x72, 0x61, 0x76, 0x69, 0x74, 0x61, 0x74, 0x69, 0x6f, - 0x6e, 0x61, 0x6c, 0x2f, 0x74, 0x65, 0x6c, 0x65, 0x70, 0x6f, 0x72, 0x74, 0x2f, 0x61, 0x70, 0x69, - 0x2f, 0x67, 0x65, 0x6e, 0x2f, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x2f, 0x67, 0x6f, 0x2f, 0x75, 0x73, - 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x2f, 0x76, 0x31, - 0x3b, 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, - 0x76, 0x31, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, + 0x73, 0x74, 0x1a, 0x16, 0x2e, 0x67, 0x6f, 0x6f, 0x67, 0x6c, 0x65, 0x2e, 0x70, 0x72, 0x6f, 0x74, + 0x6f, 0x62, 0x75, 0x66, 0x2e, 0x45, 0x6d, 0x70, 0x74, 0x79, 0x42, 0x59, 0x5a, 0x57, 0x67, 0x69, + 0x74, 0x68, 0x75, 0x62, 0x2e, 0x63, 0x6f, 0x6d, 0x2f, 0x67, 0x72, 0x61, 0x76, 0x69, 0x74, 0x61, + 0x74, 0x69, 0x6f, 0x6e, 0x61, 0x6c, 0x2f, 0x74, 0x65, 0x6c, 0x65, 0x70, 0x6f, 0x72, 0x74, 0x2f, + 0x61, 0x70, 0x69, 0x2f, 0x67, 0x65, 0x6e, 0x2f, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x2f, 0x67, 0x6f, + 0x2f, 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, + 0x2f, 0x76, 0x31, 0x3b, 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, + 0x63, 0x65, 0x73, 0x76, 0x31, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, } var ( diff --git a/api/proto/teleport/userpreferences/v1/userpreferences.proto b/api/proto/teleport/userpreferences/v1/userpreferences.proto index 1d142c9632ee1..f13808a90ca90 100644 --- a/api/proto/teleport/userpreferences/v1/userpreferences.proto +++ b/api/proto/teleport/userpreferences/v1/userpreferences.proto @@ -35,8 +35,7 @@ message UserPreferences { // GetUserPreferencesRequest is a request to get the user preferences. message GetUserPreferencesRequest { - // username is the username of the owner of the user preferences to get. - string username = 1; + reserved 1; } // GetUserPreferencesResponse is a response to get the user preferences. @@ -49,8 +48,8 @@ message GetUserPreferencesResponse { message UpsertUserPreferencesRequest { // preferences is the new user preferences to set. UserPreferences preferences = 1; - // username is the username of the owner of the user preferences to update. - string username = 2; + + reserved 2; } // UserPreferencesService is a service that stores user settings. diff --git a/lib/auth/assist/assistv1/service.go b/lib/auth/assist/assistv1/service.go index 8f721d91e423f..2e549b31459f0 100644 --- a/lib/auth/assist/assistv1/service.go +++ b/lib/auth/assist/assistv1/service.go @@ -185,14 +185,33 @@ func (a *Service) CreateAssistantMessage(ctx context.Context, req *assist.Create // IsAssistEnabled returns true if the assist is enabled or not on the auth level. func (a *Service) IsAssistEnabled(ctx context.Context, _ *assist.IsAssistEnabledRequest) (*assist.IsAssistEnabledResponse, error) { - _, err := authz.AuthorizeWithVerbs(ctx, a.log, a.authorizer, true, types.KindAssistant, types.VerbRead) + // If the embedder is not configured, the assist is not enabled as we cannot compute embeddings. + if a.embedder == nil { + return &assist.IsAssistEnabledResponse{Enabled: false}, nil + } + + authCtx, err := a.authorizer.Authorize(ctx) if err != nil { return nil, trace.Wrap(err) } - if a.embedder == nil { - // If the embedder is not configured, the assist is not enabled as we cannot compute embeddings. - return &assist.IsAssistEnabledResponse{Enabled: false}, nil + // Check if this endpoint is called by a user or Proxy. + if _, ok := authCtx.UnmappedIdentity.(authz.LocalUser); ok { + checkErr := authCtx.Checker.CheckAccessToRule( + &services.Context{User: authCtx.User}, + defaults.Namespace, types.KindAssistant, types.VerbRead, + false, /* silent */ + ) + if checkErr != nil { + return nil, authz.ConvertAuthorizerError(ctx, a.log, err) + } + } else { + // This endpoint is called from Proxy to check if the assist is enabled. + // Proxy credentials are used instead of the user credentials. + requestedByProxy := authz.HasBuiltinRole(*authCtx, string(types.RoleProxy)) + if !requestedByProxy { + return nil, trace.AccessDenied("only proxy is allowed to call IsAssistEnabled endpoint") + } } // Check if assist can use the backend. diff --git a/lib/auth/auth.go b/lib/auth/auth.go index 3280c682895f4..288b7fb213b44 100644 --- a/lib/auth/auth.go +++ b/lib/auth/auth.go @@ -300,7 +300,7 @@ func NewServer(cfg *InitConfig, opts ...ServerOption) (*Server, error) { StatusInternal: cfg.Status, UsageReporter: cfg.UsageReporter, Assistant: cfg.Assist, - UserPreferences: cfg.UserPreferences, + UserPreferencesBackend: cfg.UserPreferences, } closeCtx, cancelFunc := context.WithCancel(context.TODO()) @@ -416,7 +416,7 @@ type Services struct { services.AccessLists services.Assistant services.Embeddings - services.UserPreferences + services.UserPreferencesBackend usagereporter.UsageReporter types.Events events.AuditLogSessionStreamer @@ -5372,13 +5372,13 @@ func (a *Server) CompareAndSwapHeadlessAuthentication(ctx context.Context, old, // GetUserPreferences returns the user preferences for a given user. func (a *Server) GetUserPreferences(ctx context.Context, request *userpreferencesv1.GetUserPreferencesRequest) (*userpreferencesv1.GetUserPreferencesResponse, error) { - resp, err := a.Services.GetUserPreferences(ctx, request) + resp, err := a.Services.GetUserPreferences(ctx, "", request) return resp, trace.Wrap(err) } // UpsertUserPreferences creates or updates user preferences for a given username. func (a *Server) UpsertUserPreferences(ctx context.Context, request *userpreferencesv1.UpsertUserPreferencesRequest) error { - return trace.Wrap(a.Services.UpsertUserPreferences(ctx, request)) + return trace.Wrap(a.Services.UpsertUserPreferences(ctx, "", request)) } // getProxyPublicAddr returns the first valid, non-empty proxy public address it diff --git a/lib/auth/init.go b/lib/auth/init.go index 0f66807ee0e5e..fea25e4121b39 100644 --- a/lib/auth/init.go +++ b/lib/auth/init.go @@ -152,7 +152,7 @@ type InitConfig struct { Assist services.Assistant // UserPreferences is a service that manages user preferences. - UserPreferences services.UserPreferences + UserPreferences services.UserPreferencesBackend // Roles is a set of roles to create Roles []types.Role diff --git a/lib/auth/userpreferences/userpreferencesv1/service.go b/lib/auth/userpreferences/userpreferencesv1/service.go index f99cc9b025f24..9495d5984eaf4 100644 --- a/lib/auth/userpreferences/userpreferencesv1/service.go +++ b/lib/auth/userpreferences/userpreferencesv1/service.go @@ -32,7 +32,7 @@ import ( // ServiceConfig holds configuration options for the user preferences service. type ServiceConfig struct { - Backend services.UserPreferences + Backend services.UserPreferencesBackend Authorizer authz.Authorizer Logger *logrus.Entry } @@ -41,7 +41,7 @@ type ServiceConfig struct { type Service struct { userpreferences.UnimplementedUserPreferencesServiceServer - backend services.UserPreferences + backend services.UserPreferencesBackend authorizer authz.Authorizer log *logrus.Entry } @@ -71,11 +71,9 @@ func (a *Service) GetUserPreferences(ctx context.Context, req *userpreferences.G return nil, trace.Wrap(err) } - if authCtx.User.GetName() != req.GetUsername() { - return nil, trace.AccessDenied("user %q is not allowed to access preferences for user %q", authCtx.User.GetName(), req.Username) - } + username := authCtx.User.GetName() - return a.backend.GetUserPreferences(ctx, req) + return a.backend.GetUserPreferences(ctx, username, req) } // UpsertUserPreferences creates or updates user preferences for a given username. @@ -85,9 +83,7 @@ func (a *Service) UpsertUserPreferences(ctx context.Context, req *userpreference return nil, trace.Wrap(err) } - if authCtx.User.GetName() != req.GetUsername() { - return nil, trace.AccessDenied("user %q is not allowed to access preferences for user %q", authCtx.User.GetName(), req.Username) - } + username := authCtx.User.GetName() - return &emptypb.Empty{}, trace.Wrap(a.backend.UpsertUserPreferences(ctx, req)) + return &emptypb.Empty{}, trace.Wrap(a.backend.UpsertUserPreferences(ctx, username, req)) } diff --git a/lib/auth/userpreferences/userpreferencesv1/service_test.go b/lib/auth/userpreferences/userpreferencesv1/service_test.go index e553ad244f69d..4503d00359813 100644 --- a/lib/auth/userpreferences/userpreferencesv1/service_test.go +++ b/lib/auth/userpreferences/userpreferencesv1/service_test.go @@ -50,9 +50,7 @@ func TestService_GetUserPreferences(t *testing.T) { { name: "success", userName: defaultUser, - req: &userpreferencesv1.GetUserPreferencesRequest{ - Username: defaultUser, - }, + req: &userpreferencesv1.GetUserPreferencesRequest{}, want: &userpreferencesv1.GetUserPreferencesResponse{ Preferences: &userpreferencesv1.UserPreferences{ Assist: &userpreferencesv1.AssistUserPreferences{ @@ -67,23 +65,12 @@ func TestService_GetUserPreferences(t *testing.T) { }, wantErr: assert.NoError, }, - { - name: "access denied - incorrect user", - userName: defaultUser, - req: &userpreferencesv1.GetUserPreferencesRequest{ - Username: "non-existent-user", - }, - want: nil, - wantErr: assert.Error, - }, { name: "access denied - RBAC rule", userName: noAccessUser, - req: &userpreferencesv1.GetUserPreferencesRequest{ - Username: "test-user-no-access", - }, - want: nil, - wantErr: assert.Error, + req: &userpreferencesv1.GetUserPreferencesRequest{}, + want: nil, + wantErr: assert.Error, }, } @@ -125,25 +112,14 @@ func TestService_UpsertUserPreferences(t *testing.T) { name: "success", userName: defaultUser, req: &userpreferencesv1.UpsertUserPreferencesRequest{ - Username: defaultUser, Preferences: defaultPreferences, }, wantErr: assert.NoError, }, - { - name: "access denied - incorrect user", - userName: defaultUser, - req: &userpreferencesv1.UpsertUserPreferencesRequest{ - Username: "non-existent-user", - Preferences: defaultPreferences, - }, - wantErr: assert.Error, - }, { name: "access denied - RBAC rule", userName: noAccessUser, req: &userpreferencesv1.UpsertUserPreferencesRequest{ - Username: noAccessUser, Preferences: defaultPreferences, }, wantErr: assert.Error, diff --git a/lib/authz/permissions.go b/lib/authz/permissions.go index 2c398dafd733c..def3ce0fd14de 100644 --- a/lib/authz/permissions.go +++ b/lib/authz/permissions.go @@ -1355,3 +1355,16 @@ func UserFromContext(ctx context.Context) (IdentityGetter, error) { } return user, nil } + +// HasBuiltinRole checks if the identity is a builtin role with the matching +// name. +func HasBuiltinRole(authContext Context, name string) bool { + if _, ok := authContext.Identity.(BuiltinRole); !ok { + return false + } + if !authContext.Checker.HasRole(name) { + return false + } + + return true +} diff --git a/lib/services/local/userpreferences.go b/lib/services/local/userpreferences.go index f237c35787790..453bb3316291a 100644 --- a/lib/services/local/userpreferences.go +++ b/lib/services/local/userpreferences.go @@ -55,8 +55,8 @@ func NewUserPreferencesService(backend backend.Backend) *UserPreferencesService } // GetUserPreferences returns the user preferences for the given user. -func (u *UserPreferencesService) GetUserPreferences(ctx context.Context, req *userpreferencesv1.GetUserPreferencesRequest) (*userpreferencesv1.GetUserPreferencesResponse, error) { - preferences, err := u.getUserPreferences(ctx, req.Username) +func (u *UserPreferencesService) GetUserPreferences(ctx context.Context, username string, _ *userpreferencesv1.GetUserPreferencesRequest) (*userpreferencesv1.GetUserPreferencesResponse, error) { + preferences, err := u.getUserPreferences(ctx, username) if err != nil { if trace.IsNotFound(err) { return &userpreferencesv1.GetUserPreferencesResponse{Preferences: DefaultUserPreferences()}, nil @@ -69,15 +69,15 @@ func (u *UserPreferencesService) GetUserPreferences(ctx context.Context, req *us } // UpsertUserPreferences creates or updates user preferences for a given username. -func (u *UserPreferencesService) UpsertUserPreferences(ctx context.Context, req *userpreferencesv1.UpsertUserPreferencesRequest) error { - if req.Username == "" { +func (u *UserPreferencesService) UpsertUserPreferences(ctx context.Context, username string, req *userpreferencesv1.UpsertUserPreferencesRequest) error { + if username == "" { return trace.BadParameter("missing username") } if err := validatePreferences(req.Preferences); err != nil { return trace.Wrap(err) } - preferences, err := u.getUserPreferences(ctx, req.Username) + preferences, err := u.getUserPreferences(ctx, username) if err != nil { if !trace.IsNotFound(err) { return trace.Wrap(err) @@ -89,7 +89,7 @@ func (u *UserPreferencesService) UpsertUserPreferences(ctx context.Context, req return trace.Wrap(err) } - item, err := createBackendItem(req.Username, preferences) + item, err := createBackendItem(username, preferences) if err != nil { return trace.Wrap(err) } diff --git a/lib/services/local/userpreferences_test.go b/lib/services/local/userpreferences_test.go index 0bcf015c773a4..9c84cd72129eb 100644 --- a/lib/services/local/userpreferences_test.go +++ b/lib/services/local/userpreferences_test.go @@ -63,7 +63,6 @@ func TestUserPreferencesCRUD2(t *testing.T) { { name: "update the theme preference only", req: &userpreferencesv1.UpsertUserPreferencesRequest{ - Username: username, Preferences: &userpreferencesv1.UserPreferences{ Theme: userpreferencesv1.Theme_THEME_DARK, }, @@ -77,7 +76,6 @@ func TestUserPreferencesCRUD2(t *testing.T) { { name: "update the assist preferred logins only", req: &userpreferencesv1.UpsertUserPreferencesRequest{ - Username: username, Preferences: &userpreferencesv1.UserPreferences{ Assist: &userpreferencesv1.AssistUserPreferences{ PreferredLogins: []string{"foo", "bar"}, @@ -99,7 +97,6 @@ func TestUserPreferencesCRUD2(t *testing.T) { { name: "update the assist view mode only", req: &userpreferencesv1.UpsertUserPreferencesRequest{ - Username: username, Preferences: &userpreferencesv1.UserPreferences{ Assist: &userpreferencesv1.AssistUserPreferences{ ViewMode: userpreferencesv1.AssistViewMode_ASSIST_VIEW_MODE_POPUP_EXPANDED_SIDEBAR_VISIBLE, @@ -118,7 +115,6 @@ func TestUserPreferencesCRUD2(t *testing.T) { { name: "update the onboard preference only", req: &userpreferencesv1.UpsertUserPreferencesRequest{ - Username: username, Preferences: &userpreferencesv1.UserPreferences{ Onboard: &userpreferencesv1.OnboardUserPreferences{ PreferredResources: []userpreferencesv1.Resource{userpreferencesv1.Resource_RESOURCE_DATABASES}, @@ -136,7 +132,6 @@ func TestUserPreferencesCRUD2(t *testing.T) { { name: "update all the settings at once", req: &userpreferencesv1.UpsertUserPreferencesRequest{ - Username: username, Preferences: &userpreferencesv1.UserPreferences{ Theme: userpreferencesv1.Theme_THEME_LIGHT, Assist: &userpreferencesv1.AssistUserPreferences{ @@ -168,21 +163,17 @@ func TestUserPreferencesCRUD2(t *testing.T) { identity := newUserPreferencesService(t) - res, err := identity.GetUserPreferences(ctx, &userpreferencesv1.GetUserPreferencesRequest{ - Username: username, - }) + res, err := identity.GetUserPreferences(ctx, username, &userpreferencesv1.GetUserPreferencesRequest{}) require.NoError(t, err) // Clone the proto as the accessing fields for some reason modifies the state. require.Empty(t, cmp.Diff(defaultPref, proto.Clone(res.Preferences), protocmp.Transform())) if test.req != nil { - err := identity.UpsertUserPreferences(ctx, test.req) + err := identity.UpsertUserPreferences(ctx, username, test.req) require.NoError(t, err) } - res, err = identity.GetUserPreferences(ctx, &userpreferencesv1.GetUserPreferencesRequest{ - Username: username, - }) + res, err = identity.GetUserPreferences(ctx, username, &userpreferencesv1.GetUserPreferencesRequest{}) require.NoError(t, err) require.Empty(t, cmp.Diff(test.expected, res.Preferences, protocmp.Transform())) @@ -213,9 +204,7 @@ func TestLayoutUpdate(t *testing.T) { require.NoError(t, err) // Get the preferences and ensure that the layout is updated. - prefs, err := identity.GetUserPreferences(ctx, &userpreferencesv1.GetUserPreferencesRequest{ - Username: "test", - }) + prefs, err := identity.GetUserPreferences(ctx, "test", &userpreferencesv1.GetUserPreferencesRequest{}) require.NoError(t, err) // The layout should be updated to the latest version (values should not be nil). require.NotNil(t, prefs.Preferences.Onboard) diff --git a/lib/services/userpreferences.go b/lib/services/userpreferences.go index fcc683e7f3128..da6832dad8f5c 100644 --- a/lib/services/userpreferences.go +++ b/lib/services/userpreferences.go @@ -31,3 +31,11 @@ type UserPreferences interface { // UpsertUserPreferences creates or updates user preferences for a given username. UpsertUserPreferences(ctx context.Context, req *userpreferencesv1.UpsertUserPreferencesRequest) error } + +// UserPreferencesBackend is the database interface for managing user preferences. +type UserPreferencesBackend interface { + // GetUserPreferences returns the user preferences for a given user. + GetUserPreferences(ctx context.Context, username string, req *userpreferencesv1.GetUserPreferencesRequest) (*userpreferencesv1.GetUserPreferencesResponse, error) + // UpsertUserPreferences creates or updates user preferences for a given username. + UpsertUserPreferences(ctx context.Context, username string, req *userpreferencesv1.UpsertUserPreferencesRequest) error +} diff --git a/lib/web/userpreferences.go b/lib/web/userpreferences.go index f5ae62ffe879f..0c247d4862e0a 100644 --- a/lib/web/userpreferences.go +++ b/lib/web/userpreferences.go @@ -50,9 +50,7 @@ func (h *Handler) getUserPreferences(_ http.ResponseWriter, r *http.Request, p h return nil, trace.Wrap(err) } - resp, err := authClient.GetUserPreferences(r.Context(), &userpreferencesv1.GetUserPreferencesRequest{ - Username: sctx.GetUser(), - }) + resp, err := authClient.GetUserPreferences(r.Context(), &userpreferencesv1.GetUserPreferencesRequest{}) if err != nil { return nil, trace.Wrap(err) } @@ -74,7 +72,6 @@ func (h *Handler) updateUserPreferences(_ http.ResponseWriter, r *http.Request, } preferences := &userpreferencesv1.UpsertUserPreferencesRequest{ - Username: sctx.GetUser(), Preferences: &userpreferencesv1.UserPreferences{ Theme: req.Theme, Assist: &userpreferencesv1.AssistUserPreferences{ From f9131ccc18c7898aa6a8459b66d20d0e5dd6fc5d Mon Sep 17 00:00:00 2001 From: Jakub Nyckowski Date: Mon, 24 Jul 2023 17:17:57 -0400 Subject: [PATCH 04/13] Refactor to use authz.HasBuiltinRole Refactored code in the 'auth_with_roles.go' file to use 'authz.HasBuiltinRole' instead of 'HasBuiltinRole'. This change is in line with recommended practices for deprecation and makes the code more standard and easier to manage. The original 'HasBuiltinRole' function is marked as deprecated and will be removed in future once 'teleport.e' is updated to use 'authz.HasBuiltinRole'. --- lib/auth/auth_with_roles.go | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go index f6d939b8e380a..a5baad0d49020 100644 --- a/lib/auth/auth_with_roles.go +++ b/lib/auth/auth_with_roles.go @@ -236,7 +236,7 @@ func (a *ServerWithRoles) isLocalOrRemoteServerAction() bool { // whether any of the given roles match the role set. func (a *ServerWithRoles) hasBuiltinRole(roles ...types.SystemRole) bool { for _, role := range roles { - if HasBuiltinRole(a.context, string(role)) { + if authz.HasBuiltinRole(a.context, string(role)) { return true } } @@ -245,15 +245,11 @@ func (a *ServerWithRoles) hasBuiltinRole(roles ...types.SystemRole) bool { // HasBuiltinRole checks if the identity is a builtin role with the matching // name. +// Deprecated: use authz.HasBuiltinRole instead. func HasBuiltinRole(authContext authz.Context, name string) bool { - if _, ok := authContext.Identity.(authz.BuiltinRole); !ok { - return false - } - if !authContext.Checker.HasRole(name) { - return false - } - - return true + // TODO(jakule): This function can be removed once teleport.e is updated + // to use authz.HasBuiltinRole. + return authz.HasBuiltinRole(authContext, name) } // HasRemoteBuiltinRole checks if the identity is a remote builtin role with the @@ -5508,8 +5504,8 @@ func (a *ServerWithRoles) checkAccessToNode(node types.Server) error { // In addition, allow proxy (and remote proxy) to access all nodes for its // smart resolution address resolution. Once the smart resolution logic is // moved to the auth server, this logic can be removed. - builtinRole := HasBuiltinRole(a.context, string(types.RoleAdmin)) || - HasBuiltinRole(a.context, string(types.RoleProxy)) || + builtinRole := authz.HasBuiltinRole(a.context, string(types.RoleAdmin)) || + authz.HasBuiltinRole(a.context, string(types.RoleProxy)) || HasRemoteBuiltinRole(a.context, string(types.RoleRemoteProxy)) if builtinRole { From c257fb380f0fee877cdf902bfcd6edb75aa3bbbf Mon Sep 17 00:00:00 2001 From: Jakub Nyckowski Date: Mon, 24 Jul 2023 17:21:52 -0400 Subject: [PATCH 05/13] Reserve removed username again? --- .../userpreferences/v1/userpreferences.pb.go | 80 ++++++++++--------- .../userpreferences/v1/userpreferences.proto | 2 + 2 files changed, 43 insertions(+), 39 deletions(-) diff --git a/api/gen/proto/go/userpreferences/v1/userpreferences.pb.go b/api/gen/proto/go/userpreferences/v1/userpreferences.pb.go index f470f0fdb08da..85f08816262a8 100644 --- a/api/gen/proto/go/userpreferences/v1/userpreferences.pb.go +++ b/api/gen/proto/go/userpreferences/v1/userpreferences.pb.go @@ -271,46 +271,48 @@ var file_teleport_userpreferences_v1_userpreferences_proto_rawDesc = []byte{ 0x74, 0x2e, 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x2e, 0x76, 0x31, 0x2e, 0x4f, 0x6e, 0x62, 0x6f, 0x61, 0x72, 0x64, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x07, 0x6f, 0x6e, 0x62, - 0x6f, 0x61, 0x72, 0x64, 0x22, 0x21, 0x0a, 0x19, 0x47, 0x65, 0x74, 0x55, 0x73, 0x65, 0x72, 0x50, + 0x6f, 0x61, 0x72, 0x64, 0x22, 0x2b, 0x0a, 0x19, 0x47, 0x65, 0x74, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, - 0x74, 0x4a, 0x04, 0x08, 0x01, 0x10, 0x02, 0x22, 0x6c, 0x0a, 0x1a, 0x47, 0x65, 0x74, 0x55, 0x73, - 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x65, 0x73, - 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x12, 0x4e, 0x0a, 0x0b, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, - 0x6e, 0x63, 0x65, 0x73, 0x18, 0x01, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x2c, 0x2e, 0x74, 0x65, 0x6c, - 0x65, 0x70, 0x6f, 0x72, 0x74, 0x2e, 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, - 0x65, 0x6e, 0x63, 0x65, 0x73, 0x2e, 0x76, 0x31, 0x2e, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, - 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x0b, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, - 0x65, 0x6e, 0x63, 0x65, 0x73, 0x22, 0x74, 0x0a, 0x1c, 0x55, 0x70, 0x73, 0x65, 0x72, 0x74, 0x55, - 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x65, - 0x71, 0x75, 0x65, 0x73, 0x74, 0x12, 0x4e, 0x0a, 0x0b, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, - 0x6e, 0x63, 0x65, 0x73, 0x18, 0x01, 0x20, 0x01, 0x28, 0x0b, 0x32, 0x2c, 0x2e, 0x74, 0x65, 0x6c, - 0x65, 0x70, 0x6f, 0x72, 0x74, 0x2e, 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, - 0x65, 0x6e, 0x63, 0x65, 0x73, 0x2e, 0x76, 0x31, 0x2e, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, - 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x0b, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, - 0x65, 0x6e, 0x63, 0x65, 0x73, 0x4a, 0x04, 0x08, 0x02, 0x10, 0x03, 0x32, 0x8c, 0x02, 0x0a, 0x16, - 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x53, - 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0x12, 0x85, 0x01, 0x0a, 0x12, 0x47, 0x65, 0x74, 0x55, 0x73, - 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x12, 0x36, 0x2e, - 0x74, 0x65, 0x6c, 0x65, 0x70, 0x6f, 0x72, 0x74, 0x2e, 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, - 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x2e, 0x76, 0x31, 0x2e, 0x47, 0x65, 0x74, 0x55, - 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x65, - 0x71, 0x75, 0x65, 0x73, 0x74, 0x1a, 0x37, 0x2e, 0x74, 0x65, 0x6c, 0x65, 0x70, 0x6f, 0x72, 0x74, - 0x2e, 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, - 0x2e, 0x76, 0x31, 0x2e, 0x47, 0x65, 0x74, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, - 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x12, 0x6a, - 0x0a, 0x15, 0x55, 0x70, 0x73, 0x65, 0x72, 0x74, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, - 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x12, 0x39, 0x2e, 0x74, 0x65, 0x6c, 0x65, 0x70, 0x6f, - 0x72, 0x74, 0x2e, 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, - 0x65, 0x73, 0x2e, 0x76, 0x31, 0x2e, 0x55, 0x70, 0x73, 0x65, 0x72, 0x74, 0x55, 0x73, 0x65, 0x72, - 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x65, 0x71, 0x75, 0x65, - 0x73, 0x74, 0x1a, 0x16, 0x2e, 0x67, 0x6f, 0x6f, 0x67, 0x6c, 0x65, 0x2e, 0x70, 0x72, 0x6f, 0x74, - 0x6f, 0x62, 0x75, 0x66, 0x2e, 0x45, 0x6d, 0x70, 0x74, 0x79, 0x42, 0x59, 0x5a, 0x57, 0x67, 0x69, - 0x74, 0x68, 0x75, 0x62, 0x2e, 0x63, 0x6f, 0x6d, 0x2f, 0x67, 0x72, 0x61, 0x76, 0x69, 0x74, 0x61, - 0x74, 0x69, 0x6f, 0x6e, 0x61, 0x6c, 0x2f, 0x74, 0x65, 0x6c, 0x65, 0x70, 0x6f, 0x72, 0x74, 0x2f, - 0x61, 0x70, 0x69, 0x2f, 0x67, 0x65, 0x6e, 0x2f, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x2f, 0x67, 0x6f, - 0x2f, 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, - 0x2f, 0x76, 0x31, 0x3b, 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, - 0x63, 0x65, 0x73, 0x76, 0x31, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33, + 0x74, 0x4a, 0x04, 0x08, 0x01, 0x10, 0x02, 0x52, 0x08, 0x75, 0x73, 0x65, 0x72, 0x6e, 0x61, 0x6d, + 0x65, 0x22, 0x6c, 0x0a, 0x1a, 0x47, 0x65, 0x74, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, + 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x73, 0x65, 0x12, + 0x4e, 0x0a, 0x0b, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x18, 0x01, + 0x20, 0x01, 0x28, 0x0b, 0x32, 0x2c, 0x2e, 0x74, 0x65, 0x6c, 0x65, 0x70, 0x6f, 0x72, 0x74, 0x2e, + 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x2e, + 0x76, 0x31, 0x2e, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, + 0x65, 0x73, 0x52, 0x0b, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x22, + 0x7e, 0x0a, 0x1c, 0x55, 0x70, 0x73, 0x65, 0x72, 0x74, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, + 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x12, + 0x4e, 0x0a, 0x0b, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x18, 0x01, + 0x20, 0x01, 0x28, 0x0b, 0x32, 0x2c, 0x2e, 0x74, 0x65, 0x6c, 0x65, 0x70, 0x6f, 0x72, 0x74, 0x2e, + 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x2e, + 0x76, 0x31, 0x2e, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, + 0x65, 0x73, 0x52, 0x0b, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x4a, + 0x04, 0x08, 0x02, 0x10, 0x03, 0x52, 0x08, 0x75, 0x73, 0x65, 0x72, 0x6e, 0x61, 0x6d, 0x65, 0x32, + 0x8c, 0x02, 0x0a, 0x16, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, + 0x63, 0x65, 0x73, 0x53, 0x65, 0x72, 0x76, 0x69, 0x63, 0x65, 0x12, 0x85, 0x01, 0x0a, 0x12, 0x47, + 0x65, 0x74, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, + 0x73, 0x12, 0x36, 0x2e, 0x74, 0x65, 0x6c, 0x65, 0x70, 0x6f, 0x72, 0x74, 0x2e, 0x75, 0x73, 0x65, + 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x2e, 0x76, 0x31, 0x2e, + 0x47, 0x65, 0x74, 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, + 0x65, 0x73, 0x52, 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x1a, 0x37, 0x2e, 0x74, 0x65, 0x6c, 0x65, + 0x70, 0x6f, 0x72, 0x74, 0x2e, 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, + 0x6e, 0x63, 0x65, 0x73, 0x2e, 0x76, 0x31, 0x2e, 0x47, 0x65, 0x74, 0x55, 0x73, 0x65, 0x72, 0x50, + 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, 0x65, 0x73, 0x70, 0x6f, 0x6e, + 0x73, 0x65, 0x12, 0x6a, 0x0a, 0x15, 0x55, 0x70, 0x73, 0x65, 0x72, 0x74, 0x55, 0x73, 0x65, 0x72, + 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x12, 0x39, 0x2e, 0x74, 0x65, + 0x6c, 0x65, 0x70, 0x6f, 0x72, 0x74, 0x2e, 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, + 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x2e, 0x76, 0x31, 0x2e, 0x55, 0x70, 0x73, 0x65, 0x72, 0x74, + 0x55, 0x73, 0x65, 0x72, 0x50, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x52, + 0x65, 0x71, 0x75, 0x65, 0x73, 0x74, 0x1a, 0x16, 0x2e, 0x67, 0x6f, 0x6f, 0x67, 0x6c, 0x65, 0x2e, + 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x62, 0x75, 0x66, 0x2e, 0x45, 0x6d, 0x70, 0x74, 0x79, 0x42, 0x59, + 0x5a, 0x57, 0x67, 0x69, 0x74, 0x68, 0x75, 0x62, 0x2e, 0x63, 0x6f, 0x6d, 0x2f, 0x67, 0x72, 0x61, + 0x76, 0x69, 0x74, 0x61, 0x74, 0x69, 0x6f, 0x6e, 0x61, 0x6c, 0x2f, 0x74, 0x65, 0x6c, 0x65, 0x70, + 0x6f, 0x72, 0x74, 0x2f, 0x61, 0x70, 0x69, 0x2f, 0x67, 0x65, 0x6e, 0x2f, 0x70, 0x72, 0x6f, 0x74, + 0x6f, 0x2f, 0x67, 0x6f, 0x2f, 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, 0x65, 0x72, 0x65, + 0x6e, 0x63, 0x65, 0x73, 0x2f, 0x76, 0x31, 0x3b, 0x75, 0x73, 0x65, 0x72, 0x70, 0x72, 0x65, 0x66, + 0x65, 0x72, 0x65, 0x6e, 0x63, 0x65, 0x73, 0x76, 0x31, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, + 0x33, } var ( diff --git a/api/proto/teleport/userpreferences/v1/userpreferences.proto b/api/proto/teleport/userpreferences/v1/userpreferences.proto index f13808a90ca90..229a768f444c7 100644 --- a/api/proto/teleport/userpreferences/v1/userpreferences.proto +++ b/api/proto/teleport/userpreferences/v1/userpreferences.proto @@ -36,6 +36,7 @@ message UserPreferences { // GetUserPreferencesRequest is a request to get the user preferences. message GetUserPreferencesRequest { reserved 1; + reserved "username"; } // GetUserPreferencesResponse is a response to get the user preferences. @@ -50,6 +51,7 @@ message UpsertUserPreferencesRequest { UserPreferences preferences = 1; reserved 2; + reserved "username"; } // UserPreferencesService is a service that stores user settings. From 4f5ae322ff6c7624adc0f79f51e738f8cb81ae3c Mon Sep 17 00:00:00 2001 From: Jakub Nyckowski Date: Mon, 24 Jul 2023 17:38:21 -0400 Subject: [PATCH 06/13] Fix UT --- lib/web/assistant_test.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/web/assistant_test.go b/lib/web/assistant_test.go index cce3bc1fa3f0e..d6c82f1ee5e46 100644 --- a/lib/web/assistant_test.go +++ b/lib/web/assistant_test.go @@ -37,9 +37,11 @@ import ( "golang.org/x/time/rate" authproto "github.com/gravitational/teleport/api/client/proto" + "github.com/gravitational/teleport/api/types" aitest "github.com/gravitational/teleport/lib/ai/testutils" "github.com/gravitational/teleport/lib/assist" "github.com/gravitational/teleport/lib/client" + "github.com/gravitational/teleport/lib/services" ) func Test_runAssistant(t *testing.T) { @@ -239,7 +241,18 @@ func Test_runAssistError(t *testing.T) { s := newWebSuiteWithConfig(t, webSuiteConfig{OpenAIConfig: &openaiCfg}) ctx := context.Background() - authPack := s.authPack(t, "foo") + + assistRole, err := types.NewRole("assist-access", types.RoleSpecV6{ + Allow: types.RoleConditions{ + Rules: []types.Rule{ + types.NewRule(types.KindAssistant, services.RW()), + }, + }, + }) + require.NoError(t, err) + require.NoError(t, s.server.Auth().UpsertRole(s.ctx, assistRole)) + + authPack := s.authPack(t, "foo", assistRole.GetName()) // Create the conversation conversationID := s.makeAssistConversation(t, ctx, authPack) From 74d6489bf93b7f0b76e166f09c15b143220f11c7 Mon Sep 17 00:00:00 2001 From: Jakub Nyckowski Date: Mon, 24 Jul 2023 17:48:42 -0400 Subject: [PATCH 07/13] Add local user permissions checks in authz This commit introduces two new methods in permissions.go to check if a user is a local user, and if a given action is performed by a local user. These permission checks are then used to replace existing checks in service.go, when performing actions like creating conversation, updating, listing, etc. This simplifies checks and provides a more consolidated and unified method for verifying user actions. --- lib/auth/assist/assistv1/service.go | 12 ++++++------ lib/authz/permissions.go | 11 +++++++++++ 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/lib/auth/assist/assistv1/service.go b/lib/auth/assist/assistv1/service.go index 2e549b31459f0..4e995220767ba 100644 --- a/lib/auth/assist/assistv1/service.go +++ b/lib/auth/assist/assistv1/service.go @@ -98,7 +98,7 @@ func (a *Service) CreateAssistantConversation(ctx context.Context, req *assist.C return nil, trace.Wrap(err) } - if authCtx.User.GetName() != req.GetUsername() { + if !authz.IsLocalUserAction(*authCtx, req.GetUsername()) { return nil, trace.AccessDenied("user %q is not allowed to create conversation for user %q", authCtx.User.GetName(), req.Username) } @@ -113,7 +113,7 @@ func (a *Service) UpdateAssistantConversationInfo(ctx context.Context, req *assi return nil, trace.Wrap(err) } - if authCtx.User.GetName() != req.GetUsername() { + if !authz.IsLocalUserAction(*authCtx, req.GetUsername()) { return nil, trace.AccessDenied("user %q is not allowed to update conversation for user %q", authCtx.User.GetName(), req.Username) } @@ -132,7 +132,7 @@ func (a *Service) GetAssistantConversations(ctx context.Context, req *assist.Get return nil, trace.Wrap(err) } - if authCtx.User.GetName() != req.GetUsername() { + if !authz.IsLocalUserAction(*authCtx, req.GetUsername()) { return nil, trace.AccessDenied("user %q is not allowed to list conversations for user %q", authCtx.User.GetName(), req.GetUsername()) } @@ -147,7 +147,7 @@ func (a *Service) DeleteAssistantConversation(ctx context.Context, req *assist.D return nil, trace.Wrap(err) } - if authCtx.User.GetName() != req.GetUsername() { + if !authz.IsLocalUserAction(*authCtx, req.GetUsername()) { return nil, trace.AccessDenied("user %q is not allowed to delete conversation for user %q", authCtx.User.GetName(), req.GetUsername()) } @@ -161,7 +161,7 @@ func (a *Service) GetAssistantMessages(ctx context.Context, req *assist.GetAssis return nil, trace.Wrap(err) } - if authCtx.User.GetName() != req.GetUsername() { + if !authz.IsLocalUserAction(*authCtx, req.GetUsername()) { return nil, trace.AccessDenied("user %q is not allowed to get messages for user %q", authCtx.User.GetName(), req.GetUsername()) } @@ -176,7 +176,7 @@ func (a *Service) CreateAssistantMessage(ctx context.Context, req *assist.Create return nil, trace.Wrap(err) } - if authCtx.User.GetName() != req.GetUsername() { + if !authz.IsLocalUserAction(*authCtx, req.GetUsername()) { return nil, trace.AccessDenied("user %q is not allowed to create message for user %q", authCtx.User.GetName(), req.GetUsername()) } diff --git a/lib/authz/permissions.go b/lib/authz/permissions.go index def3ce0fd14de..5e5f1fcfa396c 100644 --- a/lib/authz/permissions.go +++ b/lib/authz/permissions.go @@ -1368,3 +1368,14 @@ func HasBuiltinRole(authContext Context, name string) bool { return true } + +// IsLocalUser checks if the identity is a local user. +func IsLocalUser(authContext Context) bool { + _, ok := authContext.Identity.(LocalUser) + return ok +} + +// IsLocalUserAction checks if the identity is a local user and the username +func IsLocalUserAction(authContext Context, username string) bool { + return IsLocalUser(authContext) && authContext.User.GetName() == username +} From 925c845c3e95882844b385cc889a46faec0c41f6 Mon Sep 17 00:00:00 2001 From: Jakub Nyckowski Date: Mon, 24 Jul 2023 18:25:27 -0400 Subject: [PATCH 08/13] Fix tests --- lib/web/assistant_test.go | 24 ++++++++++++++++++++-- lib/web/command_test.go | 42 +++++++++++++++++++++++++++++++++++---- 2 files changed, 60 insertions(+), 6 deletions(-) diff --git a/lib/web/assistant_test.go b/lib/web/assistant_test.go index d6c82f1ee5e46..55c7dd1b96122 100644 --- a/lib/web/assistant_test.go +++ b/lib/web/assistant_test.go @@ -152,8 +152,18 @@ func Test_runAssistant(t *testing.T) { tc.setup(t, s) } + assistRole, err := types.NewRole("assist-access", types.RoleSpecV6{ + Allow: types.RoleConditions{ + Rules: []types.Rule{ + types.NewRule(types.KindAssistant, services.RW()), + }, + }, + }) + require.NoError(t, err) + require.NoError(t, s.server.Auth().UpsertRole(s.ctx, assistRole)) + ctx := context.Background() - authPack := s.authPack(t, "foo") + authPack := s.authPack(t, "foo", assistRole.GetName()) // Create the conversation conversationID := s.makeAssistConversation(t, ctx, authPack) @@ -350,7 +360,17 @@ func Test_generateAssistantTitle(t *testing.T) { OpenAIConfig: &openaiCfg, }) - pack := s.authPack(t, "foo") + assistRole, err := types.NewRole("assist-access", types.RoleSpecV6{ + Allow: types.RoleConditions{ + Rules: []types.Rule{ + types.NewRule(types.KindAssistant, services.RW()), + }, + }, + }) + require.NoError(t, err) + require.NoError(t, s.server.Auth().UpsertRole(s.ctx, assistRole)) + + pack := s.authPack(t, "foo", assistRole.GetName()) // Real test: we craft a request asking for a summary endpoint := pack.clt.Endpoint("webapi", "assistant", "title", "summary") diff --git a/lib/web/command_test.go b/lib/web/command_test.go index c2eca4a0eeeda..2b3c7bc9ca24f 100644 --- a/lib/web/command_test.go +++ b/lib/web/command_test.go @@ -42,10 +42,12 @@ import ( "google.golang.org/protobuf/types/known/timestamppb" "github.com/gravitational/teleport/api/gen/proto/go/assist/v1" + "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/lib/ai/testutils" assistlib "github.com/gravitational/teleport/lib/assist" "github.com/gravitational/teleport/lib/auth" "github.com/gravitational/teleport/lib/client" + "github.com/gravitational/teleport/lib/services" "github.com/gravitational/teleport/lib/session" "github.com/gravitational/teleport/lib/utils" ) @@ -65,7 +67,17 @@ func TestExecuteCommand(t *testing.T) { OpenAIConfig: &openAIConfig, }) - ws, _, err := s.makeCommand(t, s.authPack(t, testUser), uuid.New()) + assistRole, err := types.NewRole("assist-access", types.RoleSpecV6{ + Allow: types.RoleConditions{ + Rules: []types.Rule{ + types.NewRule(types.KindAssistant, services.RW()), + }, + }, + }) + require.NoError(t, err) + require.NoError(t, s.server.Auth().UpsertRole(s.ctx, assistRole)) + + ws, _, err := s.makeCommand(t, s.authPack(t, testUser, assistRole.GetName()), uuid.New()) require.NoError(t, err) t.Cleanup(func() { require.NoError(t, ws.Close()) }) @@ -84,7 +96,18 @@ func TestExecuteCommandHistory(t *testing.T) { disableDiskBasedRecording: true, OpenAIConfig: &openAIConfig, }) - authPack := s.authPack(t, testUser) + + assistRole, err := types.NewRole("assist-access", types.RoleSpecV6{ + Allow: types.RoleConditions{ + Rules: []types.Rule{ + types.NewRule(types.KindAssistant, services.RW()), + }, + }, + }) + require.NoError(t, err) + require.NoError(t, s.server.Auth().UpsertRole(s.ctx, assistRole)) + + authPack := s.authPack(t, testUser, assistRole.GetName()) ctx := context.Background() clt, err := s.server.NewClient(auth.TestUser(testUser)) @@ -115,7 +138,7 @@ func TestExecuteCommandHistory(t *testing.T) { // Then command execution history is saved var messages *assist.GetAssistantMessagesResponse - // Command execution history is saved in asynchronusly, so we need to wait for it. + // Command execution history is saved in asynchronously, so we need to wait for it. require.Eventually(t, func() bool { messages, err = clt.GetAssistantMessages(ctx, &assist.GetAssistantMessagesRequest{ ConversationId: conversationID.String(), @@ -152,7 +175,18 @@ func TestExecuteCommandSummary(t *testing.T) { disableDiskBasedRecording: true, OpenAIConfig: &openAIConfig, }) - authPack := s.authPack(t, testUser) + + assistRole, err := types.NewRole("assist-access", types.RoleSpecV6{ + Allow: types.RoleConditions{ + Rules: []types.Rule{ + types.NewRule(types.KindAssistant, services.RW()), + }, + }, + }) + require.NoError(t, err) + require.NoError(t, s.server.Auth().UpsertRole(s.ctx, assistRole)) + + authPack := s.authPack(t, testUser, assistRole.GetName()) ctx := context.Background() clt, err := s.server.NewClient(auth.TestUser(testUser)) From 24e630fcdea9406a7b2657493e74856b4666d248 Mon Sep 17 00:00:00 2001 From: Jakub Nyckowski Date: Mon, 24 Jul 2023 19:44:52 -0400 Subject: [PATCH 09/13] Tweak RBAC --- lib/auth/assist/assistv1/service.go | 33 +++++++++++++++++------------ 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/lib/auth/assist/assistv1/service.go b/lib/auth/assist/assistv1/service.go index 4e995220767ba..aded1292b0ffc 100644 --- a/lib/auth/assist/assistv1/service.go +++ b/lib/auth/assist/assistv1/service.go @@ -95,10 +95,10 @@ func NewService(cfg *ServiceConfig) (*Service, error) { func (a *Service) CreateAssistantConversation(ctx context.Context, req *assist.CreateAssistantConversationRequest) (*assist.CreateAssistantConversationResponse, error) { authCtx, err := authz.AuthorizeWithVerbs(ctx, a.log, a.authorizer, true, types.KindAssistant, types.VerbCreate) if err != nil { - return nil, trace.Wrap(err) + return nil, authz.ConvertAuthorizerError(ctx, a.log, err) } - if !authz.IsLocalUserAction(*authCtx, req.GetUsername()) { + if userHasAccess(authCtx, req) { return nil, trace.AccessDenied("user %q is not allowed to create conversation for user %q", authCtx.User.GetName(), req.Username) } @@ -110,10 +110,10 @@ func (a *Service) CreateAssistantConversation(ctx context.Context, req *assist.C func (a *Service) UpdateAssistantConversationInfo(ctx context.Context, req *assist.UpdateAssistantConversationInfoRequest) (*emptypb.Empty, error) { authCtx, err := authz.AuthorizeWithVerbs(ctx, a.log, a.authorizer, true, types.KindAssistant, types.VerbUpdate) if err != nil { - return nil, trace.Wrap(err) + return nil, authz.ConvertAuthorizerError(ctx, a.log, err) } - if !authz.IsLocalUserAction(*authCtx, req.GetUsername()) { + if userHasAccess(authCtx, req) { return nil, trace.AccessDenied("user %q is not allowed to update conversation for user %q", authCtx.User.GetName(), req.Username) } @@ -129,10 +129,10 @@ func (a *Service) UpdateAssistantConversationInfo(ctx context.Context, req *assi func (a *Service) GetAssistantConversations(ctx context.Context, req *assist.GetAssistantConversationsRequest) (*assist.GetAssistantConversationsResponse, error) { authCtx, err := authz.AuthorizeWithVerbs(ctx, a.log, a.authorizer, true, types.KindAssistant, types.VerbRead, types.VerbList) if err != nil { - return nil, trace.Wrap(err) + return nil, authz.ConvertAuthorizerError(ctx, a.log, err) } - if !authz.IsLocalUserAction(*authCtx, req.GetUsername()) { + if userHasAccess(authCtx, req) { return nil, trace.AccessDenied("user %q is not allowed to list conversations for user %q", authCtx.User.GetName(), req.GetUsername()) } @@ -144,10 +144,10 @@ func (a *Service) GetAssistantConversations(ctx context.Context, req *assist.Get func (a *Service) DeleteAssistantConversation(ctx context.Context, req *assist.DeleteAssistantConversationRequest) (*emptypb.Empty, error) { authCtx, err := authz.AuthorizeWithVerbs(ctx, a.log, a.authorizer, true, types.KindAssistant, types.VerbDelete) if err != nil { - return nil, trace.Wrap(err) + return nil, authz.ConvertAuthorizerError(ctx, a.log, err) } - if !authz.IsLocalUserAction(*authCtx, req.GetUsername()) { + if userHasAccess(authCtx, req) { return nil, trace.AccessDenied("user %q is not allowed to delete conversation for user %q", authCtx.User.GetName(), req.GetUsername()) } @@ -158,10 +158,10 @@ func (a *Service) DeleteAssistantConversation(ctx context.Context, req *assist.D func (a *Service) GetAssistantMessages(ctx context.Context, req *assist.GetAssistantMessagesRequest) (*assist.GetAssistantMessagesResponse, error) { authCtx, err := authz.AuthorizeWithVerbs(ctx, a.log, a.authorizer, true, types.KindAssistant, types.VerbRead) if err != nil { - return nil, trace.Wrap(err) + return nil, authz.ConvertAuthorizerError(ctx, a.log, err) } - if !authz.IsLocalUserAction(*authCtx, req.GetUsername()) { + if userHasAccess(authCtx, req) { return nil, trace.AccessDenied("user %q is not allowed to get messages for user %q", authCtx.User.GetName(), req.GetUsername()) } @@ -173,10 +173,10 @@ func (a *Service) GetAssistantMessages(ctx context.Context, req *assist.GetAssis func (a *Service) CreateAssistantMessage(ctx context.Context, req *assist.CreateAssistantMessageRequest) (*emptypb.Empty, error) { authCtx, err := authz.AuthorizeWithVerbs(ctx, a.log, a.authorizer, true, types.KindAssistant, types.VerbCreate) if err != nil { - return nil, trace.Wrap(err) + return nil, authz.ConvertAuthorizerError(ctx, a.log, err) } - if !authz.IsLocalUserAction(*authCtx, req.GetUsername()) { + if userHasAccess(authCtx, req) { return nil, trace.AccessDenied("user %q is not allowed to create message for user %q", authCtx.User.GetName(), req.GetUsername()) } @@ -192,7 +192,7 @@ func (a *Service) IsAssistEnabled(ctx context.Context, _ *assist.IsAssistEnabled authCtx, err := a.authorizer.Authorize(ctx) if err != nil { - return nil, trace.Wrap(err) + return nil, authz.ConvertAuthorizerError(ctx, a.log, err) } // Check if this endpoint is called by a user or Proxy. @@ -222,7 +222,7 @@ func (a *Service) GetAssistantEmbeddings(ctx context.Context, msg *assist.GetAss // TODO(jakule): The kind needs to be updated when we add more resources. authCtx, err := authz.AuthorizeWithVerbs(ctx, a.log, a.authorizer, true, types.KindNode, types.VerbRead, types.VerbList) if err != nil { - return nil, trace.Wrap(err) + return nil, authz.ConvertAuthorizerError(ctx, a.log, err) } if a.embedder == nil { @@ -272,3 +272,8 @@ func (a *Service) GetAssistantEmbeddings(ctx context.Context, msg *assist.GetAss Embeddings: protoDocs, }, nil } + +// userHasAccess returns true if the user should have access to the resource. +func userHasAccess(authCtx *authz.Context, req interface{ GetUsername() string }) bool { + return !authz.IsLocalUserAction(*authCtx, req.GetUsername()) && !authz.HasBuiltinRole(*authCtx, string(types.RoleAdmin)) +} From cb70cada3dd2e308df34d23569a4d224833267c3 Mon Sep 17 00:00:00 2001 From: Jakub Nyckowski Date: Wed, 26 Jul 2023 23:14:13 -0400 Subject: [PATCH 10/13] Address review comments --- lib/auth/assist/assistv1/service.go | 2 +- .../userpreferencesv1/service.go | 12 +++++++++--- .../userpreferencesv1/service_test.go | 18 ++++++------------ lib/authz/permissions.go | 4 ++-- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/lib/auth/assist/assistv1/service.go b/lib/auth/assist/assistv1/service.go index aded1292b0ffc..e72b85201c06c 100644 --- a/lib/auth/assist/assistv1/service.go +++ b/lib/auth/assist/assistv1/service.go @@ -275,5 +275,5 @@ func (a *Service) GetAssistantEmbeddings(ctx context.Context, msg *assist.GetAss // userHasAccess returns true if the user should have access to the resource. func userHasAccess(authCtx *authz.Context, req interface{ GetUsername() string }) bool { - return !authz.IsLocalUserAction(*authCtx, req.GetUsername()) && !authz.HasBuiltinRole(*authCtx, string(types.RoleAdmin)) + return !authz.IsCurrentUser(*authCtx, req.GetUsername()) && !authz.HasBuiltinRole(*authCtx, string(types.RoleAdmin)) } diff --git a/lib/auth/userpreferences/userpreferencesv1/service.go b/lib/auth/userpreferences/userpreferencesv1/service.go index 9495d5984eaf4..2ab12328c4e14 100644 --- a/lib/auth/userpreferences/userpreferencesv1/service.go +++ b/lib/auth/userpreferences/userpreferencesv1/service.go @@ -25,7 +25,6 @@ import ( "google.golang.org/protobuf/types/known/emptypb" userpreferences "github.com/gravitational/teleport/api/gen/proto/go/userpreferences/v1" - "github.com/gravitational/teleport/api/types" "github.com/gravitational/teleport/lib/authz" "github.com/gravitational/teleport/lib/services" ) @@ -66,11 +65,15 @@ func NewService(cfg *ServiceConfig) (*Service, error) { // GetUserPreferences returns the user preferences for a given user. func (a *Service) GetUserPreferences(ctx context.Context, req *userpreferences.GetUserPreferencesRequest) (*userpreferences.GetUserPreferencesResponse, error) { - authCtx, err := authz.AuthorizeWithVerbs(ctx, a.log, a.authorizer, true, types.KindUser, types.VerbUpdate) + authCtx, err := a.authorizer.Authorize(ctx) if err != nil { return nil, trace.Wrap(err) } + if !authz.IsLocalUser(*authCtx) { + return nil, trace.AccessDenied("Non-local user cannot get user preferences") + } + username := authCtx.User.GetName() return a.backend.GetUserPreferences(ctx, username, req) @@ -78,10 +81,13 @@ func (a *Service) GetUserPreferences(ctx context.Context, req *userpreferences.G // UpsertUserPreferences creates or updates user preferences for a given username. func (a *Service) UpsertUserPreferences(ctx context.Context, req *userpreferences.UpsertUserPreferencesRequest) (*emptypb.Empty, error) { - authCtx, err := authz.AuthorizeWithVerbs(ctx, a.log, a.authorizer, true, types.KindUser, types.VerbUpdate) + authCtx, err := a.authorizer.Authorize(ctx) if err != nil { return nil, trace.Wrap(err) } + if !authz.IsLocalUser(*authCtx) { + return nil, trace.AccessDenied("Non-local user cannot get user preferences") + } username := authCtx.User.GetName() diff --git a/lib/auth/userpreferences/userpreferencesv1/service_test.go b/lib/auth/userpreferences/userpreferencesv1/service_test.go index 4503d00359813..6e96f89a4d92f 100644 --- a/lib/auth/userpreferences/userpreferencesv1/service_test.go +++ b/lib/auth/userpreferences/userpreferencesv1/service_test.go @@ -33,8 +33,8 @@ import ( ) const ( - defaultUser = "test-user" - noAccessUser = "user-no-access" + defaultUser = "test-user" + nonExistingUser = "non-existing-user" ) func TestService_GetUserPreferences(t *testing.T) { @@ -66,8 +66,8 @@ func TestService_GetUserPreferences(t *testing.T) { wantErr: assert.NoError, }, { - name: "access denied - RBAC rule", - userName: noAccessUser, + name: "access denied - user doesn't exist", + userName: nonExistingUser, req: &userpreferencesv1.GetUserPreferencesRequest{}, want: nil, wantErr: assert.Error, @@ -117,8 +117,8 @@ func TestService_UpsertUserPreferences(t *testing.T) { wantErr: assert.NoError, }, { - name: "access denied - RBAC rule", - userName: noAccessUser, + name: "access denied - user doesn't exist", + userName: nonExistingUser, req: &userpreferencesv1.UpsertUserPreferencesRequest{ Preferences: defaultPreferences, }, @@ -200,12 +200,6 @@ func initSvc(t *testing.T) (map[string]context.Context, *Service) { roles[defaultUser] = role - roleNoAccess, err := types.NewRole("no-rules", types.RoleSpecV6{ - Allow: types.RoleConditions{}, - }) - require.NoError(t, err) - roles["user-no-access"] = roleNoAccess - ctxs := make(map[string]context.Context, len(roles)) for username, role := range roles { err = roleSvc.CreateRole(ctx, role) diff --git a/lib/authz/permissions.go b/lib/authz/permissions.go index 5e5f1fcfa396c..2943ef65b9370 100644 --- a/lib/authz/permissions.go +++ b/lib/authz/permissions.go @@ -1375,7 +1375,7 @@ func IsLocalUser(authContext Context) bool { return ok } -// IsLocalUserAction checks if the identity is a local user and the username -func IsLocalUserAction(authContext Context, username string) bool { +// IsCurrentUser checks if the identity is a local user matching the given username +func IsCurrentUser(authContext Context, username string) bool { return IsLocalUser(authContext) && authContext.User.GetName() == username } From 8785eabe9ae374875387863fafb881f8d0ab6779 Mon Sep 17 00:00:00 2001 From: joerger Date: Thu, 27 Jul 2023 12:50:57 -0700 Subject: [PATCH 11/13] Separate client and server interfaces for user preference services. --- lib/auth/auth.go | 16 ++------------- lib/auth/clt.go | 8 +++++++- lib/auth/init.go | 2 +- .../userpreferencesv1/service.go | 15 ++++++++++---- lib/services/local/userpreferences.go | 12 +++++------ lib/services/local/userpreferences_test.go | 20 +++++++++---------- lib/services/userpreferences.go | 12 ++--------- 7 files changed, 39 insertions(+), 46 deletions(-) diff --git a/lib/auth/auth.go b/lib/auth/auth.go index 288b7fb213b44..6d8d30f0b4872 100644 --- a/lib/auth/auth.go +++ b/lib/auth/auth.go @@ -59,7 +59,6 @@ import ( "github.com/gravitational/teleport/api/constants" apidefaults "github.com/gravitational/teleport/api/defaults" "github.com/gravitational/teleport/api/gen/proto/go/assist/v1" - userpreferencesv1 "github.com/gravitational/teleport/api/gen/proto/go/userpreferences/v1" "github.com/gravitational/teleport/api/internalutils/stream" "github.com/gravitational/teleport/api/types" apievents "github.com/gravitational/teleport/api/types/events" @@ -300,7 +299,7 @@ func NewServer(cfg *InitConfig, opts ...ServerOption) (*Server, error) { StatusInternal: cfg.Status, UsageReporter: cfg.UsageReporter, Assistant: cfg.Assist, - UserPreferencesBackend: cfg.UserPreferences, + UserPreferences: cfg.UserPreferences, } closeCtx, cancelFunc := context.WithCancel(context.TODO()) @@ -416,7 +415,7 @@ type Services struct { services.AccessLists services.Assistant services.Embeddings - services.UserPreferencesBackend + services.UserPreferences usagereporter.UsageReporter types.Events events.AuditLogSessionStreamer @@ -5370,17 +5369,6 @@ func (a *Server) CompareAndSwapHeadlessAuthentication(ctx context.Context, old, return headlessAuthn, trace.Wrap(err) } -// GetUserPreferences returns the user preferences for a given user. -func (a *Server) GetUserPreferences(ctx context.Context, request *userpreferencesv1.GetUserPreferencesRequest) (*userpreferencesv1.GetUserPreferencesResponse, error) { - resp, err := a.Services.GetUserPreferences(ctx, "", request) - return resp, trace.Wrap(err) -} - -// UpsertUserPreferences creates or updates user preferences for a given username. -func (a *Server) UpsertUserPreferences(ctx context.Context, request *userpreferencesv1.UpsertUserPreferencesRequest) error { - return trace.Wrap(a.Services.UpsertUserPreferences(ctx, "", request)) -} - // getProxyPublicAddr returns the first valid, non-empty proxy public address it // finds, or empty otherwise. func (a *Server) getProxyPublicAddr() string { diff --git a/lib/auth/clt.go b/lib/auth/clt.go index 62456a0b90f65..b6ae7f1662224 100644 --- a/lib/auth/clt.go +++ b/lib/auth/clt.go @@ -33,6 +33,7 @@ import ( loginrulepb "github.com/gravitational/teleport/api/gen/proto/go/teleport/loginrule/v1" pluginspb "github.com/gravitational/teleport/api/gen/proto/go/teleport/plugins/v1" samlidppb "github.com/gravitational/teleport/api/gen/proto/go/teleport/samlidp/v1" + userpreferencesv1 "github.com/gravitational/teleport/api/gen/proto/go/userpreferences/v1" "github.com/gravitational/teleport/api/internalutils/stream" "github.com/gravitational/teleport/api/types" apievents "github.com/gravitational/teleport/api/types/events" @@ -691,7 +692,6 @@ type ClientI interface { services.SAMLIdPServiceProviders services.UserGroups services.Assistant - services.UserPreferences WebService services.Status services.ClusterConfiguration @@ -848,4 +848,10 @@ type ClientI interface { // GetResources returns a paginated list of resources. GetResources(ctx context.Context, req *proto.ListResourcesRequest) (*proto.ListResourcesResponse, error) + + // GetUserPreferences returns the user preferences for a given user. + GetUserPreferences(ctx context.Context, req *userpreferencesv1.GetUserPreferencesRequest) (*userpreferencesv1.GetUserPreferencesResponse, error) + + // UpsertUserPreferences creates or updates user preferences for a given username. + UpsertUserPreferences(ctx context.Context, req *userpreferencesv1.UpsertUserPreferencesRequest) error } diff --git a/lib/auth/init.go b/lib/auth/init.go index fea25e4121b39..0f66807ee0e5e 100644 --- a/lib/auth/init.go +++ b/lib/auth/init.go @@ -152,7 +152,7 @@ type InitConfig struct { Assist services.Assistant // UserPreferences is a service that manages user preferences. - UserPreferences services.UserPreferencesBackend + UserPreferences services.UserPreferences // Roles is a set of roles to create Roles []types.Role diff --git a/lib/auth/userpreferences/userpreferencesv1/service.go b/lib/auth/userpreferences/userpreferencesv1/service.go index 2ab12328c4e14..b5a70f06853b9 100644 --- a/lib/auth/userpreferences/userpreferencesv1/service.go +++ b/lib/auth/userpreferences/userpreferencesv1/service.go @@ -31,7 +31,7 @@ import ( // ServiceConfig holds configuration options for the user preferences service. type ServiceConfig struct { - Backend services.UserPreferencesBackend + Backend services.UserPreferences Authorizer authz.Authorizer Logger *logrus.Entry } @@ -40,7 +40,7 @@ type ServiceConfig struct { type Service struct { userpreferences.UnimplementedUserPreferencesServiceServer - backend services.UserPreferencesBackend + backend services.UserPreferences authorizer authz.Authorizer log *logrus.Entry } @@ -76,7 +76,14 @@ func (a *Service) GetUserPreferences(ctx context.Context, req *userpreferences.G username := authCtx.User.GetName() - return a.backend.GetUserPreferences(ctx, username, req) + prefs, err := a.backend.GetUserPreferences(ctx, username) + if err != nil { + return nil, trace.Wrap(err) + } + + return &userpreferences.GetUserPreferencesResponse{ + Preferences: prefs, + }, nil } // UpsertUserPreferences creates or updates user preferences for a given username. @@ -91,5 +98,5 @@ func (a *Service) UpsertUserPreferences(ctx context.Context, req *userpreference username := authCtx.User.GetName() - return &emptypb.Empty{}, trace.Wrap(a.backend.UpsertUserPreferences(ctx, username, req)) + return &emptypb.Empty{}, trace.Wrap(a.backend.UpsertUserPreferences(ctx, username, req.Preferences)) } diff --git a/lib/services/local/userpreferences.go b/lib/services/local/userpreferences.go index 453bb3316291a..e36a9dd61a415 100644 --- a/lib/services/local/userpreferences.go +++ b/lib/services/local/userpreferences.go @@ -55,25 +55,25 @@ func NewUserPreferencesService(backend backend.Backend) *UserPreferencesService } // GetUserPreferences returns the user preferences for the given user. -func (u *UserPreferencesService) GetUserPreferences(ctx context.Context, username string, _ *userpreferencesv1.GetUserPreferencesRequest) (*userpreferencesv1.GetUserPreferencesResponse, error) { +func (u *UserPreferencesService) GetUserPreferences(ctx context.Context, username string) (*userpreferencesv1.UserPreferences, error) { preferences, err := u.getUserPreferences(ctx, username) if err != nil { if trace.IsNotFound(err) { - return &userpreferencesv1.GetUserPreferencesResponse{Preferences: DefaultUserPreferences()}, nil + return DefaultUserPreferences(), nil } return nil, trace.Wrap(err) } - return &userpreferencesv1.GetUserPreferencesResponse{Preferences: preferences}, nil + return preferences, nil } // UpsertUserPreferences creates or updates user preferences for a given username. -func (u *UserPreferencesService) UpsertUserPreferences(ctx context.Context, username string, req *userpreferencesv1.UpsertUserPreferencesRequest) error { +func (u *UserPreferencesService) UpsertUserPreferences(ctx context.Context, username string, prefs *userpreferencesv1.UserPreferences) error { if username == "" { return trace.BadParameter("missing username") } - if err := validatePreferences(req.Preferences); err != nil { + if err := validatePreferences(prefs); err != nil { return trace.Wrap(err) } @@ -85,7 +85,7 @@ func (u *UserPreferencesService) UpsertUserPreferences(ctx context.Context, user preferences = DefaultUserPreferences() } - if err := overwriteValues(preferences, req.Preferences); err != nil { + if err := overwriteValues(preferences, prefs); err != nil { return trace.Wrap(err) } diff --git a/lib/services/local/userpreferences_test.go b/lib/services/local/userpreferences_test.go index 9c84cd72129eb..dcfeace7ee75a 100644 --- a/lib/services/local/userpreferences_test.go +++ b/lib/services/local/userpreferences_test.go @@ -163,20 +163,20 @@ func TestUserPreferencesCRUD2(t *testing.T) { identity := newUserPreferencesService(t) - res, err := identity.GetUserPreferences(ctx, username, &userpreferencesv1.GetUserPreferencesRequest{}) + res, err := identity.GetUserPreferences(ctx, username) require.NoError(t, err) // Clone the proto as the accessing fields for some reason modifies the state. - require.Empty(t, cmp.Diff(defaultPref, proto.Clone(res.Preferences), protocmp.Transform())) + require.Empty(t, cmp.Diff(defaultPref, proto.Clone(res), protocmp.Transform())) if test.req != nil { - err := identity.UpsertUserPreferences(ctx, username, test.req) + err := identity.UpsertUserPreferences(ctx, username, test.req.Preferences) require.NoError(t, err) } - res, err = identity.GetUserPreferences(ctx, username, &userpreferencesv1.GetUserPreferencesRequest{}) + res, err = identity.GetUserPreferences(ctx, username) require.NoError(t, err) - require.Empty(t, cmp.Diff(test.expected, res.Preferences, protocmp.Transform())) + require.Empty(t, cmp.Diff(test.expected, res, protocmp.Transform())) }) } } @@ -204,13 +204,13 @@ func TestLayoutUpdate(t *testing.T) { require.NoError(t, err) // Get the preferences and ensure that the layout is updated. - prefs, err := identity.GetUserPreferences(ctx, "test", &userpreferencesv1.GetUserPreferencesRequest{}) + prefs, err := identity.GetUserPreferences(ctx, "test") require.NoError(t, err) // The layout should be updated to the latest version (values should not be nil). - require.NotNil(t, prefs.Preferences.Onboard) + require.NotNil(t, prefs.Onboard) // Non-existing values should be set to the default value. - require.Equal(t, userpreferencesv1.AssistViewMode_ASSIST_VIEW_MODE_DOCKED, prefs.Preferences.Assist.ViewMode) - require.Equal(t, userpreferencesv1.Theme_THEME_LIGHT, prefs.Preferences.Theme) + require.Equal(t, userpreferencesv1.AssistViewMode_ASSIST_VIEW_MODE_DOCKED, prefs.Assist.ViewMode) + require.Equal(t, userpreferencesv1.Theme_THEME_LIGHT, prefs.Theme) // Existing values should be preserved. - require.Equal(t, []string{"foo", "bar"}, prefs.Preferences.Assist.PreferredLogins) + require.Equal(t, []string{"foo", "bar"}, prefs.Assist.PreferredLogins) } diff --git a/lib/services/userpreferences.go b/lib/services/userpreferences.go index da6832dad8f5c..3f2539be32bda 100644 --- a/lib/services/userpreferences.go +++ b/lib/services/userpreferences.go @@ -27,15 +27,7 @@ import ( // UserPreferences is the interface for managing user preferences. type UserPreferences interface { // GetUserPreferences returns the user preferences for a given user. - GetUserPreferences(ctx context.Context, req *userpreferencesv1.GetUserPreferencesRequest) (*userpreferencesv1.GetUserPreferencesResponse, error) + GetUserPreferences(ctx context.Context, username string) (*userpreferencesv1.UserPreferences, error) // UpsertUserPreferences creates or updates user preferences for a given username. - UpsertUserPreferences(ctx context.Context, req *userpreferencesv1.UpsertUserPreferencesRequest) error -} - -// UserPreferencesBackend is the database interface for managing user preferences. -type UserPreferencesBackend interface { - // GetUserPreferences returns the user preferences for a given user. - GetUserPreferences(ctx context.Context, username string, req *userpreferencesv1.GetUserPreferencesRequest) (*userpreferencesv1.GetUserPreferencesResponse, error) - // UpsertUserPreferences creates or updates user preferences for a given username. - UpsertUserPreferences(ctx context.Context, username string, req *userpreferencesv1.UpsertUserPreferencesRequest) error + UpsertUserPreferences(ctx context.Context, username string, prefs *userpreferencesv1.UserPreferences) error } From ac9a4de144df96afa1c7f0f01fd70ed941b14d0e Mon Sep 17 00:00:00 2001 From: Jakub Nyckowski Date: Fri, 28 Jul 2023 19:13:40 -0400 Subject: [PATCH 12/13] Apply core review suggestions --- lib/auth/userpreferences/userpreferencesv1/service.go | 4 ++-- .../userpreferences/userpreferencesv1/service_test.go | 11 +---------- 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/lib/auth/userpreferences/userpreferencesv1/service.go b/lib/auth/userpreferences/userpreferencesv1/service.go index b5a70f06853b9..37393f0c770ad 100644 --- a/lib/auth/userpreferences/userpreferencesv1/service.go +++ b/lib/auth/userpreferences/userpreferencesv1/service.go @@ -64,7 +64,7 @@ func NewService(cfg *ServiceConfig) (*Service, error) { } // GetUserPreferences returns the user preferences for a given user. -func (a *Service) GetUserPreferences(ctx context.Context, req *userpreferences.GetUserPreferencesRequest) (*userpreferences.GetUserPreferencesResponse, error) { +func (a *Service) GetUserPreferences(ctx context.Context, _ *userpreferences.GetUserPreferencesRequest) (*userpreferences.GetUserPreferencesResponse, error) { authCtx, err := a.authorizer.Authorize(ctx) if err != nil { return nil, trace.Wrap(err) @@ -93,7 +93,7 @@ func (a *Service) UpsertUserPreferences(ctx context.Context, req *userpreference return nil, trace.Wrap(err) } if !authz.IsLocalUser(*authCtx) { - return nil, trace.AccessDenied("Non-local user cannot get user preferences") + return nil, trace.AccessDenied("Non-local user cannot upsert user preferences") } username := authCtx.User.GetName() diff --git a/lib/auth/userpreferences/userpreferencesv1/service_test.go b/lib/auth/userpreferences/userpreferencesv1/service_test.go index 6e96f89a4d92f..6b15f8980fd89 100644 --- a/lib/auth/userpreferences/userpreferencesv1/service_test.go +++ b/lib/auth/userpreferences/userpreferencesv1/service_test.go @@ -186,16 +186,7 @@ func initSvc(t *testing.T) (map[string]context.Context, *Service) { roles := map[string]types.Role{} - role, err := types.NewRole("allow-rules", types.RoleSpecV6{ - Allow: types.RoleConditions{ - Rules: []types.Rule{ - { - Resources: []string{types.KindUser}, - Verbs: []string{types.VerbList, types.VerbRead, types.VerbUpdate, types.VerbCreate, types.VerbDelete}, - }, - }, - }, - }) + role, err := types.NewRole("allow-rules", types.RoleSpecV6{}) require.NoError(t, err) roles[defaultUser] = role From d38636a3dc07aaf3257ae25bfd6823c48f160928 Mon Sep 17 00:00:00 2001 From: Jakub Nyckowski Date: Tue, 1 Aug 2023 10:00:51 -0400 Subject: [PATCH 13/13] Apply suggestions from code review Co-authored-by: Brian Joerger --- lib/auth/assist/assistv1/service.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/auth/assist/assistv1/service.go b/lib/auth/assist/assistv1/service.go index e72b85201c06c..fd5a5b0efb940 100644 --- a/lib/auth/assist/assistv1/service.go +++ b/lib/auth/assist/assistv1/service.go @@ -127,7 +127,7 @@ func (a *Service) UpdateAssistantConversationInfo(ctx context.Context, req *assi // GetAssistantConversations returns all conversations started by a user. func (a *Service) GetAssistantConversations(ctx context.Context, req *assist.GetAssistantConversationsRequest) (*assist.GetAssistantConversationsResponse, error) { - authCtx, err := authz.AuthorizeWithVerbs(ctx, a.log, a.authorizer, true, types.KindAssistant, types.VerbRead, types.VerbList) + authCtx, err := authz.AuthorizeWithVerbs(ctx, a.log, a.authorizer, true, types.KindAssistant, types.VerbList) if err != nil { return nil, authz.ConvertAuthorizerError(ctx, a.log, err) } @@ -196,7 +196,7 @@ func (a *Service) IsAssistEnabled(ctx context.Context, _ *assist.IsAssistEnabled } // Check if this endpoint is called by a user or Proxy. - if _, ok := authCtx.UnmappedIdentity.(authz.LocalUser); ok { + if authz.IsLocalUser(*authCtx) { checkErr := authCtx.Checker.CheckAccessToRule( &services.Context{User: authCtx.User}, defaults.Namespace, types.KindAssistant, types.VerbRead,