diff --git a/.changeset/blue-lions-greet.md b/.changeset/blue-lions-greet.md new file mode 100644 index 0000000000..c4f4d2a337 --- /dev/null +++ b/.changeset/blue-lions-greet.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-user-settings': patch +--- + +Handle errors that may occur when the user logs out diff --git a/.changeset/hot-geese-vanish.md b/.changeset/hot-geese-vanish.md new file mode 100644 index 0000000000..b392c875c7 --- /dev/null +++ b/.changeset/hot-geese-vanish.md @@ -0,0 +1,5 @@ +--- +'@backstage/plugin-auth-backend': minor +--- + +Google OAuth refresh tokens will now be revoked on logout by calling Google's API diff --git a/plugins/auth-backend/api-report.md b/plugins/auth-backend/api-report.md index ea5b923baa..cc6962cdbe 100644 --- a/plugins/auth-backend/api-report.md +++ b/plugins/auth-backend/api-report.md @@ -314,7 +314,7 @@ export interface OAuthHandlers { response: OAuthResponse; refreshToken?: string; }>; - logout?(): Promise; + logout?(req: OAuthLogoutRequest): Promise; refresh?(req: OAuthRefreshRequest): Promise<{ response: OAuthResponse; refreshToken?: string; @@ -322,6 +322,11 @@ export interface OAuthHandlers { start(req: OAuthStartRequest): Promise; } +// @public (undocumented) +export type OAuthLogoutRequest = express.Request<{}> & { + refreshToken: string; +}; + // @public (undocumented) export type OAuthProviderInfo = { accessToken: string; diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts index 526b05d0ab..e0d4f0e051 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.test.ts @@ -17,7 +17,7 @@ import express from 'express'; import { THOUSAND_DAYS_MS, TEN_MINUTES_MS, OAuthAdapter } from './OAuthAdapter'; import { encodeState } from './helpers'; -import { OAuthHandlers, OAuthState } from './types'; +import { OAuthHandlers, OAuthLogoutRequest, OAuthState } from './types'; import { CookieConfigurer } from '../../providers/types'; const mockResponseData = { @@ -60,6 +60,7 @@ describe('OAuthAdapter', () => { refreshToken: 'token', }; } + async logout(_: OAuthLogoutRequest) {} } const providerInstance = new MyAuthProvider(); const mockCookieConfig: ReturnType = { @@ -262,13 +263,17 @@ describe('OAuthAdapter', () => { ); }); - it('removes refresh cookie when logging out', async () => { + it('removes refresh cookie and calls logout handler when logging out', async () => { + const logoutSpy = jest.spyOn(providerInstance, 'logout'); const oauthProvider = new OAuthAdapter(providerInstance, { ...oAuthProviderOptions, isOriginAllowed: () => false, }); const mockRequest = { + cookies: { + 'test-provider-refresh-token': 'token', + }, header: () => 'XMLHttpRequest', get: jest.fn(), } as unknown as express.Request; @@ -281,6 +286,7 @@ describe('OAuthAdapter', () => { await oauthProvider.logout(mockRequest, mockResponse); expect(mockRequest.get).toHaveBeenCalledTimes(1); + expect(logoutSpy).toHaveBeenCalledTimes(1); expect(mockResponse.cookie).toHaveBeenCalledTimes(1); expect(mockResponse.cookie).toHaveBeenCalledWith( expect.stringContaining('test-provider-refresh-token'), diff --git a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts index 8f08cfbbd0..85104e9608 100644 --- a/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts +++ b/plugins/auth-backend/src/lib/oauth/OAuthAdapter.ts @@ -39,6 +39,7 @@ import { OAuthStartRequest, OAuthRefreshRequest, OAuthState, + OAuthLogoutRequest, } from './types'; import { prepareBackstageIdentityResponse } from '../../providers/prepareBackstageIdentityResponse'; @@ -190,6 +191,14 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { throw new AuthenticationError('Invalid X-Requested-With header'); } + if (this.handlers.logout) { + const refreshToken = this.getRefreshTokenFromCookie(req); + const revokeRequest: OAuthLogoutRequest = Object.assign(req, { + refreshToken, + }); + await this.handlers.logout(revokeRequest); + } + // remove refresh token cookie if it is set const origin = req.get('origin'); const cookieConfig = this.getCookieConfig(origin); @@ -210,8 +219,7 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { } try { - const refreshToken = - req.cookies[`${this.options.providerId}-refresh-token`]; + const refreshToken = this.getRefreshTokenFromCookie(req); // throw error if refresh token is missing in the request if (!refreshToken) { @@ -286,6 +294,10 @@ export class OAuthAdapter implements AuthProviderRouteHandlers { }); }; + private getRefreshTokenFromCookie = (req: express.Request) => { + return req.cookies[`${this.options.providerId}-refresh-token`]; + }; + private getGrantedScopeFromCookie = (req: express.Request) => { return req.cookies[`${this.options.providerId}-granted-scope`]; }; diff --git a/plugins/auth-backend/src/lib/oauth/index.ts b/plugins/auth-backend/src/lib/oauth/index.ts index 671e8e48e9..3643898bed 100644 --- a/plugins/auth-backend/src/lib/oauth/index.ts +++ b/plugins/auth-backend/src/lib/oauth/index.ts @@ -26,5 +26,6 @@ export type { OAuthState, OAuthStartRequest, OAuthRefreshRequest, + OAuthLogoutRequest, OAuthResult, } from './types'; diff --git a/plugins/auth-backend/src/lib/oauth/types.ts b/plugins/auth-backend/src/lib/oauth/types.ts index e8b5282aad..3902c3f026 100644 --- a/plugins/auth-backend/src/lib/oauth/types.ts +++ b/plugins/auth-backend/src/lib/oauth/types.ts @@ -104,6 +104,11 @@ export type OAuthRefreshRequest = express.Request<{}> & { refreshToken: string; }; +/** @public */ +export type OAuthLogoutRequest = express.Request<{}> & { + refreshToken: string; +}; + /** * Any OAuth provider needs to implement this interface which has provider specific * handlers for different methods to perform authentication, get access tokens, @@ -136,5 +141,5 @@ export interface OAuthHandlers { /** * (Optional) Sign out of the auth provider. */ - logout?(): Promise; + logout?(req: OAuthLogoutRequest): Promise; } diff --git a/plugins/auth-backend/src/providers/google/provider.ts b/plugins/auth-backend/src/providers/google/provider.ts index 7689cd1a3a..afe5c27f6b 100644 --- a/plugins/auth-backend/src/providers/google/provider.ts +++ b/plugins/auth-backend/src/providers/google/provider.ts @@ -16,6 +16,7 @@ import express from 'express'; import passport from 'passport'; +import { OAuth2Client } from 'google-auth-library'; import { Strategy as GoogleStrategy } from 'passport-google-oauth20'; import { encodeState, @@ -27,6 +28,7 @@ import { OAuthResponse, OAuthResult, OAuthStartRequest, + OAuthLogoutRequest, } from '../../lib/oauth'; import { executeFetchUserProfileStrategy, @@ -119,6 +121,11 @@ export class GoogleAuthProvider implements OAuthHandlers { }; } + async logout(req: OAuthLogoutRequest) { + const oauthClient = new OAuth2Client(); + await oauthClient.revokeToken(req.refreshToken); + } + async refresh(req: OAuthRefreshRequest) { const { accessToken, refreshToken, params } = await executeRefreshTokenStrategy( diff --git a/plugins/user-settings/src/components/AuthProviders/ProviderSettingsItem.tsx b/plugins/user-settings/src/components/AuthProviders/ProviderSettingsItem.tsx index ab6ad8dff9..a5962d47ab 100644 --- a/plugins/user-settings/src/components/AuthProviders/ProviderSettingsItem.tsx +++ b/plugins/user-settings/src/components/AuthProviders/ProviderSettingsItem.tsx @@ -32,6 +32,7 @@ import { ProfileInfoApi, ProfileInfo, useApi, + errorApiRef, IconComponent, } from '@backstage/core-plugin-api'; import { ProviderSettingsAvatar } from './ProviderSettingsAvatar'; @@ -46,6 +47,7 @@ export const ProviderSettingsItem = (props: { const { title, description, icon: Icon, apiRef } = props; const api = useApi(apiRef); + const errorApi = useApi(errorApiRef); const [signedIn, setSignedIn] = useState(false); const emptyProfile: ProfileInfo = {}; const [profile, setProfile] = useState(emptyProfile); @@ -126,7 +128,10 @@ export const ProviderSettingsItem = (props: { diff --git a/plugins/user-settings/src/components/AuthProviders/UserSettingsAuthProviders.test.tsx b/plugins/user-settings/src/components/AuthProviders/UserSettingsAuthProviders.test.tsx index 4e14dec2ef..fe5440d355 100644 --- a/plugins/user-settings/src/components/AuthProviders/UserSettingsAuthProviders.test.tsx +++ b/plugins/user-settings/src/components/AuthProviders/UserSettingsAuthProviders.test.tsx @@ -26,7 +26,7 @@ import { UserSettingsAuthProviders } from './UserSettingsAuthProviders'; import { ApiProvider, ConfigReader } from '@backstage/core-app-api'; import { configApiRef, googleAuthApiRef } from '@backstage/core-plugin-api'; -const mockSignInHandler = jest.fn().mockReturnValue(''); +const mockSignInHandler = jest.fn().mockReturnValue(Promise.resolve()); const mockGoogleAuth = { sessionState$: () => ({ [Symbol.observable]: jest.fn(), diff --git a/plugins/user-settings/src/components/General/UserSettingsMenu.test.tsx b/plugins/user-settings/src/components/General/UserSettingsMenu.test.tsx index c8d30cd338..9c501bc775 100644 --- a/plugins/user-settings/src/components/General/UserSettingsMenu.test.tsx +++ b/plugins/user-settings/src/components/General/UserSettingsMenu.test.tsx @@ -14,8 +14,15 @@ * limitations under the License. */ -import { renderWithEffects, wrapInTestApp } from '@backstage/test-utils'; -import { fireEvent } from '@testing-library/react'; +import { + MockErrorApi, + renderInTestApp, + TestApiProvider, + renderWithEffects, + wrapInTestApp, +} from '@backstage/test-utils'; +import { errorApiRef, identityApiRef } from '@backstage/core-plugin-api'; +import { fireEvent, waitFor } from '@testing-library/react'; import React from 'react'; import { UserSettingsMenu } from './UserSettingsMenu'; @@ -30,4 +37,29 @@ describe('', () => { expect(rendered.getByText('Sign Out')).toBeInTheDocument(); }); + + it('handles errors that occur when signing out', async () => { + const failingIdentityApi = { + signOut: jest.fn().mockRejectedValue(new Error('Logout error')), + }; + const mockErrorApi = new MockErrorApi({ collect: true }); + const rendered = await renderInTestApp( + + + , + ); + + const menuButton = rendered.getByLabelText('more'); + fireEvent.click(menuButton); + fireEvent.click(rendered.getByText('Sign Out')); + + await waitFor(() => { + expect(mockErrorApi.getErrors()).toHaveLength(1); + }); + }); }); diff --git a/plugins/user-settings/src/components/General/UserSettingsMenu.tsx b/plugins/user-settings/src/components/General/UserSettingsMenu.tsx index 2483f6cd36..c83bdd5b74 100644 --- a/plugins/user-settings/src/components/General/UserSettingsMenu.tsx +++ b/plugins/user-settings/src/components/General/UserSettingsMenu.tsx @@ -18,10 +18,15 @@ import React from 'react'; import { IconButton, ListItemIcon, Menu, MenuItem } from '@material-ui/core'; import SignOutIcon from '@material-ui/icons/MeetingRoom'; import MoreVertIcon from '@material-ui/icons/MoreVert'; -import { identityApiRef, useApi } from '@backstage/core-plugin-api'; +import { + identityApiRef, + errorApiRef, + useApi, +} from '@backstage/core-plugin-api'; /** @public */ export const UserSettingsMenu = () => { + const errorApi = useApi(errorApiRef); const identityApi = useApi(identityApiRef); const [open, setOpen] = React.useState(false); const [anchorEl, setAnchorEl] = React.useState( @@ -48,7 +53,12 @@ export const UserSettingsMenu = () => { - identityApi.signOut()}> + + identityApi.signOut().catch(error => errorApi.post(error)) + } + >