Skip to content

Commit

Permalink
log user out when a role in their session certificate no longer exists (
Browse files Browse the repository at this point in the history
  • Loading branch information
rudream authored Aug 23, 2024
1 parent b5cdd3d commit 838abdd
Show file tree
Hide file tree
Showing 10 changed files with 155 additions and 11 deletions.
1 change: 1 addition & 0 deletions lib/services/local/access.go
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,7 @@ func (s *AccessService) GetRole(ctx context.Context, name string) (types.Role, e
item, err := s.Get(ctx, backend.NewKey(rolesPrefix, name, paramsPrefix))
if err != nil {
if trace.IsNotFound(err) {
// This error message format should be kept in sync with web/packages/teleport/src/services/api/api.isRoleNotFoundError
return nil, trace.NotFound("role %v is not found", name)
}
return nil, trace.Wrap(err)
Expand Down
10 changes: 10 additions & 0 deletions web/packages/teleport/src/Login/Login.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ beforeEach(() => {
jest.restoreAllMocks();
jest.spyOn(history, 'push').mockImplementation();
jest.spyOn(history, 'getRedirectParam').mockImplementation(() => '/');
jest.spyOn(history, 'hasAccessChangedParam').mockImplementation(() => false);
user = userEvent.setup();
});

Expand Down Expand Up @@ -184,4 +185,13 @@ describe('test MOTD', () => {
screen.queryByText('Welcome to cluster, your activity will be recorded.')
).not.toBeInTheDocument();
});

test('access changed message renders when the URL param is set', () => {
jest.spyOn(history, 'hasAccessChangedParam').mockImplementation(() => true);

render(<Login />);

expect(screen.getByText(/sign in to teleport/i)).toBeInTheDocument();
expect(screen.getByText(/Your access has changed/i)).toBeInTheDocument();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,11 @@ export function ErrorDialog({ errMsg }: { errMsg: string }) {
</DialogContent>
<DialogFooter>
<ButtonSecondary
onClick={() => history.goToLogin(true /* rememberLocation */)}
onClick={() =>
history.goToLogin({
rememberLocation: true,
})
}
>
Go to Login
</ButtonSecondary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,15 @@ import React from 'react';

import { render, fireEvent, screen } from 'design/utils/testing';

import history from 'teleport/services/history';

import FormLogin, { Props } from './FormLogin';

beforeEach(() => {
jest.restoreAllMocks();
jest.spyOn(history, 'hasAccessChangedParam').mockImplementation(() => false);
});

test('primary username and password with mfa off', () => {
const onLogin = jest.fn();

Expand Down
8 changes: 8 additions & 0 deletions web/packages/teleport/src/components/FormLogin/FormLogin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import { StepSlider, StepComponentProps } from 'design/StepSlider';
import { P } from 'design/Text/Text';

import { UserCredentials } from 'teleport/services/auth';
import history from 'teleport/services/history';

import { PasskeyIcons } from '../Passkeys';

Expand Down Expand Up @@ -87,13 +88,20 @@ export default function LoginForm(props: Props) {
errorMessage = attempt.message;
}

const showAccessChangedMessage = history.hasAccessChangedParam();

// Everything below requires local auth to be enabled.
return (
<Card my="5" mx="auto" width={500} py={4}>
<Text typography="h1" mb={4} textAlign="center">
Sign in to Teleport
</Text>
{errorMessage && <Alerts.Danger m={4}>{errorMessage}</Alerts.Danger>}
{showAccessChangedMessage && (
<Alerts.Warning m={4}>
Your access has changed. Please re-login.
</Alerts.Warning>
)}
{allowedAuthTypes.length > 0 ? (
<StepSlider<typeof loginViews>
flows={loginViews}
Expand Down
18 changes: 17 additions & 1 deletion web/packages/teleport/src/services/api/api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,12 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import api, { MFA_HEADER, defaultRequestOptions, getAuthHeaders } from './api';
import api, {
MFA_HEADER,
defaultRequestOptions,
getAuthHeaders,
isRoleNotFoundError,
} from './api';

describe('api.fetch', () => {
const mockedFetch = jest.spyOn(global, 'fetch').mockResolvedValue({} as any); // we don't care about response
Expand Down Expand Up @@ -175,3 +180,14 @@ test('fetchJson 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);

const errorMessage2 = ' role test-role is not found ';
expect(isRoleNotFoundError(errorMessage2)).toBe(true);

const errorMessage3 = 'failed to list access lists';
expect(isRoleNotFoundError(errorMessage3)).toBe(false);
});
19 changes: 19 additions & 0 deletions web/packages/teleport/src/services/api/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import 'whatwg-fetch';
import auth, { MfaChallengeScope } from 'teleport/services/auth/auth';
import websession from 'teleport/services/websession';

import { storageService } from '../storageService';
import { WebauthnAssertionResponse } from '../auth';
Expand Down Expand Up @@ -157,6 +158,18 @@ const api = {
return json;
}

/** This error can occur in the edge case where a role in the user's certificate was deleted during their session. */
const isRoleNotFoundErr = isRoleNotFoundError(parseError(json));
if (isRoleNotFoundErr) {
websession.logoutWithoutSlo({
/* Don't remember location after login, since they may no longer have access to the page they were on. */
rememberLocation: false,
/* Show "access changed" notice on login page. */
withAccessChangedMessage: true,
});
return;
}

// Retry with MFA if we get an admin action missing MFA error.
const isAdminActionMfaError = isAdminActionRequiresMfaError(
parseError(json)
Expand Down Expand Up @@ -301,4 +314,10 @@ function isAdminActionRequiresMfaError(errMessage) {
);
}

/** 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);
}

export default api;
54 changes: 53 additions & 1 deletion web/packages/teleport/src/services/history/history.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,63 @@ describe('services/history', () => {
.spyOn(history, 'getRoutes')
.mockReturnValue(['/web/login', '/current-location']);
history.original().location.pathname = '/current-location';
history.goToLogin(true);
history.goToLogin({ rememberLocation: true });

const expected =
'/web/login?redirect_uri=http://localhost/current-location';
expect(history._pageRefresh).toHaveBeenCalledWith(expected);
});

it('should navigate to login with access_changed param and no redirect_uri', () => {
jest
.spyOn(history, 'getRoutes')
.mockReturnValue(['/web/login', '/current-location']);
history.original().location.pathname = '/current-location';
history.goToLogin({ withAccessChangedMessage: true });

const expected = '/web/login?access_changed';
expect(history._pageRefresh).toHaveBeenCalledWith(expected);
});

it('should navigate to login with access_changed param and redirect_uri', () => {
jest
.spyOn(history, 'getRoutes')
.mockReturnValue(['/web/login', '/current-location']);
history.original().location.pathname = '/current-location';
history.goToLogin({
rememberLocation: true,
withAccessChangedMessage: true,
});

const expected =
'/web/login?access_changed&redirect_uri=http://localhost/current-location';
expect(history._pageRefresh).toHaveBeenCalledWith(expected);
});

it('should navigate to login with no params', () => {
jest
.spyOn(history, 'getRoutes')
.mockReturnValue(['/web/login', '/current-location']);
history.original().location.pathname = '/current-location';
history.goToLogin();

const expected = '/web/login';
expect(history._pageRefresh).toHaveBeenCalledWith(expected);
});

it('should preserve query params in the redirect_uri', () => {
jest
.spyOn(history, 'getRoutes')
.mockReturnValue(['/web/login', '/current-location']);
history.original().location.pathname = '/current-location?test=value';
history.goToLogin({
rememberLocation: true,
withAccessChangedMessage: true,
});

const expected =
'/web/login?access_changed&redirect_uri=http://localhost/current-location?test=value';
expect(history._pageRefresh).toHaveBeenCalledWith(expected);
});
});
});
31 changes: 27 additions & 4 deletions web/packages/teleport/src/services/history/history.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,24 +53,42 @@ const history = {
window.location.reload();
},

goToLogin(rememberLocation = false) {
let url = cfg.routes.login;
goToLogin({
rememberLocation = false,
withAccessChangedMessage = false,
} = {}) {
const params: string[] = [];

// withAccessChangedMessage determines whether the login page the user is redirected to should include a notice that
// they were logged out due to their roles having changed.
if (withAccessChangedMessage) {
params.push('access_changed');
}

if (rememberLocation) {
const { search, pathname } = _inst.location;
const knownRoute = this.ensureKnownRoute(pathname);
const knownRedirect = this.ensureBaseUrl(knownRoute);
const query = search ? encodeURIComponent(search) : '';

url = `${url}?redirect_uri=${knownRedirect}${query}`;
params.push(`redirect_uri=${knownRedirect}${query}`);
}

const queryString = params.join('&');
const url = queryString
? `${cfg.routes.login}?${queryString}`
: cfg.routes.login;

this._pageRefresh(url);
},

getRedirectParam() {
return getUrlParameter('redirect_uri', this.original().location.search);
},

hasAccessChangedParam() {
return hasUrlParameter('access_changed', this.original().location.search);
},

ensureKnownRoute(route = '') {
return this._canPush(route) ? route : cfg.routes.root;
},
Expand Down Expand Up @@ -124,3 +142,8 @@ export function getUrlParameter(name = '', path = '') {
const value = params.get(name);
return value || '';
}

function hasUrlParameter(name = '', path = '') {
const params = new URLSearchParams(path);
return params.has(name);
}
12 changes: 8 additions & 4 deletions web/packages/teleport/src/services/websession/websession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,20 +41,24 @@ const session = {
if (response.samlSloUrl) {
window.open(response.samlSloUrl, '_self');
} else {
history.goToLogin(rememberLocation);
history.goToLogin({ rememberLocation });
}
});
},

logoutWithoutSlo(rememberLocation = false) {
logoutWithoutSlo({
rememberLocation = false,
withAccessChangedMessage = false,
} = {}) {
api.delete(cfg.api.webSessionPath).finally(() => {
history.goToLogin(rememberLocation);
this.clear();
history.goToLogin({ rememberLocation, withAccessChangedMessage });
});
},

clearBrowserSession(rememberLocation = false) {
this.clear();
history.goToLogin(rememberLocation);
history.goToLogin({ rememberLocation });
},

clear() {
Expand Down

0 comments on commit 838abdd

Please sign in to comment.