Merge pull request #13670 from fabuloso/secure-google-oauth-logout

Revoke Google OAuth refresh token on logout

.changeset/blue-lions-greet.md

@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-user-settings': patch
+---
+
+Handle errors that may occur when the user logs out

.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

plugins/auth-backend/api-report.md

@@ -314,7 +314,7 @@ export interface OAuthHandlers {
     response: OAuthResponse;
     refreshToken?: string;
   }>;
-  logout?(): Promise<void>;
+  logout?(req: OAuthLogoutRequest): Promise<void>;
   refresh?(req: OAuthRefreshRequest): Promise<{
     response: OAuthResponse;
     refreshToken?: string;
@@ -322,6 +322,11 @@ export interface OAuthHandlers {
   start(req: OAuthStartRequest): Promise<OAuthStartResponse>;
 }
 
+// @public (undocumented)
+export type OAuthLogoutRequest = express.Request<{}> & {
+  refreshToken: string;
+};
+
 // @public (undocumented)
 export type OAuthProviderInfo = {
   accessToken: string;

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<CookieConfigurer> = {
@@ -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'),

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`];
   };

plugins/auth-backend/src/lib/oauth/index.ts

@@ -26,5 +26,6 @@ export type {
   OAuthState,
   OAuthStartRequest,
   OAuthRefreshRequest,
+  OAuthLogoutRequest,
   OAuthResult,
 } from './types';

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<void>;
+  logout?(req: OAuthLogoutRequest): Promise<void>;
 }

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(

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: {
           <Button
             variant="outlined"
             color="primary"
-            onClick={() => (signedIn ? api.signOut() : api.signIn())}
+            onClick={() => {
+              const action = signedIn ? api.signOut() : api.signIn();
+              action.catch(error => errorApi.post(error));
+            }}
           >
             {signedIn ? `Sign out` : `Sign in`}
           </Button>

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(),

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('<UserSettingsMenu />', () => {
 
     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(
+      <TestApiProvider
+        apis={[
+          [errorApiRef, mockErrorApi],
+          [identityApiRef, failingIdentityApi],
+        ]}
+      >
+        <UserSettingsMenu />
+      </TestApiProvider>,
+    );
+
+    const menuButton = rendered.getByLabelText('more');
+    fireEvent.click(menuButton);
+    fireEvent.click(rendered.getByText('Sign Out'));
+
+    await waitFor(() => {
+      expect(mockErrorApi.getErrors()).toHaveLength(1);
+    });
+  });
 });

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<undefined | HTMLElement>(
@@ -48,7 +53,12 @@ export const UserSettingsMenu = () => {
         <MoreVertIcon />
       </IconButton>
       <Menu anchorEl={anchorEl} open={open} onClose={handleClose}>
-        <MenuItem data-testid="sign-out" onClick={() => identityApi.signOut()}>
+        <MenuItem
+          data-testid="sign-out"
+          onClick={() =>
+            identityApi.signOut().catch(error => errorApi.post(error))
+          }
+        >
           <ListItemIcon>
             <SignOutIcon />
           </ListItemIcon>