Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SSO option during PerSessionMFA in the web #47876

Merged
merged 8 commits into from
Oct 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 2 additions & 11 deletions web/packages/shared/components/ButtonSso/ButtonSso.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { ButtonProps, ButtonSecondary } from 'design/Button';

import { ResourceIcon } from 'design/ResourceIcon';

import { AuthProviderType } from 'shared/services';
import { AuthProviderType, SSOType } from 'shared/services';

const ButtonSso = forwardRef<HTMLButtonElement, Props>((props: Props, ref) => {
const { ssoType = 'unknown', title, ...rest } = props;
Expand All @@ -41,16 +41,7 @@ type Props = ButtonProps<'button'> & {
title: string;
};

type SSOType =
| 'microsoft'
| 'github'
| 'bitbucket'
| 'google'
| 'openid'
| 'okta'
| 'unknown';

function SSOIcon({ type }: { type: SSOType }) {
export function SSOIcon({ type }: { type: SSOType }) {
const commonResourceIconProps = {
width: '24px',
height: '24px',
Expand Down
9 changes: 9 additions & 0 deletions web/packages/shared/services/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,15 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

export type SSOType =
| 'microsoft'
| 'github'
| 'bitbucket'
| 'google'
| 'openid'
| 'okta'
| 'unknown';

export type AuthProviderType = 'oidc' | 'saml' | 'github';

export type Auth2faType = 'otp' | 'off' | 'optional' | 'on' | 'webauthn';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,41 @@ export default {
title: 'Teleport/AuthnDialog',
};

export const Loaded = () => <AuthnDialog {...props} />;
export const Loaded = () => {
const props: Props = {
...defaultProps,
mfa: {
...defaultProps.mfa,
ssoChallenge: {
redirectUrl: 'hi',
requestId: '123',
channelId: '123',
device: {
connectorId: '123',
connectorType: 'saml',
displayName: 'Okta',
},
},
webauthnPublicKey: {
challenge: new ArrayBuffer(1),
},
},
};
return <AuthnDialog {...props} />;
};

export const Error = () => <AuthnDialog {...props} />;
export const Error = () => {
const props: Props = {
...defaultProps,
mfa: {
...defaultProps.mfa,
errorText: 'Something went wrong',
},
};
return <AuthnDialog {...props} />;
};

const props: Props = {
const defaultProps: Props = {
mfa: makeDefaultMfaState(),
onCancel: () => null,
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/**
* Teleport
* Copyright (C) 2024 Gravitational, Inc.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

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

import { makeDefaultMfaState, MfaState } from 'teleport/lib/useMfa';
import { SSOChallenge } from 'teleport/services/auth';

import AuthnDialog from './AuthnDialog';

const mockSsoChallenge: SSOChallenge = {
redirectUrl: 'url',
requestId: '123',
device: {
displayName: 'Okta',
connectorId: '123',
connectorType: 'saml',
},
channelId: '123',
};

function makeMockState(partial: Partial<MfaState>): MfaState {
const mfa = makeDefaultMfaState();
return {
...mfa,
...partial,
};
}

describe('AuthnDialog', () => {
const mockOnCancel = jest.fn();

beforeEach(() => {
jest.clearAllMocks();
});

test('renders the dialog with basic content', () => {
const mfa = makeMockState({ ssoChallenge: mockSsoChallenge });
render(<AuthnDialog mfa={mfa} onCancel={mockOnCancel} />);

expect(
screen.getByText('Re-authenticate in the Browser')
).toBeInTheDocument();
expect(
screen.getByText(
'To continue, you must verify your identity by re-authenticating:'
)
).toBeInTheDocument();
expect(screen.getByText('Okta')).toBeInTheDocument();
expect(screen.getByTestId('close-dialog')).toBeInTheDocument();
});

test('displays error text when provided', () => {
const errorText = 'Authentication failed';
const mfa = makeMockState({ errorText });
render(<AuthnDialog mfa={mfa} onCancel={mockOnCancel} />);

expect(screen.getByTestId('danger-alert')).toBeInTheDocument();
expect(screen.getByText(errorText)).toBeInTheDocument();
});

test('sso button renders with callback', async () => {
const mfa = makeMockState({
ssoChallenge: mockSsoChallenge,
onSsoAuthenticate: jest.fn(),
});
render(<AuthnDialog mfa={mfa} onCancel={mockOnCancel} />);
const ssoButton = screen.getByText('Okta');
fireEvent.click(ssoButton);
expect(mfa.onSsoAuthenticate).toHaveBeenCalledTimes(1);
});

test('webauthn button renders with callback', async () => {
const mfa = makeMockState({
webauthnPublicKey: { challenge: new ArrayBuffer(0) },
onWebauthnAuthenticate: jest.fn(),
});
render(<AuthnDialog mfa={mfa} onCancel={mockOnCancel} />);
const webauthn = screen.getByText('Passkey or MFA Device');
fireEvent.click(webauthn);
expect(mfa.onWebauthnAuthenticate).toHaveBeenCalledTimes(1);
});
});
74 changes: 46 additions & 28 deletions web/packages/teleport/src/components/AuthnDialog/AuthnDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -17,46 +17,64 @@
*/

import React from 'react';
import Dialog, {
DialogHeader,
DialogTitle,
DialogContent,
} from 'design/Dialog';
import Dialog, { DialogContent } from 'design/Dialog';
import { Danger } from 'design/Alert';
import { Text, ButtonPrimary, ButtonSecondary, Flex } from 'design';
import { FingerprintSimple, Cross } from 'design/Icon';

import { Text, ButtonSecondary, Flex, ButtonIcon, H2 } from 'design';

import { guessProviderType } from 'shared/components/ButtonSso';
import { SSOIcon } from 'shared/components/ButtonSso/ButtonSso';

import { MfaState } from 'teleport/lib/useMfa';

export default function AuthnDialog({ mfa, onCancel }: Props) {
return (
<Dialog dialogCss={() => ({ width: '500px' })} open={true}>
<DialogHeader style={{ flexDirection: 'column' }}>
<DialogTitle textAlign="center">
Multi-factor authentication
</DialogTitle>
</DialogHeader>
<DialogContent mb={6}>
<Dialog dialogCss={() => ({ width: '400px' })} open={true}>
<Flex justifyContent="space-between" alignItems="center" mb={4}>
<H2>Re-authenticate in the Browser</H2>
<ButtonIcon data-testid="close-dialog" onClick={onCancel}>
<Cross color="text.slightlyMuted" />
</ButtonIcon>
</Flex>
<DialogContent mb={5}>
{mfa.errorText && (
<Danger mt={2} width="100%">
<Danger data-testid="danger-alert" mt={2} width="100%">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, as a side note (not to be addressed in this particular PR): perhaps we should use an alert ARIA role for warning and danger boxes. This would both help making these more accessible, and would help in writing tests. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that'd probably be more helpful

{mfa.errorText}
</Danger>
)}
<Text textAlign="center">
Re-enter your multi-factor authentication in the browser to continue.
<Text textAlign="center" color="text.slightlyMuted">
To continue, you must verify your identity by re-authenticating:
</Text>
</DialogContent>
<Flex textAlign="center" justifyContent="center">
{/* TODO (avatus) this will eventually be conditionally rendered based on what
type of challenges exist. For now, its only webauthn. */}
<ButtonPrimary
onClick={mfa.onWebauthnAuthenticate}
autoFocus
mr={3}
width="130px"
>
{mfa.errorText ? 'Retry' : 'OK'}
</ButtonPrimary>
<ButtonSecondary onClick={onCancel}>Cancel</ButtonSecondary>
<Flex textAlign="center" width="100%" flexDirection="column" gap={2}>
{mfa.ssoChallenge && (
<ButtonSecondary
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we just reuse the entire ButtonSso component here? If it's about size or different icon gap, I'd encourage unifying it instead of creating a slightly customized version everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forget what issue I had using the entire button SSO component but I'll revisit and try again, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh thats right, because the rest of the buttons around this dialog aren't sso. There is only 1 SSO challenge being retreives, and/or a webauthn challenge.

So its either use ButtonSso for the single sso option and also for two non-sso buttons (webauthn and totp. totp isnt in yet tho) or just remake a button in here for this dialog to match. And I'd rather these 3 buttons in this dialog all use the same button instead of ButtonSso for one and a "copy" of its styles for the non sso buttons

size="extra-large"
onClick={mfa.onSsoAuthenticate}
gap={2}
block
>
<SSOIcon
type={guessProviderType(
mfa.ssoChallenge.device.displayName,
mfa.ssoChallenge.device.connectorType
)}
/>
{mfa.ssoChallenge.device.displayName}
</ButtonSecondary>
)}
{mfa.webauthnPublicKey && (
<ButtonSecondary
size="extra-large"
onClick={mfa.onWebauthnAuthenticate}
gap={2}
block
>
<FingerprintSimple />
Passkey or MFA Device
</ButtonSecondary>
)}
</Flex>
</Dialog>
);
Expand Down
14 changes: 13 additions & 1 deletion web/packages/teleport/src/lib/EventEmitterMfaSender.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,25 @@

import { EventEmitter } from 'events';

import { WebauthnAssertionResponse } from 'teleport/services/auth';
import {
MfaChallengeResponse,
WebauthnAssertionResponse,
} from 'teleport/services/auth';

class EventEmitterMfaSender extends EventEmitter {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this class extend EventEmitter if it doesn't emit anything? I'm a bit puzzled here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class gets extended by tty (and desktop client), which emits many things. This prior work seems to be more for typing (perhaps virtual/abstract methods instead but I'll check out the impact of that)

constructor() {
super();
}

// eslint-disable-next-line @typescript-eslint/no-unused-vars
sendChallengeResponse(data: MfaChallengeResponse) {
throw new Error('Not implemented');
}

// TODO (avatus) DELETE IN 18
/**
* @deprecated Use sendChallengeResponse instead.
*/
// eslint-disable-next-line @typescript-eslint/no-unused-vars
sendWebAuthn(data: WebauthnAssertionResponse) {
throw new Error('Not implemented');
Expand Down
2 changes: 1 addition & 1 deletion web/packages/teleport/src/lib/tdp/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ export default class Client extends EventEmitterMfaSender {
try {
const mfaJson = this.codec.decodeMfaJson(buffer);
if (mfaJson.mfaType == 'n') {
this.emit(TermEvent.WEBAUTHN_CHALLENGE, mfaJson.jsonString);
this.emit(TermEvent.MFA_CHALLENGE, mfaJson.jsonString);
} else {
// mfaJson.mfaType === 'u', or else decodeMfaJson would have thrown an error.
this.handleError(
Expand Down
2 changes: 1 addition & 1 deletion web/packages/teleport/src/lib/term/enums.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export enum TermEvent {
SESSION = 'terminal.new_session',
DATA = 'terminal.data',
CONN_CLOSE = 'connection.close',
WEBAUTHN_CHALLENGE = 'terminal.webauthn',
MFA_CHALLENGE = 'terminal.webauthn',
LATENCY = 'terminal.latency',
KUBE_EXEC = 'terminal.kube_exec',
}
Expand Down
4 changes: 2 additions & 2 deletions web/packages/teleport/src/lib/term/protobuf.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export const MessageTypeEnum = {
RESIZE: 'w',
FILE_TRANSFER_REQUEST: 'f',
FILE_TRANSFER_DECISION: 't',
WEBAUTHN_CHALLENGE: 'n',
MFA_CHALLENGE: 'n',
ERROR: 'e',
LATENCY: 'l',
KUBE_EXEC: 'k',
Expand Down Expand Up @@ -59,7 +59,7 @@ export const messageFields = {
data: MessageTypeEnum.RAW.charCodeAt(0),
event: MessageTypeEnum.AUDIT.charCodeAt(0),
close: MessageTypeEnum.SESSION_END.charCodeAt(0),
challengeResponse: MessageTypeEnum.WEBAUTHN_CHALLENGE.charCodeAt(0),
challengeResponse: MessageTypeEnum.MFA_CHALLENGE.charCodeAt(0),
kubeExec: MessageTypeEnum.KUBE_EXEC.charCodeAt(0),
error: MessageTypeEnum.ERROR.charCodeAt(0),
},
Expand Down
31 changes: 28 additions & 3 deletions web/packages/teleport/src/lib/term/tty.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@
import Logger from 'shared/libs/logger';

import { EventEmitterMfaSender } from 'teleport/lib/EventEmitterMfaSender';
import { WebauthnAssertionResponse } from 'teleport/services/auth';
import {
MfaChallengeResponse,
WebauthnAssertionResponse,
} from 'teleport/services/auth';
import { AuthenticatedWebSocket } from 'teleport/lib/AuthenticatedWebSocket';

import { EventType, TermEvent, WebsocketCloseCode } from './enums';
Expand Down Expand Up @@ -80,6 +83,28 @@ class Tty extends EventEmitterMfaSender {
this.socket.send(bytearray.buffer);
}

sendChallengeResponse(data: MfaChallengeResponse) {
// we want to have the backend listen on a single message type
// for any responses. so our data will look like data.webauthn, data.sso, etc
// but to be backward compatible, we need to still spread the existing webauthn only fields
// as "top level" fields so old proxies can still respond to webauthn challenges.
// in 19, we can just pass "data" without this extra step
// TODO (avatus): DELETE IN 18
const backwardCompatibleData = {
...data.webauthn_response,
...data,
};
const encoded = this._proto.encodeChallengeResponse(
JSON.stringify(backwardCompatibleData)
);
const bytearray = new Uint8Array(encoded);
this.socket.send(bytearray);
}

// TODO (avatus) DELETE IN 18
/**
* @deprecated Use sendChallengeResponse instead.
*/
sendWebAuthn(data: WebauthnAssertionResponse) {
const encoded = this._proto.encodeChallengeResponse(JSON.stringify(data));
const bytearray = new Uint8Array(encoded);
Expand Down Expand Up @@ -190,8 +215,8 @@ class Tty extends EventEmitterMfaSender {
const msg = this._proto.decode(uintArray);

switch (msg.type) {
case MessageTypeEnum.WEBAUTHN_CHALLENGE:
this.emit(TermEvent.WEBAUTHN_CHALLENGE, msg.payload);
case MessageTypeEnum.MFA_CHALLENGE:
this.emit(TermEvent.MFA_CHALLENGE, msg.payload);
break;
case MessageTypeEnum.AUDIT:
this._processAuditPayload(msg.payload);
Expand Down
Loading
Loading