From f0e10df37268aa64ea787cd8285b35690cb8f79e Mon Sep 17 00:00:00 2001 From: Yassine Bounekhla <56373201+rudream@users.noreply.github.com> Date: Fri, 22 Aug 2025 09:43:03 -0400 Subject: [PATCH] Fix users being logged out on any `role not found` error (#57677) * fix users being logged out on any role not found error * CR --- lib/auth/auth_with_roles.go | 9 ++- lib/services/access_checker.go | 21 ++++++ lib/services/access_checker_test.go | 67 +++++++++++++++++++ lib/services/role.go | 7 ++ lib/web/sessions.go | 2 +- .../teleport/src/services/api/api.test.ts | 62 +++++++++++------ web/packages/teleport/src/services/api/api.ts | 31 +++------ .../src/services/resources/resource.ts | 9 +-- 8 files changed, 157 insertions(+), 51 deletions(-) diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go index 07a4a8f1754..9159481dc79 100644 --- a/lib/auth/auth_with_roles.go +++ b/lib/auth/auth_with_roles.go @@ -4747,7 +4747,14 @@ func (a *ServerWithRoles) GetRole(ctx context.Context, name string) (types.Role, // that they hold. This requirement is checked first to avoid // misleading denial messages in the logs. if slices.Contains(a.context.User.GetRoles(), name) { - return a.authServer.GetRole(ctx, name) + role, err := a.authServer.GetRole(ctx, name) + if err != nil && trace.IsNotFound(err) { + // Add the UserSessionRoleNotFoundErrorMsg message to indicate this role not found error was + // encountered while the user was looking up one of their own roles. At this point, the + // role not found error can only happen if a role in their session cert doesn't exist anymore. + return nil, trace.Wrap(err, services.UserSessionRoleNotFoundErrorMsg) + } + return role, trace.Wrap(err) } authErr := a.authorizeAction(types.KindRole, types.VerbRead) diff --git a/lib/services/access_checker.go b/lib/services/access_checker.go index 0b4307cf6b8..76e126fb84d 100644 --- a/lib/services/access_checker.go +++ b/lib/services/access_checker.go @@ -350,6 +350,27 @@ func NewAccessChecker(info *AccessInfo, localCluster string, access RoleGetter) }, nil } +// NewAccessCheckerForUserSession is an alternative to NewAccessChecker that includes a UserSessionRoleNotFoundErrorMsg if +// a role from the user's session is not found during the access check. This allows the Web UI to distinguish between +// a user session role lookup error (which should prompt the user to re-login) vs. other role lookup +// failures. +func NewAccessCheckerForUserSession(info *AccessInfo, localCluster string, access RoleGetter) (AccessChecker, error) { + roleSet, err := FetchRoles(info.Roles, access, info.Traits) + if err != nil { + if trace.IsNotFound(err) { + // Add the UserSessionRoleNotFoundErrorMsg message to indicate this role not found error was encountered fetching + // the user's session roles. This can only happen if the user's session certificate contains a role that no longer exists. + return nil, trace.Wrap(err, UserSessionRoleNotFoundErrorMsg) + } + return nil, trace.Wrap(err) + } + return &accessChecker{ + info: info, + localCluster: localCluster, + RoleSet: roleSet, + }, nil +} + // NewAccessCheckerWithRoleSet is similar to NewAccessChecker, but accepts the // full RoleSet rather than a RoleGetter. func NewAccessCheckerWithRoleSet(info *AccessInfo, localCluster string, roleSet RoleSet) AccessChecker { diff --git a/lib/services/access_checker_test.go b/lib/services/access_checker_test.go index a8045d76d90..aefc4a7c76a 100644 --- a/lib/services/access_checker_test.go +++ b/lib/services/access_checker_test.go @@ -19,9 +19,11 @@ package services import ( + "context" "sort" "testing" + "github.com/gravitational/trace" "github.com/stretchr/testify/require" decisionpb "github.com/gravitational/teleport/api/gen/proto/go/teleport/decision/v1alpha1" @@ -1160,3 +1162,68 @@ func TestIdentityCenterAccountAccessRequestMatcher(t *testing.T) { }) } } + +// TestUserSessionRoleNotFoundError ensures that role not found errors during user session access checks include UserSessionRoleNotFoundErrorMsg when appropriate, +func TestUserSessionRoleNotFoundError(t *testing.T) { + // Create a mock RoleGetter that returns "role not found" error for a specific role + mockRoleGetter := &mockRoleGetter{ + roles: map[string]types.Role{ + "existing-role": newRole(func(rv *types.RoleV6) { rv.SetName("existing-role") }), + }, + } + + t.Run("NewAccessChecker with missing role does not add UserSessionRoleNotFoundErrorMsg", func(t *testing.T) { + accessInfo := &AccessInfo{ + Roles: []string{"missing-role"}, + } + + _, err := NewAccessChecker(accessInfo, "cluster", mockRoleGetter) + require.Error(t, err) + require.True(t, trace.IsNotFound(err)) + require.Contains(t, err.Error(), "role missing-role is not found") + require.NotContains(t, err.Error(), UserSessionRoleNotFoundErrorMsg) + }) + + t.Run("NewAccessCheckerForUserSession with missing role adds UserSessionRoleNotFoundErrorMsg", func(t *testing.T) { + accessInfo := &AccessInfo{ + Roles: []string{"missing-role"}, + } + + _, err := NewAccessCheckerForUserSession(accessInfo, "cluster", mockRoleGetter) + require.Error(t, err) + require.True(t, trace.IsNotFound(err)) + require.Contains(t, err.Error(), "role missing-role is not found") + require.Contains(t, err.Error(), UserSessionRoleNotFoundErrorMsg) + }) + + t.Run("NewAccessCheckerForUserSession with existing role succeeds", func(t *testing.T) { + accessInfo := &AccessInfo{ + Roles: []string{"existing-role"}, + } + + checker, err := NewAccessCheckerForUserSession(accessInfo, "cluster", mockRoleGetter) + require.NoError(t, err) + require.NotNil(t, checker) + }) +} + +// mockRoleGetter implements RoleGetter for testing +type mockRoleGetter struct { + roles map[string]types.Role +} + +func (m *mockRoleGetter) GetRole(ctx context.Context, name string) (types.Role, error) { + if role, exists := m.roles[name]; exists { + return role, nil + } + // Return the same error format as the real implementation + return nil, trace.NotFound("role %v is not found", name) +} + +func (m *mockRoleGetter) GetRoles(ctx context.Context) ([]types.Role, error) { + var roles []types.Role + for _, role := range m.roles { + roles = append(roles, role) + } + return roles, nil +} diff --git a/lib/services/role.go b/lib/services/role.go index 044e4d53fee..34d62a79eef 100644 --- a/lib/services/role.go +++ b/lib/services/role.go @@ -3651,6 +3651,13 @@ const ( MFARequiredPerRole MFARequired = "per-role" ) +// UserSessionRoleNotFoundErrorMsg is added to "role not found" errors when they occur +// during user session roles validation. This allows the Web UI to distinguish between +// a user session role lookup error (which should prompt the user to re-login) vs. other role lookup +// failures. +// Keep in sync with teleport/src/services/api/api.ts(isUserSessionRoleNotFoundError) +const UserSessionRoleNotFoundErrorMsg = "user session role not found" + // UnmarshalRole unmarshals the Role resource from JSON. func UnmarshalRole(bytes []byte, opts ...MarshalOption) (types.Role, error) { return UnmarshalRoleV6(bytes, opts...) diff --git a/lib/web/sessions.go b/lib/web/sessions.go index 05a28f21d5f..c6f9931d9b7 100644 --- a/lib/web/sessions.go +++ b/lib/web/sessions.go @@ -515,7 +515,7 @@ func (c *SessionContext) GetUserAccessChecker() (services.AccessChecker, error) accessInfo := services.AccessInfoFromLocalSSHIdentity(ident) - accessChecker, err := services.NewAccessChecker(accessInfo, c.cfg.RootClusterName, c.cfg.UnsafeCachedAuthClient) + accessChecker, err := services.NewAccessCheckerForUserSession(accessInfo, c.cfg.RootClusterName, c.cfg.UnsafeCachedAuthClient) return accessChecker, trace.Wrap(err) } diff --git a/web/packages/teleport/src/services/api/api.test.ts b/web/packages/teleport/src/services/api/api.test.ts index cf4197b5a30..985a639952d 100644 --- a/web/packages/teleport/src/services/api/api.test.ts +++ b/web/packages/teleport/src/services/api/api.test.ts @@ -21,7 +21,7 @@ import websession from '../websession'; import api, { defaultRequestOptions, getAuthHeaders, - isRoleNotFoundError, + isUserSessionRoleNotFoundError, MFA_HEADER, } from './api'; import { ApiError } from './parseError'; @@ -205,31 +205,45 @@ test('fetchJsonWithMfaAuthnRetry does not return any', () => { expect(true).toBe(true); }); -test('isRoleNotFoundError correctly identifies role not found errors', () => { - const errorMessage1 = 'role admin is not found'; - expect(isRoleNotFoundError(errorMessage1)).toBe(true); +test('isUserSessionRoleNotFoundError correctly identifies user session role not found errors', () => { + const userSessionRoleError = { + error: 'role admin is not found', + messages: ['user session role not found'], + }; + expect(isUserSessionRoleNotFoundError(userSessionRoleError)).toBe(true); - const errorMessage2 = ' role test-role is not found '; - expect(isRoleNotFoundError(errorMessage2)).toBe(true); + const regularRoleError = { error: 'role admin is not found' }; + expect(isUserSessionRoleNotFoundError(regularRoleError)).toBe(false); - const errorMessage3 = 'failed to list access lists'; - expect(isRoleNotFoundError(errorMessage3)).toBe(false); + const regularRoleWithIrrelevantMessages = { + error: 'role admin is not found', + messages: ['some_other_message'], + }; + expect( + isUserSessionRoleNotFoundError(regularRoleWithIrrelevantMessages) + ).toBe(false); + + const nonRoleError = { error: 'failed to list access lists' }; + expect(isUserSessionRoleNotFoundError(nonRoleError)).toBe(false); }); -describe('api.get handling of role not found errors', () => { - beforeEach(() => { - jest.spyOn(global, 'fetch').mockResolvedValue({ - json: async () => ({ error: { message: 'role foo is not found' } }), - ok: false, - status: 404, - } as Response); // we don't care about response - }); - +describe('handling of role not found errors', () => { afterEach(() => { jest.resetAllMocks(); }); - test('sign out on error', async () => { + test('sign out on user session role not found error', async () => { + jest.spyOn(global, 'fetch').mockResolvedValue({ + json: async () => ({ + error: { + message: 'role foo is not found', + }, + messages: ['user session role not found'], + }), + ok: false, + status: 404, + } as Response); + await api.get('/foobar'); expect(mockedWebsession.logoutWithoutSlo).toHaveBeenCalledWith({ rememberLocation: false, @@ -237,10 +251,14 @@ describe('api.get handling of role not found errors', () => { }); }); - test("don't sign out on error", async () => { - await expect( - api.get('/foobar', null, null, { allowRoleNotFound: true }) - ).rejects.toThrow(ApiError); + test("don't sign out on regular role not found error", async () => { + jest.spyOn(global, 'fetch').mockResolvedValue({ + json: async () => ({ error: { message: 'role foo is not found' } }), + ok: false, + status: 404, + } as Response); + + await expect(api.get('/foobar')).rejects.toThrow(ApiError); expect(mockedWebsession.logoutWithoutSlo).not.toHaveBeenCalled(); }); }); diff --git a/web/packages/teleport/src/services/api/api.ts b/web/packages/teleport/src/services/api/api.ts index a441cd73d0d..84e045decda 100644 --- a/web/packages/teleport/src/services/api/api.ts +++ b/web/packages/teleport/src/services/api/api.ts @@ -28,15 +28,6 @@ import parseError, { ApiError, parseProxyVersion } from './parseError'; export const MFA_HEADER = 'Teleport-Mfa-Response'; type RequestOptions = { - /** - * Usually, an HTTP/404 with a "role not found" message means that the user - * can't be authorized and needs to sign in again. In such case the API - * service immediately signs the user out. Setting this flag to `true` - * overrides this behavior and allows a "role not found" error to be - * propagated up the call stack. - */ - allowRoleNotFound?: boolean; - /** * If set to `true`, the API service will not attempt to retry after an MFA * challenge. @@ -203,7 +194,7 @@ const api = { ): Promise { try { const response = await api.fetch(url, customOptions, mfaResponse); - return await api.getJsonFromFetchResponse(response, options); + return await api.getJsonFromFetchResponse(response); } catch (err) { // Retry with MFA if we get an admin action MFA error. if ( @@ -213,15 +204,14 @@ const api = { ) { mfaResponse = await api.getAdminActionMfaResponse(); const response = await api.fetch(url, customOptions, mfaResponse); - return await api.getJsonFromFetchResponse(response, options); + return await api.getJsonFromFetchResponse(response); } else { throw err; } } }, - async getJsonFromFetchResponse(response: Response, options: RequestOptions) { - const { allowRoleNotFound = false } = options; + async getJsonFromFetchResponse(response: Response) { let json; try { json = await response.json(); @@ -238,9 +228,8 @@ const api = { } /** This error can occur in the edge case where a role in the user's certificate was deleted during their session. */ - const isRoleNotFoundErr = - !allowRoleNotFound && isRoleNotFoundError(parseError(json)); - if (isRoleNotFoundErr) { + const isUserSessionRoleNotFoundErr = isUserSessionRoleNotFoundError(json); + if (isUserSessionRoleNotFoundErr) { websession.logoutWithoutSlo({ /* Don't remember location after login, since they may no longer have access to the page they were on. */ rememberLocation: false, @@ -394,10 +383,12 @@ export function isAdminActionRequiresMfaError(err: Error) { ); } -/** isRoleNotFoundError returns true if the error message is due to a role not being found. */ -export function isRoleNotFoundError(errMessage: string): boolean { - // This error message format should be kept in sync with the NotFound error message returned in lib/services/local/access.GetRole - return /role \S+ is not found/.test(errMessage); +/** isUserSessionRoleNotFoundError returns true if the error is a role not found error encountered durings user session role validation */ +export function isUserSessionRoleNotFoundError(json: any): boolean { + // Keep in sync with lib/services/role.go(UserSessionRoleNotFoundError) + return ( + !!json.error && !!json?.messages?.includes('user session role not found') + ); } export default api; diff --git a/web/packages/teleport/src/services/resources/resource.ts b/web/packages/teleport/src/services/resources/resource.ts index f5bf266dec9..ee06fe4cfaf 100644 --- a/web/packages/teleport/src/services/resources/resource.ts +++ b/web/packages/teleport/src/services/resources/resource.ts @@ -174,10 +174,7 @@ class ResourceService { await api.get( cfg.getRoleUrl({ action: 'get', name }), undefined, - undefined, - { - allowRoleNotFound: true, - } + undefined ) ); } @@ -252,9 +249,7 @@ export async function fetchRole( signal?: AbortSignal ): Promise { return makeResource<'role'>( - await api.get(cfg.getRoleUrl({ action: 'get', name }), signal, undefined, { - allowRoleNotFound: true, - }) + await api.get(cfg.getRoleUrl({ action: 'get', name }), signal, undefined) ); }