auth-backend: fix OAuthAdapter scope store, only storing on success

Signed-off-by: Patrik Oldsberg <poldsberg@gmail.com>

.changeset/chilled-papayas-wonder.md

@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-auth-backend': patch
+---
+
+Fixed a bug where providers that tracked the granted scopes through a cookie would not take failed authentication attempts into account.

plugins/auth-backend/api-report.md

@@ -592,6 +592,7 @@ export type OAuthState = {
   nonce: string;
   env: string;
   origin?: string;
+  scope?: string;
 };
 
 // @public

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, OAuthResponse } from './types';
+import { OAuthHandlers, OAuthResponse, OAuthState } from './types';
 
 const mockResponseData = {
   providerInfo: {
@@ -148,6 +148,102 @@ describe('OAuthAdapter', () => {
     );
   });
 
+  it('persists scope through cookie if enabled', async () => {
+    const handlers = {
+      start: jest.fn(async (_req: { state: OAuthState }) => ({
+        url: '/url',
+        status: 301,
+      })),
+      handler: jest.fn(async () => ({ response: mockResponseData })),
+      refresh: jest.fn(async () => ({ response: mockResponseData })),
+    };
+    const oauthProvider = new OAuthAdapter(handlers, {
+      ...oAuthProviderOptions,
+      disableRefresh: false,
+      persistScopes: true,
+    });
+
+    // First we test the /start request, making sure state is set
+    const mockStartReq = {
+      query: {
+        scope: 'user',
+        env: 'development',
+      },
+    } as unknown as express.Request;
+    const mockStartRes = {
+      cookie: jest.fn().mockReturnThis(),
+      end: jest.fn().mockReturnThis(),
+      setHeader: jest.fn().mockReturnThis(),
+      statusCode: jest.fn().mockReturnThis(),
+    } as unknown as express.Response;
+
+    await oauthProvider.start(mockStartReq, mockStartRes);
+
+    expect(handlers.start).toHaveBeenCalledTimes(1);
+    expect(handlers.start).toHaveBeenCalledWith({
+      query: {
+        scope: 'user',
+        env: 'development',
+      },
+      scope: 'user',
+      state: {
+        nonce: expect.any(String),
+        env: 'development',
+        origin: undefined,
+        scope: 'user',
+      },
+    });
+
+    // Then test the /handler, making sure the granted scope cookie is set
+    const providedState = handlers.start.mock.calls[0][0].state;
+    const mockHandleReq = {
+      cookies: {
+        'test-provider-nonce': providedState.nonce,
+      },
+      query: {
+        state: encodeState(providedState),
+      },
+    } as unknown as express.Request;
+    const mockHandleRes = {
+      cookie: jest.fn().mockReturnThis(),
+      setHeader: jest.fn().mockReturnThis(),
+      end: jest.fn().mockReturnThis(),
+    } as unknown as express.Response;
+
+    await oauthProvider.frameHandler(mockHandleReq, mockHandleRes);
+    expect(mockHandleRes.cookie).toHaveBeenCalledTimes(1);
+    expect(mockHandleRes.cookie).toHaveBeenCalledWith(
+      'test-provider-granted-scope',
+      'user',
+      expect.objectContaining({
+        path: '/auth/test-provider',
+        maxAge: THOUSAND_DAYS_MS,
+      }),
+    );
+
+    // Them make sure scopes are forwarded correctly during refresh
+    const mockRefreshReq = {
+      query: { scope: 'ignore-me' },
+      cookies: {
+        'test-provider-granted-scope': 'user',
+        'test-provider-refresh-token': 'refresh-token',
+      },
+      header: jest.fn().mockReturnValue('XMLHttpRequest'),
+    } as unknown as express.Request;
+    const mockRefreshRes = {
+      status: jest.fn().mockReturnThis(),
+      json: jest.fn().mockReturnThis(),
+    } as unknown as express.Response;
+    await oauthProvider.refresh(mockRefreshReq, mockRefreshRes);
+    expect(handlers.refresh).toHaveBeenCalledTimes(1);
+    expect(handlers.refresh).toHaveBeenCalledWith(
+      expect.objectContaining({
+        scope: 'user',
+        refreshToken: 'refresh-token',
+      }),
+    );
+  });
+
   it('does not set the refresh cookie if refresh is disabled', async () => {
     const oauthProvider = new OAuthAdapter(providerInstance, {
       ...oAuthProviderOptions,

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

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import express from 'express';
+import express, { CookieOptions } from 'express';
 import crypto from 'crypto';
 import { URL } from 'url';
 import {
@@ -90,10 +90,20 @@ export class OAuthAdapter implements AuthProviderRouteHandlers {
     });
   }
 
+  private readonly baseCookieOptions: CookieOptions;
+
   constructor(
     private readonly handlers: OAuthHandlers,
     private readonly options: Options,
-  ) {}
+  ) {
+    this.baseCookieOptions = {
+      httpOnly: true,
+      sameSite: 'lax',
+      secure: this.options.secure,
+      path: this.options.cookiePath,
+      domain: this.options.cookieDomain,
+    };
+  }
 
   async start(req: express.Request, res: express.Response): Promise<void> {
     // retrieve scopes from request
@@ -105,15 +115,17 @@ export class OAuthAdapter implements AuthProviderRouteHandlers {
       throw new InputError('No env provided in request query parameters');
     }
 
-    if (this.options.persistScopes) {
-      this.setScopesCookie(res, scope);
-    }
-
     const nonce = crypto.randomBytes(16).toString('base64');
     // set a nonce cookie before redirecting to oauth provider
     this.setNonceCookie(res, nonce);
 
-    const state = { nonce, env, origin };
+    const state: OAuthState = { nonce, env, origin };
+
+    // If scopes are persisted then we pass them through the state so that we
+    // can set the cookie on successful auth
+    if (this.options.persistScopes) {
+      state.scope = scope;
+    }
     const forwardReq = Object.assign(req, { scope, state });
 
     const { url, status } = await this.handlers.start(
@@ -151,12 +163,11 @@ export class OAuthAdapter implements AuthProviderRouteHandlers {
 
       const { response, refreshToken } = await this.handlers.handler(req);
 
-      if (this.options.persistScopes) {
-        const grantedScopes = this.getScopesFromCookie(
-          req,
-          this.options.providerId,
-        );
-        response.providerInfo.scope = grantedScopes;
+      // Store the scope that we have been granted for this session. This is useful if
+      // the provider does not return granted scopes on refresh or if they are normalized.
+      if (this.options.persistScopes && state.scope) {
+        this.setGrantedScopeCookie(res, state.scope);
+        response.providerInfo.scope = state.scope;
       }
 
       if (refreshToken && !this.options.disableRefresh) {
@@ -214,8 +225,10 @@ export class OAuthAdapter implements AuthProviderRouteHandlers {
         throw new InputError('Missing session cookie');
       }
 
-      const scope = req.query.scope?.toString() ?? '';
-
+      let scope = req.query.scope?.toString() ?? '';
+      if (this.options.persistScopes) {
+        scope = this.getGrantedScopeFromCookie(req);
+      }
       const forwardReq = Object.assign(req, { scope, refreshToken });
 
       // get new access_token
@@ -267,27 +280,20 @@ export class OAuthAdapter implements AuthProviderRouteHandlers {
   private setNonceCookie = (res: express.Response, nonce: string) => {
     res.cookie(`${this.options.providerId}-nonce`, nonce, {
       maxAge: TEN_MINUTES_MS,
-      secure: this.options.secure,
-      sameSite: 'lax',
-      domain: this.options.cookieDomain,
+      ...this.baseCookieOptions,
       path: `${this.options.cookiePath}/handler`,
-      httpOnly: true,
     });
   };
 
-  private setScopesCookie = (res: express.Response, scope: string) => {
-    res.cookie(`${this.options.providerId}-scope`, scope, {
-      maxAge: TEN_MINUTES_MS,
-      secure: this.options.secure,
-      sameSite: 'lax',
-      domain: this.options.cookieDomain,
-      path: `${this.options.cookiePath}/handler`,
-      httpOnly: true,
+  private setGrantedScopeCookie = (res: express.Response, scope: string) => {
+    res.cookie(`${this.options.providerId}-granted-scope`, scope, {
+      maxAge: THOUSAND_DAYS_MS,
+      ...this.baseCookieOptions,
     });
   };
 
-  private getScopesFromCookie = (req: express.Request, providerId: string) => {
-    return req.cookies[`${providerId}-scope`];
+  private getGrantedScopeFromCookie = (req: express.Request) => {
+    return req.cookies[`${this.options.providerId}-granted-scope`];
   };
 
   private setRefreshTokenCookie = (
@@ -296,22 +302,14 @@ export class OAuthAdapter implements AuthProviderRouteHandlers {
   ) => {
     res.cookie(`${this.options.providerId}-refresh-token`, refreshToken, {
       maxAge: THOUSAND_DAYS_MS,
-      secure: this.options.secure,
-      sameSite: 'lax',
-      domain: this.options.cookieDomain,
-      path: this.options.cookiePath,
-      httpOnly: true,
+      ...this.baseCookieOptions,
     });
   };
 
   private removeRefreshTokenCookie = (res: express.Response) => {
     res.cookie(`${this.options.providerId}-refresh-token`, '', {
       maxAge: 0,
-      secure: this.options.secure,
-      sameSite: 'lax',
-      domain: this.options.cookieDomain,
-      path: this.options.cookiePath,
-      httpOnly: true,
+      ...this.baseCookieOptions,
     });
   };
 }

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

@@ -87,6 +87,7 @@ export type OAuthState = {
   nonce: string;
   env: string;
   origin?: string;
+  scope?: string;
 };
 
 export type OAuthStartRequest = express.Request<{}> & {