From ae4d826fb215e7a6b2038cccf90970027a073bf6 Mon Sep 17 00:00:00 2001 From: headphonejames Date: Fri, 3 Feb 2023 12:03:01 -0800 Subject: [PATCH] Updates to redirect implementation after initial PR review. Signed-off-by: headphonejames --- app-config.yaml | 2 +- packages/app-defaults/src/defaults/apis.ts | 16 +++--- packages/core-app-api/config.d.ts | 6 +- .../OAuthRequestApi/OAuthRequestManager.ts | 6 ++ .../auth/atlassian/AtlassianAuth.ts | 4 +- .../auth/bitbucket/BitbucketAuth.ts | 5 +- .../implementations/auth/github/GithubAuth.ts | 5 +- .../implementations/auth/gitlab/GitlabAuth.ts | 5 +- .../implementations/auth/google/GoogleAuth.ts | 5 +- .../auth/microsoft/MicrosoftAuth.ts | 5 +- .../implementations/auth/oauth2/OAuth2.ts | 4 +- .../implementations/auth/okta/OktaAuth.ts | 5 +- .../auth/onelogin/OneLoginAuth.ts | 7 +-- .../src/apis/implementations/auth/types.ts | 2 +- .../DefaultAuthConnector.test.ts | 40 +------------- .../lib/AuthConnector/DefaultAuthConnector.ts | 55 +++++++++---------- .../src/lib/AuthConnector/types.ts | 1 - .../OAuthRequestDialog/OAuthRequestDialog.tsx | 15 ++--- .../src/layout/SignInPage/commonProvider.tsx | 15 +++-- .../src/layout/SignInPage/providers.tsx | 7 +-- .../src/apis/definitions/OAuthRequestApi.ts | 10 ++++ .../src/apis/definitions/auth.ts | 5 -- .../src/lib/flow/authFlowHelpers.test.ts | 17 ------ .../src/lib/flow/authFlowHelpers.ts | 8 --- .../src/lib/oauth/OAuthAdapter.test.ts | 16 +++--- .../src/lib/oauth/OAuthAdapter.ts | 13 ++--- plugins/auth-backend/src/lib/oauth/types.ts | 1 + plugins/auth-backend/src/providers/types.ts | 2 - plugins/auth-backend/src/service/router.ts | 3 - 29 files changed, 107 insertions(+), 178 deletions(-) diff --git a/app-config.yaml b/app-config.yaml index 1d3bf31083..350b9068e3 100644 --- a/app-config.yaml +++ b/app-config.yaml @@ -293,7 +293,7 @@ auth: # path: my-sessions environment: development - usePopup: false + authFlow: redirect ### Providing an auth.session.secret will enable session support in the auth-backend # session: # secret: custom session secret diff --git a/packages/app-defaults/src/defaults/apis.ts b/packages/app-defaults/src/defaults/apis.ts index 9e71855714..feb4cd6cc8 100644 --- a/packages/app-defaults/src/defaults/apis.ts +++ b/packages/app-defaults/src/defaults/apis.ts @@ -131,7 +131,7 @@ export const apis = [ discoveryApi, oauthRequestApi, environment: configApi.getOptionalString('auth.environment'), - usePopup: configApi.getOptionalBoolean('auth.usePopup'), + authFlow: configApi.getOptionalString('auth.authFlow'), }), }), createApiFactory({ @@ -146,7 +146,7 @@ export const apis = [ discoveryApi, oauthRequestApi, environment: configApi.getOptionalString('auth.environment'), - usePopup: configApi.getOptionalBoolean('auth.usePopup'), + authFlow: configApi.getOptionalString('auth.authFlow'), }), }), createApiFactory({ @@ -162,7 +162,7 @@ export const apis = [ oauthRequestApi, defaultScopes: ['read:user'], environment: configApi.getOptionalString('auth.environment'), - usePopup: configApi.getOptionalBoolean('auth.usePopup'), + authFlow: configApi.getOptionalString('auth.authFlow'), }), }), createApiFactory({ @@ -177,7 +177,7 @@ export const apis = [ discoveryApi, oauthRequestApi, environment: configApi.getOptionalString('auth.environment'), - usePopup: configApi.getOptionalBoolean('auth.usePopup'), + authFlow: configApi.getOptionalString('auth.authFlow'), }), }), createApiFactory({ @@ -192,7 +192,7 @@ export const apis = [ discoveryApi, oauthRequestApi, environment: configApi.getOptionalString('auth.environment'), - usePopup: configApi.getOptionalBoolean('auth.usePopup'), + authFlow: configApi.getOptionalString('auth.authFlow'), }), }), createApiFactory({ @@ -207,7 +207,7 @@ export const apis = [ discoveryApi, oauthRequestApi, environment: configApi.getOptionalString('auth.environment'), - usePopup: configApi.getOptionalBoolean('auth.usePopup'), + authFlow: configApi.getOptionalString('auth.authFlow'), }), }), createApiFactory({ @@ -223,7 +223,7 @@ export const apis = [ oauthRequestApi, defaultScopes: ['team'], environment: configApi.getOptionalString('auth.environment'), - usePopup: configApi.getOptionalBoolean('auth.usePopup'), + authFlow: configApi.getOptionalString('auth.authFlow'), }), }), createApiFactory({ @@ -238,7 +238,7 @@ export const apis = [ discoveryApi, oauthRequestApi, environment: configApi.getOptionalString('auth.environment'), - usePopup: configApi.getOptionalBoolean('auth.usePopup'), + authFlow: configApi.getOptionalString('auth.authFlow'), }); }, }), diff --git a/packages/core-app-api/config.d.ts b/packages/core-app-api/config.d.ts index 0dd73369f6..c499dfa7c6 100644 --- a/packages/core-app-api/config.d.ts +++ b/packages/core-app-api/config.d.ts @@ -115,10 +115,10 @@ export interface Config { */ environment?: string; /** - $ The authentication flow type - either using a popup to authenticate or perform a http redirect - * default value: 'true' + $ The authentication flow type - currently supports 'popup' and 'redirect' + * default value: 'popup' * @visibility frontend */ - usePopup?: boolean; + authFlow?: string; }; } diff --git a/packages/core-app-api/src/apis/implementations/OAuthRequestApi/OAuthRequestManager.ts b/packages/core-app-api/src/apis/implementations/OAuthRequestApi/OAuthRequestManager.ts index 5d467be7b4..751f2bf24c 100644 --- a/packages/core-app-api/src/apis/implementations/OAuthRequestApi/OAuthRequestManager.ts +++ b/packages/core-app-api/src/apis/implementations/OAuthRequestApi/OAuthRequestManager.ts @@ -37,8 +37,10 @@ export class OAuthRequestManager implements OAuthRequestApi { private readonly subject = new BehaviorSubject([]); private currentRequests: PendingOAuthRequest[] = []; private handlerCount = 0; + private authFlowStr: string = 'popup'; createAuthRequester(options: OAuthRequesterOptions): OAuthRequester { + this.authFlowStr = options.authFlow; const handler = new OAuthPendingRequests(); const index = this.handlerCount; @@ -91,4 +93,8 @@ export class OAuthRequestManager implements OAuthRequestApi { authRequest$(): Observable { return this.subject; } + + authFlow(): string { + return this.authFlowStr; + } } diff --git a/packages/core-app-api/src/apis/implementations/auth/atlassian/AtlassianAuth.ts b/packages/core-app-api/src/apis/implementations/auth/atlassian/AtlassianAuth.ts index 5582a29c00..df714e2290 100644 --- a/packages/core-app-api/src/apis/implementations/auth/atlassian/AtlassianAuth.ts +++ b/packages/core-app-api/src/apis/implementations/auth/atlassian/AtlassianAuth.ts @@ -34,7 +34,7 @@ export default class AtlassianAuth { const { discoveryApi, environment = 'development', - usePopup = true, + authFlow = 'popup', provider = DEFAULT_PROVIDER, oauthRequestApi, } = options; @@ -44,7 +44,7 @@ export default class AtlassianAuth { oauthRequestApi, provider, environment, - usePopup, + authFlow, }); } } diff --git a/packages/core-app-api/src/apis/implementations/auth/bitbucket/BitbucketAuth.ts b/packages/core-app-api/src/apis/implementations/auth/bitbucket/BitbucketAuth.ts index 1d27eb1aa3..3bdb09b010 100644 --- a/packages/core-app-api/src/apis/implementations/auth/bitbucket/BitbucketAuth.ts +++ b/packages/core-app-api/src/apis/implementations/auth/bitbucket/BitbucketAuth.ts @@ -36,7 +36,6 @@ export type BitbucketAuthResponse = { const DEFAULT_PROVIDER = { id: 'bitbucket', title: 'Bitbucket', - provider_id: 'bitbucket-auth-provider', icon: () => null, }; @@ -50,7 +49,7 @@ export default class BitbucketAuth { const { discoveryApi, environment = 'development', - usePopup = true, + authFlow = 'popup', provider = DEFAULT_PROVIDER, oauthRequestApi, defaultScopes = ['team'], @@ -61,7 +60,7 @@ export default class BitbucketAuth { oauthRequestApi, provider, environment, - usePopup, + authFlow, defaultScopes, }); } diff --git a/packages/core-app-api/src/apis/implementations/auth/github/GithubAuth.ts b/packages/core-app-api/src/apis/implementations/auth/github/GithubAuth.ts index 11af9995a1..5acdd20719 100644 --- a/packages/core-app-api/src/apis/implementations/auth/github/GithubAuth.ts +++ b/packages/core-app-api/src/apis/implementations/auth/github/GithubAuth.ts @@ -21,7 +21,6 @@ import { OAuthApiCreateOptions } from '../types'; const DEFAULT_PROVIDER = { id: 'github', title: 'GitHub', - provider_id: 'github-auth-provider', icon: () => null, }; @@ -38,7 +37,7 @@ export default class GithubAuth { provider = DEFAULT_PROVIDER, oauthRequestApi, defaultScopes = ['read:user'], - usePopup = true, + authFlow = 'popup', } = options; return OAuth2.create({ @@ -47,7 +46,7 @@ export default class GithubAuth { provider, environment, defaultScopes, - usePopup, + authFlow, }); } } diff --git a/packages/core-app-api/src/apis/implementations/auth/gitlab/GitlabAuth.ts b/packages/core-app-api/src/apis/implementations/auth/gitlab/GitlabAuth.ts index 965d0431ac..919d67dd8d 100644 --- a/packages/core-app-api/src/apis/implementations/auth/gitlab/GitlabAuth.ts +++ b/packages/core-app-api/src/apis/implementations/auth/gitlab/GitlabAuth.ts @@ -21,7 +21,6 @@ import { OAuthApiCreateOptions } from '../types'; const DEFAULT_PROVIDER = { id: 'gitlab', title: 'GitLab', - provider_id: 'gitlab-auth-provider', icon: () => null, }; @@ -35,7 +34,7 @@ export default class GitlabAuth { const { discoveryApi, environment = 'development', - usePopup = true, + authFlow = 'popup', provider = DEFAULT_PROVIDER, oauthRequestApi, defaultScopes = ['read_user'], @@ -46,7 +45,7 @@ export default class GitlabAuth { oauthRequestApi, provider, environment, - usePopup, + authFlow, defaultScopes, }); } diff --git a/packages/core-app-api/src/apis/implementations/auth/google/GoogleAuth.ts b/packages/core-app-api/src/apis/implementations/auth/google/GoogleAuth.ts index 514b257ada..e8adefb08f 100644 --- a/packages/core-app-api/src/apis/implementations/auth/google/GoogleAuth.ts +++ b/packages/core-app-api/src/apis/implementations/auth/google/GoogleAuth.ts @@ -21,7 +21,6 @@ import { OAuthApiCreateOptions } from '../types'; const DEFAULT_PROVIDER = { id: 'google', title: 'Google', - provider_id: 'google-auth-provider', icon: () => null, }; @@ -38,7 +37,7 @@ export default class GoogleAuth { discoveryApi, oauthRequestApi, environment = 'development', - usePopup = true, + authFlow = 'popup', provider = DEFAULT_PROVIDER, defaultScopes = [ 'openid', @@ -52,7 +51,7 @@ export default class GoogleAuth { oauthRequestApi, provider, environment, - usePopup, + authFlow, defaultScopes, scopeTransform(scopes: string[]) { return scopes.map(scope => { diff --git a/packages/core-app-api/src/apis/implementations/auth/microsoft/MicrosoftAuth.ts b/packages/core-app-api/src/apis/implementations/auth/microsoft/MicrosoftAuth.ts index e988a79268..c28e115950 100644 --- a/packages/core-app-api/src/apis/implementations/auth/microsoft/MicrosoftAuth.ts +++ b/packages/core-app-api/src/apis/implementations/auth/microsoft/MicrosoftAuth.ts @@ -21,7 +21,6 @@ import { OAuthApiCreateOptions } from '../types'; const DEFAULT_PROVIDER = { id: 'microsoft', title: 'Microsoft', - provider_id: 'microsoft-auth-provider', icon: () => null, }; @@ -34,7 +33,7 @@ export default class MicrosoftAuth { static create(options: OAuthApiCreateOptions): typeof microsoftAuthApiRef.T { const { environment = 'development', - usePopup = true, + authFlow = 'popup', provider = DEFAULT_PROVIDER, oauthRequestApi, discoveryApi, @@ -52,7 +51,7 @@ export default class MicrosoftAuth { oauthRequestApi, provider, environment, - usePopup, + authFlow, defaultScopes, }); } diff --git a/packages/core-app-api/src/apis/implementations/auth/oauth2/OAuth2.ts b/packages/core-app-api/src/apis/implementations/auth/oauth2/OAuth2.ts index a1d0c7d3a9..7a133154f2 100644 --- a/packages/core-app-api/src/apis/implementations/auth/oauth2/OAuth2.ts +++ b/packages/core-app-api/src/apis/implementations/auth/oauth2/OAuth2.ts @@ -77,7 +77,7 @@ export default class OAuth2 provider = DEFAULT_PROVIDER, oauthRequestApi, defaultScopes = [], - usePopup = true, + authFlow = 'popup', scopeTransform = x => x, } = options; @@ -86,7 +86,7 @@ export default class OAuth2 environment, provider, oauthRequestApi: oauthRequestApi, - usePopup, + authFlow, sessionTransform(res: OAuth2Response): OAuth2Session { return { ...res, diff --git a/packages/core-app-api/src/apis/implementations/auth/okta/OktaAuth.ts b/packages/core-app-api/src/apis/implementations/auth/okta/OktaAuth.ts index db574e7fb2..9075d8cb82 100644 --- a/packages/core-app-api/src/apis/implementations/auth/okta/OktaAuth.ts +++ b/packages/core-app-api/src/apis/implementations/auth/okta/OktaAuth.ts @@ -21,7 +21,6 @@ import { OAuthApiCreateOptions } from '../types'; const DEFAULT_PROVIDER = { id: 'okta', title: 'Okta', - provider_id: 'okta-auth-provider', icon: () => null, }; @@ -47,7 +46,7 @@ export default class OktaAuth { const { discoveryApi, environment = 'development', - usePopup = true, + authFlow = 'popup', provider = DEFAULT_PROVIDER, oauthRequestApi, defaultScopes = ['openid', 'email', 'profile', 'offline_access'], @@ -58,7 +57,7 @@ export default class OktaAuth { oauthRequestApi, provider, environment, - usePopup, + authFlow, defaultScopes, scopeTransform(scopes) { return scopes.map(scope => { diff --git a/packages/core-app-api/src/apis/implementations/auth/onelogin/OneLoginAuth.ts b/packages/core-app-api/src/apis/implementations/auth/onelogin/OneLoginAuth.ts index d767cb6b55..edfcbc4463 100644 --- a/packages/core-app-api/src/apis/implementations/auth/onelogin/OneLoginAuth.ts +++ b/packages/core-app-api/src/apis/implementations/auth/onelogin/OneLoginAuth.ts @@ -30,14 +30,13 @@ export type OneLoginAuthCreateOptions = { discoveryApi: DiscoveryApi; oauthRequestApi: OAuthRequestApi; environment?: string; - usePopup?: boolean; + authFlow?: string; provider?: AuthProviderInfo; }; const DEFAULT_PROVIDER = { id: 'onelogin', title: 'onelogin', - provider_id: 'onelogin-auth-provider', icon: () => null, }; @@ -65,7 +64,7 @@ export default class OneLoginAuth { const { discoveryApi, environment = 'development', - usePopup = true, + authFlow = 'popup', provider = DEFAULT_PROVIDER, oauthRequestApi, } = options; @@ -75,7 +74,7 @@ export default class OneLoginAuth { oauthRequestApi, provider, environment, - usePopup, + authFlow: authFlow, defaultScopes: ['openid', 'email', 'profile', 'offline_access'], scopeTransform(scopes) { return scopes.map(scope => { diff --git a/packages/core-app-api/src/apis/implementations/auth/types.ts b/packages/core-app-api/src/apis/implementations/auth/types.ts index e29fe0c7d2..fdb2bbc7f8 100644 --- a/packages/core-app-api/src/apis/implementations/auth/types.ts +++ b/packages/core-app-api/src/apis/implementations/auth/types.ts @@ -37,5 +37,5 @@ export type AuthApiCreateOptions = { discoveryApi: DiscoveryApi; environment?: string; provider?: AuthProviderInfo; - usePopup?: boolean; + authFlow?: string; }; diff --git a/packages/core-app-api/src/lib/AuthConnector/DefaultAuthConnector.test.ts b/packages/core-app-api/src/lib/AuthConnector/DefaultAuthConnector.test.ts index 113ee95bd0..fd2a2833a8 100644 --- a/packages/core-app-api/src/lib/AuthConnector/DefaultAuthConnector.test.ts +++ b/packages/core-app-api/src/lib/AuthConnector/DefaultAuthConnector.test.ts @@ -34,16 +34,15 @@ const defaultOptions = { provider: { id: 'my-provider', title: 'My Provider', - provider_id: 'myprovider-auth-provider', icon: () => null, }, - usePopup: true, oauthRequestApi: new MockOAuthApi(), sessionTransform: ({ expiresInSeconds, ...res }: any) => ({ ...res, scopes: new Set(res.scopes.split(' ')), expiresAt: new Date(Date.now() + expiresInSeconds * 1000), }), + authFlow: 'popup', }; describe('DefaultAuthConnector', () => { @@ -133,7 +132,7 @@ describe('DefaultAuthConnector', () => { expect(popupSpy).toHaveBeenCalledTimes(1); expect(popupSpy.mock.calls[0][0]).toMatchObject({ - url: 'http://my-host/api/auth/my-provider/start?scope=a%20b&origin=http%3A%2F%2Flocalhost&env=production', + url: 'http://my-host/api/auth/my-provider/start?scope=a%20b&origin=http%3A%2F%2Flocalhost&authFlow=popup&env=production', }); await expect(sessionPromise).resolves.toEqual({ @@ -181,40 +180,7 @@ describe('DefaultAuthConnector', () => { expect(popupSpy).toHaveBeenCalledTimes(1); expect(popupSpy.mock.calls[0][0]).toMatchObject({ - url: 'http://my-host/api/auth/my-provider/start?scope=-ab-&origin=http%3A%2F%2Flocalhost&env=production', + url: 'http://my-host/api/auth/my-provider/start?scope=-ab-&origin=http%3A%2F%2Flocalhost&authFlow=popup&env=production', }); }); - - it('should redirect to api server', async () => { - const mockOauth = new MockOAuthApi(); - const mockResponse = jest.fn(); - // replace the window.location object - Object.defineProperty(window, 'location', { - value: { - hash: { - endsWith: mockResponse, - includes: mockResponse, - }, - assign: mockResponse, - }, - writable: true, - }); - const helper = new DefaultAuthConnector({ - ...defaultOptions, - usePopup: false, - oauthRequestApi: mockOauth, - }); - - const sessionPromise = helper.createSession({ - scopes: new Set(['a', 'b']), - }); - - await mockOauth.triggerAll(); - - await expect(sessionPromise).resolves.toEqual({}); - // redirect to the auth api - expect(window.location.href).toMatch( - 'http://my-host/api/auth/my-provider/start?scope=a%20b&authType=redirect&env=production', - ); - }); }); diff --git a/packages/core-app-api/src/lib/AuthConnector/DefaultAuthConnector.ts b/packages/core-app-api/src/lib/AuthConnector/DefaultAuthConnector.ts index acc6b97b2c..7cada8b0f9 100644 --- a/packages/core-app-api/src/lib/AuthConnector/DefaultAuthConnector.ts +++ b/packages/core-app-api/src/lib/AuthConnector/DefaultAuthConnector.ts @@ -32,10 +32,6 @@ type Options = { * Environment hint passed on to auth backend, for example 'production' or 'development' */ environment: string; - /** - * use a popup or a redirect for authentication with backend authentication api - */ - usePopup: boolean; /** * Information about the auth provider to be shown to the user. * The ID Must match the backend auth plugin configuration, for example 'google'. @@ -53,6 +49,10 @@ type Options = { * Function used to transform an auth response into the session type. */ sessionTransform?(response: any): AuthSession | Promise; + /** + * The UI authentication flow with backend authentication api. Supports either 'popup' or 'redirect'. + */ + authFlow: string; }; function defaultJoinScopes(scopes: Set) { @@ -73,6 +73,7 @@ export class DefaultAuthConnector private readonly joinScopesFunc: (scopes: Set) => string; private readonly authRequester: OAuthRequester; private readonly sessionTransform: (response: any) => Promise; + private readonly authFlow: string; constructor(options: Options) { const { @@ -81,39 +82,22 @@ export class DefaultAuthConnector provider, joinScopes = defaultJoinScopes, oauthRequestApi, - usePopup, + authFlow, sessionTransform = id => id, } = options; this.authRequester = oauthRequestApi.createAuthRequester({ provider, onAuthRequest: async scopes => { - if (!usePopup) { - // modal before redirect - const scope = this.joinScopesFunc(scopes); - const redirectUrl = await this.buildUrl('/start', { - scope, - origin: window.location.origin, - redirectUrl: window.location.href, - authType: 'redirect', - }); - - if (provider.hasOwnProperty('provider_id')) { - // set the sign in provider here - localStorage.setItem( - '@backstage/core:SignInPage:provider', - provider.provider_id!, - ); - } - - window.location.href = redirectUrl; - // we need to return to exit function or else popup occurs - return Promise.resolve({} as AuthSession); + if (authFlow === 'popup') { + return this.showPopup(scopes); } - return this.showPopup(scopes); + return this.executeRedirect(scopes); }, + authFlow, }); + this.authFlow = authFlow; this.discoveryApi = discoveryApi; this.environment = environment; this.provider = provider; @@ -122,7 +106,7 @@ export class DefaultAuthConnector } async createSession(options: CreateSessionOptions): Promise { - if (options.instantPopup) { + if (options.instantPopup && this.authFlow === 'popup') { return this.showPopup(options.scopes); } return this.authRequester(options.scopes); @@ -184,6 +168,7 @@ export class DefaultAuthConnector const popupUrl = await this.buildUrl('/start', { scope, origin: window.location.origin, + authFlow: 'popup', }); const payload = await showLoginPopup({ @@ -197,6 +182,20 @@ export class DefaultAuthConnector return await this.sessionTransform(payload); } + private async executeRedirect(scopes: Set): Promise { + const scope = this.joinScopesFunc(scopes); + const redirectUrl = await this.buildUrl('/start', { + scope, + origin: window.location.origin, + redirectUrl: window.location.href, + authFlow: 'redirect', + }); + // redirect to auth api + window.location.href = redirectUrl; + // return a promise that never resolves + return new Promise(() => {}); + } + private async buildUrl( path: string, query?: { [key: string]: string | boolean | undefined }, diff --git a/packages/core-app-api/src/lib/AuthConnector/types.ts b/packages/core-app-api/src/lib/AuthConnector/types.ts index 7c4f19184e..464a2ed627 100644 --- a/packages/core-app-api/src/lib/AuthConnector/types.ts +++ b/packages/core-app-api/src/lib/AuthConnector/types.ts @@ -17,7 +17,6 @@ export type CreateSessionOptions = { scopes: Set; instantPopup?: boolean; - redirectUrl?: string; }; /** diff --git a/packages/core-components/src/components/OAuthRequestDialog/OAuthRequestDialog.tsx b/packages/core-components/src/components/OAuthRequestDialog/OAuthRequestDialog.tsx index 1884e6e358..e68120298a 100644 --- a/packages/core-components/src/components/OAuthRequestDialog/OAuthRequestDialog.tsx +++ b/packages/core-components/src/components/OAuthRequestDialog/OAuthRequestDialog.tsx @@ -24,11 +24,7 @@ import Button from '@material-ui/core/Button'; import React, { useMemo, useState } from 'react'; import useObservable from 'react-use/lib/useObservable'; import LoginRequestListItem from './LoginRequestListItem'; -import { - useApi, - oauthRequestApiRef, - configApiRef, -} from '@backstage/core-plugin-api'; +import { useApi, oauthRequestApiRef } from '@backstage/core-plugin-api'; import Typography from '@material-ui/core/Typography'; export type OAuthRequestDialogClassKey = @@ -62,11 +58,10 @@ export function OAuthRequestDialog(_props: {}) { const classes = useStyles(); const [busy, setBusy] = useState(false); const oauthRequestApi = useApi(oauthRequestApiRef); - const configApi = useApi(configApiRef); - const usePopup = configApi.getOptionalBoolean('auth.usePopup') ?? true; - const redirectMessage = usePopup - ? '' - : 'This will trigger a http redirect to OAuth Login.'; + const redirectMessage = + oauthRequestApi.authFlow() === 'popup' + ? '' + : 'This will trigger a http redirect to OAuth Login.'; const requests = useObservable( useMemo(() => oauthRequestApi.authRequest$(), [oauthRequestApi]), diff --git a/packages/core-components/src/layout/SignInPage/commonProvider.tsx b/packages/core-components/src/layout/SignInPage/commonProvider.tsx index aa458692dd..6796e3f22d 100644 --- a/packages/core-components/src/layout/SignInPage/commonProvider.tsx +++ b/packages/core-components/src/layout/SignInPage/commonProvider.tsx @@ -24,27 +24,25 @@ import { SignInProvider, SignInProviderConfig, } from './types'; -import { useApi, errorApiRef, configApiRef } from '@backstage/core-plugin-api'; +import { useApi, errorApiRef } from '@backstage/core-plugin-api'; import { GridItem } from './styles'; import { ForwardedError } from '@backstage/errors'; import { UserIdentity } from './UserIdentity'; +import { PROVIDER_STORAGE_KEY } from './providers'; const Component: ProviderComponent = ({ config, onSignInSuccess }) => { - const { apiRef, title, message } = config as SignInProviderConfig; + const { apiRef, title, message, id } = config as SignInProviderConfig; const authApi = useApi(apiRef); const errorApi = useApi(errorApiRef); - const configApi = useApi(configApiRef); - const usePopup = configApi.getOptionalBoolean('auth.usePopup') ?? true; const handleLogin = async () => { try { + localStorage.setItem(PROVIDER_STORAGE_KEY, id); const identityResponse = await authApi.getBackstageIdentity({ - instantPopup: usePopup, + instantPopup: true, }); if (!identityResponse) { - if (!usePopup) { - return; - } + localStorage.removeItem(PROVIDER_STORAGE_KEY); throw new Error( `The ${title} provider is not configured to support sign-in`, ); @@ -60,6 +58,7 @@ const Component: ProviderComponent = ({ config, onSignInSuccess }) => { }), ); } catch (error) { + localStorage.removeItem(PROVIDER_STORAGE_KEY); errorApi.post(new ForwardedError('Login failed', error)); } }; diff --git a/packages/core-components/src/layout/SignInPage/providers.tsx b/packages/core-components/src/layout/SignInPage/providers.tsx index 23d6c95b3c..716befda17 100644 --- a/packages/core-components/src/layout/SignInPage/providers.tsx +++ b/packages/core-components/src/layout/SignInPage/providers.tsx @@ -32,7 +32,7 @@ import { guestProvider } from './guestProvider'; import { customProvider } from './customProvider'; import { IdentityApiSignOutProxy } from './IdentityApiSignOutProxy'; -const PROVIDER_STORAGE_KEY = '@backstage/core:SignInPage:provider'; +export const PROVIDER_STORAGE_KEY = '@backstage/core:SignInPage:provider'; export type SignInProviderType = { [key: string]: { @@ -134,6 +134,7 @@ export const useSignInProviders = ( .loader(apiHolder, provider.config?.apiRef!) .then(result => { if (didCancel) { + localStorage.removeItem(PROVIDER_STORAGE_KEY); return; } if (result) { @@ -143,10 +144,10 @@ export const useSignInProviders = ( } }) .catch(error => { + localStorage.removeItem(PROVIDER_STORAGE_KEY); if (didCancel) { return; } - localStorage.removeItem(PROVIDER_STORAGE_KEY); errorApi.post(error); setLoading(false); }); @@ -172,8 +173,6 @@ export const useSignInProviders = ( const { Component } = provider.components; const handleSignInSuccess = (result: IdentityApi) => { - localStorage.setItem(PROVIDER_STORAGE_KEY, provider.id); - handleWrappedResult(result); }; diff --git a/packages/core-plugin-api/src/apis/definitions/OAuthRequestApi.ts b/packages/core-plugin-api/src/apis/definitions/OAuthRequestApi.ts index 75bf3a3864..2027852de7 100644 --- a/packages/core-plugin-api/src/apis/definitions/OAuthRequestApi.ts +++ b/packages/core-plugin-api/src/apis/definitions/OAuthRequestApi.ts @@ -35,6 +35,11 @@ export type OAuthRequesterOptions = { * trigger() is called on an auth requests. */ onAuthRequest(scopes: Set): Promise; + + /** + * The authentication flow type + */ + authFlow: string; }; /** @@ -119,6 +124,11 @@ export type OAuthRequestApi = { * AuthRequester calls will resolve to the value returned by the onAuthRequest call. */ authRequest$(): Observable; + + /** + * The authentication flow type + */ + authFlow(): string; }; /** diff --git a/packages/core-plugin-api/src/apis/definitions/auth.ts b/packages/core-plugin-api/src/apis/definitions/auth.ts index 6ae19d53a6..3ab8c98fee 100644 --- a/packages/core-plugin-api/src/apis/definitions/auth.ts +++ b/packages/core-plugin-api/src/apis/definitions/auth.ts @@ -54,11 +54,6 @@ export type AuthProviderInfo = { * Icon for the auth provider. */ icon: IconComponent; - - /** - * The provider id from the list of provider configured in the login page - */ - provider_id?: string; }; /** diff --git a/plugins/auth-backend/src/lib/flow/authFlowHelpers.test.ts b/plugins/auth-backend/src/lib/flow/authFlowHelpers.test.ts index 868477506a..a0c437815d 100644 --- a/plugins/auth-backend/src/lib/flow/authFlowHelpers.test.ts +++ b/plugins/auth-backend/src/lib/flow/authFlowHelpers.test.ts @@ -19,7 +19,6 @@ import { safelyEncodeURIComponent, ensuresXRequestedWith, postMessageResponse, - redirectMessageResponse, } from './authFlowHelpers'; import { WebMessageResponse } from './types'; @@ -179,22 +178,6 @@ describe('oauth helpers', () => { }); }); - describe('redirectMessageResponse', () => { - const redirectUrl = 'http://localhost:3000/catalog'; - it('should perform redirect', () => { - const mockResponse = { - end: jest.fn().mockReturnThis(), - setHeader: jest.fn().mockReturnThis(), - redirect: jest.fn().mockReturnThis(), - } as unknown as express.Response; - - redirectMessageResponse(mockResponse, redirectUrl); - expect(mockResponse.redirect).toHaveBeenCalledTimes(1); - expect(mockResponse.end).not.toHaveBeenCalled(); - expect(mockResponse.setHeader).not.toHaveBeenCalled(); - }); - }); - describe('ensuresXRequestedWith', () => { it('should return false if no header present', () => { const mockRequest = { diff --git a/plugins/auth-backend/src/lib/flow/authFlowHelpers.ts b/plugins/auth-backend/src/lib/flow/authFlowHelpers.ts index 08449fd923..94ac4fba15 100644 --- a/plugins/auth-backend/src/lib/flow/authFlowHelpers.ts +++ b/plugins/auth-backend/src/lib/flow/authFlowHelpers.ts @@ -69,14 +69,6 @@ export const postMessageResponse = ( res.end(``); }; -/** @public */ -export const redirectMessageResponse = ( - res: express.Response, - redirectUrl: string, -) => { - res.redirect(redirectUrl); -}; - /** @public */ export const ensuresXRequestedWith = (req: express.Request) => { const requiredHeader = req.header('X-Requested-With'); diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts index 31d7e5aff5..dd45ab5441 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts @@ -74,7 +74,6 @@ describe('OAuthAdapter', () => { providerId: 'test-provider', appOrigin: 'http://localhost:3000', baseUrl: 'http://domain.org/auth', - isPopupAuthenticationRequest: true, cookieConfigurer: mockCookieConfigurer, tokenIssuer: { issueToken: async () => 'my-id-token', @@ -174,10 +173,14 @@ describe('OAuthAdapter', () => { const oauthProvider = new OAuthAdapter(providerInstance, { ...oAuthProviderOptions, isOriginAllowed: () => false, - isPopupAuthenticationRequest: false, }); - const mockRequest = createEncodedQueryMockRequest(defaultState); + const state = { + ...defaultState, + redirectUrl: 'http://localhost:3000', + authFlow: 'redirect', + }; + const mockRequest = createEncodedQueryMockRequest(state); await oauthProvider.frameHandler(mockRequest, mockResponse); expect(mockResponse.redirect).toHaveBeenCalledTimes(1); @@ -343,7 +346,6 @@ describe('OAuthAdapter', () => { baseUrl: 'http://domain.org/auth', appUrl: 'http://domain.org', isOriginAllowed: () => false, - isPopupAuthenticationRequest: true, }; const oauthProvider = OAuthAdapter.fromConfig(config, providerInstance, { @@ -365,7 +367,6 @@ describe('OAuthAdapter', () => { baseUrl: 'http://domain.org/auth', appUrl: 'http://domain.org', isOriginAllowed: () => false, - isPopupAuthenticationRequest: true, }; const mockStartRequestWithOrigin = { @@ -505,7 +506,7 @@ describe('OAuthAdapter', () => { expect(mockResponse.redirect).not.toHaveBeenCalled(); }); - it('executed a response redirect when isPopupAuthenticationRequest is false', async () => { + it('executed a response redirect when authFlow query string is set to "redirect"', async () => { const handlers = { start: jest.fn(async (_req: { state: OAuthState }) => ({ url: '/url', @@ -516,7 +517,6 @@ describe('OAuthAdapter', () => { }; const configWithNoPopupEnabled = { ...configOriginAllowed, - isPopupAuthenticationRequest: false, }; const oauthProvider = OAuthAdapter.fromConfig( configWithNoPopupEnabled, @@ -531,6 +531,7 @@ describe('OAuthAdapter', () => { ...defaultState, origin: 'http://other.domain', redirectUrl: 'http://domain.org', + authFlow: 'redirect', }; const mockRequest = { @@ -540,7 +541,6 @@ describe('OAuthAdapter', () => { await oauthProvider.frameHandler(mockRequest, mockResponse); expect(mockRequest.get).not.toHaveBeenCalled(); - expect(mockResponse.end).not.toHaveBeenCalled(); expect(mockCookieConfigurer).not.toHaveBeenCalled(); expect(mockResponse.cookie).not.toHaveBeenCalled(); expect(mockResponse.redirect).toHaveBeenCalledTimes(1); diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts index 239e660298..ac55fa8ad1 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts @@ -35,7 +35,6 @@ import { import { defaultCookieConfigurer, readState, verifyNonce } from './helpers'; import { postMessageResponse, - redirectMessageResponse, ensuresXRequestedWith, WebMessageResponse, } from '../flow'; @@ -61,7 +60,6 @@ export type OAuthAdapterOptions = { cookieConfigurer: CookieConfigurer; isOriginAllowed: (origin: string) => boolean; callbackUrl: string; - isPopupAuthenticationRequest: boolean; }; /** @public */ @@ -74,8 +72,7 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { 'providerId' | 'persistScopes' | 'callbackUrl' | 'redirectUrl' >, ): OAuthAdapter { - const { appUrl, baseUrl, isOriginAllowed, isPopupAuthenticationRequest } = - config; + const { appUrl, baseUrl, isOriginAllowed } = config; const { origin: appOrigin } = new URL(appUrl); const cookieConfigurer = config.cookieConfigurer ?? defaultCookieConfigurer; @@ -86,7 +83,6 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { baseUrl, cookieConfigurer, isOriginAllowed, - isPopupAuthenticationRequest: isPopupAuthenticationRequest, }); } @@ -108,6 +104,7 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { const env = req.query.env?.toString(); const origin = req.query.origin?.toString(); const redirectUrl = req.query.redirectUrl?.toString(); + const authFlow = req.query.authFlow?.toString(); if (!env) { throw new InputError('No env provided in request query parameters'); } @@ -118,7 +115,7 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { // set a nonce cookie before redirecting to oauth provider this.setNonceCookie(res, nonce, cookieConfig); - const state: OAuthState = { nonce, env, origin, redirectUrl }; + const state: OAuthState = { nonce, env, origin, redirectUrl, authFlow }; // If scopes are persisted then we pass them through the state so that we // can set the cookie on successful auth @@ -184,8 +181,8 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { response: { ...response, backstageIdentity: identity }, }; - if (!this.options.isPopupAuthenticationRequest) { - return redirectMessageResponse(res, redirectUrl); + if (state.authFlow === 'redirect') { + res.redirect(redirectUrl); } // post message back to popup if successful return postMessageResponse(res, appOrigin, responseObj); diff --git a/plugins/auth-backend/src/lib/oauth/types.ts b/plugins/auth-backend/src/lib/oauth/types.ts index 2b06e1f78b..4cb7a94ffd 100644 --- a/plugins/auth-backend/src/lib/oauth/types.ts +++ b/plugins/auth-backend/src/lib/oauth/types.ts @@ -91,6 +91,7 @@ export type OAuthState = { origin?: string; scope?: string; redirectUrl?: string; + authFlow?: string; }; /** @public */ diff --git a/plugins/auth-backend/src/providers/types.ts b/plugins/auth-backend/src/providers/types.ts index f70b639de9..1459cff484 100644 --- a/plugins/auth-backend/src/providers/types.ts +++ b/plugins/auth-backend/src/providers/types.ts @@ -131,8 +131,6 @@ export type AuthProviderConfig = { * The function used to resolve cookie configuration based on the auth provider options. */ cookieConfigurer?: CookieConfigurer; - - isPopupAuthenticationRequest: boolean; }; /** @public */ diff --git a/plugins/auth-backend/src/service/router.ts b/plugins/auth-backend/src/service/router.ts index f19eb54a72..9440a2c4ed 100644 --- a/plugins/auth-backend/src/service/router.ts +++ b/plugins/auth-backend/src/service/router.ts @@ -107,8 +107,6 @@ export async function createRouter( ...providerFactories, }; const providersConfig = config.getConfig('auth.providers'); - const isPopupAuthenticationRequest = - config.getOptionalBoolean('auth.usePopup') ?? true; const configuredProviders = providersConfig.keys(); @@ -126,7 +124,6 @@ export async function createRouter( baseUrl: authUrl, appUrl, isOriginAllowed, - isPopupAuthenticationRequest: isPopupAuthenticationRequest, }, config: providersConfig.getConfig(providerId), logger,