feat(provider): make client secret non mandatory for public Clients

make client secret non mandatory for public clients GH-153
sourcefuse · Mar 30, 2023 · 1819e6f · 1819e6f
1 parent 3d194fe
commit 1819e6f
Show file tree

Hide file tree

Showing 11 changed files with 174 additions and 28 deletions.
diff --git a/src/__tests__/fixtures/providers/passport-client.provider.ts b/src/__tests__/fixtures/providers/passport-client.provider.ts
@@ -1,6 +1,6 @@
 import {Provider} from '@loopback/core';
 import {VerifyFunction} from '../../../strategies';
-import {ClientType, IAuthClient} from '../../../types';
+import {IAuthClient} from '../../../types';
 
 export class ClientPasswordVerifyProvider
   implements Provider<VerifyFunction.OauthClientPasswordFn>
@@ -16,7 +16,6 @@ export class ClientPasswordVerifyProvider
       const clientToPass: IAuthClient = {
         clientId: clientId || 'id',
         clientSecret: clientSecret || 'secret',
-        clientType: ClientType.public,
       };
 
       return clientToPass;

diff --git a/src/__tests__/fixtures/providers/resource-owner.provider.ts b/src/__tests__/fixtures/providers/resource-owner.provider.ts
@@ -1,7 +1,7 @@
 import {Provider} from '@loopback/core';
 import {VerifyFunction} from '../../../strategies';
 import {Request} from 'express';
-import {IAuthUser, IAuthClient, ClientType} from '../../../types';
+import {IAuthUser, IAuthClient} from '../../../types';
 
 export class ResourceOwnerVerifyProvider
   implements Provider<VerifyFunction.ResourceOwnerPasswordFn>
@@ -29,7 +29,6 @@ export class ResourceOwnerVerifyProvider
       const clientToPass: IAuthClient = {
         clientId: clientId || 'client id',
         clientSecret: clientSecret || 'client secret',
-        clientType: ClientType.public,
       };
 
       return {user: userToPass, client: clientToPass};

diff --git a/...ntegration/action-sequence/passport-client-password/client-password-verify.integration.ts b/...ntegration/action-sequence/passport-client-password/client-password-verify.integration.ts
@@ -81,9 +81,9 @@ describe('Client-password strategy', () => {
       .expect(200);
 
     expect(client.body).to.have.property('clientId');
-    // expect(client.body).to.have.property('clientSecret');
+    expect(client.body).to.have.property('clientSecret');
     expect(client.body.clientId).to.equal('some id');
-    // expect(client.body.clientSecret).to.equal('some secret');
+    expect(client.body.clientSecret).to.equal('some secret');
   });
 
   it('should return status 401 when options.passRequestToCallback is set true', async () => {
@@ -182,7 +182,7 @@ describe('integration test for client-password and no verifier', () => {
         @requestBody()
         body: {
           client_id: string;
-          // client_secret: string;
+          client_secret: string;
         },
       ) {
         return this.client;

diff --git a/src/__tests__/unit/passport-client-password/client-password-strategy.unit.ts b/src/__tests__/unit/passport-client-password/client-password-strategy.unit.ts
@@ -1,4 +1,4 @@
-import {ClientType, IAuthClient} from '../../../types';
+import {IAuthClient} from '../../../types';
 import {
   ClientPasswordStrategyFactoryProvider,
   ClientPasswordStrategyFactory,
@@ -70,7 +70,6 @@ function verifierBearer(
   const clientToPass: IAuthClient = {
     clientId: clientId,
     clientSecret: clientSecret,
-    clientType: ClientType.public,
   };
 
   return new Promise(function (resolve, reject) {

diff --git a/src/__tests__/unit/passport-resource-owner-password/resource-owner-password.unit.ts b/src/__tests__/unit/passport-resource-owner-password/resource-owner-password.unit.ts
@@ -1,4 +1,4 @@
-import {IAuthUser, IAuthClient, ClientType} from '../../../types';
+import {IAuthUser, IAuthClient} from '../../../types';
 import {expect} from '@loopback/testlab';
 import {
   ResourceOwnerPasswordStrategyFactoryProvider,
@@ -81,7 +81,6 @@ function verifierResourceOwner(
   const clientToPass: IAuthClient = {
     clientId: 'id',
     clientSecret: 'secret',
-    clientType: ClientType.public,
   };
 
   return new Promise(function (resolve, reject) {

diff --git a/src/component.ts b/src/component.ts
@@ -39,6 +39,7 @@ import {
   OtpVerifyProvider,
 } from './strategies';
 import {Strategies} from './strategies/keys';
+import {SecureClientPasswordStrategyFactoryProvider} from './strategies/passport/passport-client-password/secure-client-password-strategy-factory-provider';
 import {
   CognitoAuthVerifyProvider,
   CognitoStrategyFactoryProvider,
@@ -47,12 +48,14 @@ import {
   SamlStrategyFactoryProvider,
   SamlVerifyProvider,
 } from './strategies/SAML';
-import {AuthenticationConfig} from './types';
+import {AuthenticationConfig, IAuthSecureClientConfig} from './types';
 
 export class AuthenticationComponent implements Component {
   constructor(
     @inject(AuthenticationBindings.CONFIG, {optional: true})
     private readonly config?: AuthenticationConfig,
+    @inject(AuthenticationBindings.SecureClientConfig, {optional: true})
+    private readonly secureClientConfig?: IAuthSecureClientConfig,
   ) {
     this.providers = {
       [AuthenticationBindings.USER_AUTH_ACTION.key]: AuthenticateActionProvider,
@@ -114,6 +117,14 @@ export class AuthenticationComponent implements Component {
       [Strategies.Passport.AZURE_AD_VERIFIER.key]: AzureADAuthVerifyProvider,
       [Strategies.Passport.KEYCLOAK_VERIFIER.key]: KeycloakVerifyProvider,
     };
+
+    if (this.secureClientConfig?.secureClient) {
+      this.providers = {
+        ...this.providers,
+        [Strategies.Passport.CLIENT_PASSWORD_STRATEGY_FACTORY.key]:
+          SecureClientPasswordStrategyFactoryProvider,
+      };
+    }
     this.bindings = [];
     if (this.config?.useClientAuthenticationMiddleware) {
       this.bindings.push(

diff --git a/src/keys.ts b/src/keys.ts
@@ -8,6 +8,7 @@ import {
   AuthenticationMetadata,
   EntityWithIdentifier,
   IAuthClient,
+  IAuthSecureClientConfig,
   IAuthUser,
 } from './types';
 
@@ -17,6 +18,10 @@ export * from './strategies/keys';
  * Binding keys used by this component.
  */
 export namespace AuthenticationBindings {
+  export const SecureClientConfig =
+    BindingKey.create<IAuthSecureClientConfig | null>(
+      `auth.secure.client.config`,
+    );
   export const USER_STRATEGY = BindingKey.create<Strategy | undefined>(
     'sf.userAuthentication.strategy',
   );

diff --git a/...strategies/passport/passport-client-password/client-password-strategy-factory-provider.ts b/...strategies/passport/passport-client-password/client-password-strategy-factory-provider.ts
@@ -3,7 +3,7 @@ import {HttpErrors, Request} from '@loopback/rest';
 import * as ClientPasswordStrategy from './client-password-strategy';
 
 import {AuthErrorKeys} from '../../../error-keys';
-import {ClientType, IAuthClient} from '../../../types';
+import {IAuthClient} from '../../../types';
 import {Strategies} from '../../keys';
 import {VerifyFunction} from '../../types';
 
@@ -45,16 +45,13 @@ export class ClientPasswordStrategyFactoryProvider
             const client = await verifyFn(clientId, clientSecret, req);
             if (!client) {
               throw new HttpErrors.Unauthorized(AuthErrorKeys.ClientInvalid);
-            } else if (
-              client.clientType !== ClientType.public &&
-              !clientSecret
-            ) {
+            } else if (!clientSecret) {
               throw new HttpErrors.Unauthorized(
-                AuthErrorKeys.ConfidentialClientSecretMissing,
+                AuthErrorKeys.ClientSecretMissing,
               );
             } else if (
-              client.clientType !== ClientType.public &&
-              (!client.clientSecret || client.clientSecret !== clientSecret)
+              !client.clientSecret ||
+              client.clientSecret !== clientSecret
             ) {
               throw new HttpErrors.Unauthorized(
                 AuthErrorKeys.ClientVerificationFailed,
@@ -83,16 +80,13 @@ export class ClientPasswordStrategyFactoryProvider
 
             if (!client) {
               throw new HttpErrors.Unauthorized(AuthErrorKeys.ClientInvalid);
-            } else if (
-              client.clientType !== ClientType.public &&
-              !clientSecret
-            ) {
+            } else if (!clientSecret) {
               throw new HttpErrors.Unauthorized(
-                AuthErrorKeys.ConfidentialClientSecretMissing,
+                AuthErrorKeys.ClientSecretMissing,
               );
             } else if (
-              client.clientType !== ClientType.public &&
-              (!client.clientSecret || client.clientSecret !== clientSecret)
+              !client.clientSecret ||
+              client.clientSecret !== clientSecret
             ) {
               throw new HttpErrors.Unauthorized(
                 AuthErrorKeys.ClientVerificationFailed,

diff --git a/...ies/passport/passport-client-password/secure-client-password-strategy-factory-provider.ts b/...ies/passport/passport-client-password/secure-client-password-strategy-factory-provider.ts
@@ -0,0 +1,112 @@
+import {inject, Provider} from '@loopback/core';
+import {HttpErrors, Request} from '@loopback/rest';
+import * as ClientPasswordStrategy from './client-password-strategy';
+
+import {AuthErrorKeys} from '../../../error-keys';
+import {ClientType, IAuthClient, IAuthSecureClient} from '../../../types';
+import {Strategies} from '../../keys';
+import {VerifyFunction} from '../../types';
+
+export interface SecureClientPasswordStrategyFactory {
+  (
+    options?: ClientPasswordStrategy.StrategyOptionsWithRequestInterface,
+    verifierPassed?: VerifyFunction.OauthSecureClientPasswordFn,
+  ): ClientPasswordStrategy.Strategy;
+}
+
+export class SecureClientPasswordStrategyFactoryProvider
+  implements Provider<SecureClientPasswordStrategyFactory>
+{
+  constructor(
+    @inject(Strategies.Passport.OAUTH2_CLIENT_PASSWORD_VERIFIER)
+    private readonly verifier: VerifyFunction.OauthSecureClientPasswordFn,
+  ) {}
+
+  value(): SecureClientPasswordStrategyFactory {
+    return (options, verifier) =>
+      this.getSecureClientPasswordVerifier(options, verifier);
+  }
+
+  getSecureClientPasswordVerifier(
+    options?: ClientPasswordStrategy.StrategyOptionsWithRequestInterface,
+    verifierPassed?: VerifyFunction.OauthSecureClientPasswordFn,
+  ): ClientPasswordStrategy.Strategy {
+    const verifyFn = verifierPassed ?? this.verifier;
+    if (options?.passReqToCallback) {
+      return new ClientPasswordStrategy.Strategy(
+        // eslint-disable-next-line @typescript-eslint/no-misused-promises
+        async (
+          clientId: string,
+          clientSecret: string | undefined,
+          cb: (err: Error | null, client?: IAuthSecureClient | false) => void,
+          req: Request | undefined,
+        ) => {
+          try {
+            const client = await verifyFn(clientId, clientSecret, req);
+            if (!client) {
+              throw new HttpErrors.Unauthorized(AuthErrorKeys.ClientInvalid);
+            } else if (
+              client.clientType !== ClientType.public &&
+              !clientSecret
+            ) {
+              throw new HttpErrors.Unauthorized(
+                AuthErrorKeys.ConfidentialClientSecretMissing,
+              );
+            } else if (
+              (client.clientType !== ClientType.public &&
+                !client.clientSecret) ||
+              client.clientSecret !== clientSecret
+            ) {
+              throw new HttpErrors.Unauthorized(
+                AuthErrorKeys.ClientVerificationFailed,
+              );
+            } else {
+              // do nothing
+            }
+
+            cb(null, client);
+          } catch (err) {
+            cb(err);
+          }
+        },
+        options,
+      );
+    } else {
+      return new ClientPasswordStrategy.Strategy(
+        // eslint-disable-next-line @typescript-eslint/no-misused-promises
+        async (
+          clientId: string,
+          clientSecret: string | undefined,
+          cb: (err: Error | null, client?: IAuthClient | false) => void,
+        ) => {
+          try {
+            const client = await verifyFn(clientId, clientSecret);
+
+            if (!client) {
+              throw new HttpErrors.Unauthorized(AuthErrorKeys.ClientInvalid);
+            } else if (
+              client.clientType !== ClientType.public &&
+              !clientSecret
+            ) {
+              throw new HttpErrors.Unauthorized(
+                AuthErrorKeys.ConfidentialClientSecretMissing,
+              );
+            } else if (
+              client.clientType !== ClientType.public &&
+              (!client.clientSecret || client.clientSecret !== clientSecret)
+            ) {
+              throw new HttpErrors.Unauthorized(
+                AuthErrorKeys.ClientVerificationFailed,
+              );
+            } else {
+              // do nothing
+            }
+            cb(null, client);
+          } catch (err) {
+            cb(err);
+          }
+        },
+      );
+    }
+  }
+}
diff --git a/src/strategies/types/types.ts b/src/strategies/types/types.ts
@@ -6,7 +6,7 @@ import * as FacebookStrategy from 'passport-facebook';
 import * as AppleStrategy from 'passport-apple';
 import * as SamlStrategy from '@node-saml/passport-saml';
 import {DecodedIdToken} from 'passport-apple';
-import {Cognito, IAuthClient, IAuthUser} from '../../types';
+import {Cognito, IAuthClient, IAuthSecureClient, IAuthUser} from '../../types';
 import {Keycloak} from './keycloak.types';
 import {Otp} from '../passport';
 
@@ -23,6 +23,11 @@ export namespace VerifyFunction {
     (clientId: string, clientSecret: string, req?: Request): Promise<T | null>;
   }
 
+  export interface OauthSecureClientPasswordFn<T = IAuthSecureClient>
+    extends GenericAuthFn<T> {
+    (clientId: string, clientSecret: string, req?: Request): Promise<T | null>;
+  }
+
   export interface LocalPasswordFn<T = IAuthUser> extends GenericAuthFn<T> {
     (username: string, password: string, req?: Request): Promise<T | null>;
   }
@@ -45,6 +50,19 @@ export namespace VerifyFunction {
     ): Promise<{client: T; user: S} | null>;
   }
 
+  export interface SecureResourceOwnerPasswordFn<
+    T = IAuthSecureClient,
+    S = IAuthUser,
+  > {
+    (
+      clientId: string,
+      clientSecret: string,
+      username: string,
+      password: string,
+      req?: Request,
+    ): Promise<{client: T; user: S} | null>;
+  }
+
   export interface GoogleAuthFn<T = IAuthUser> extends GenericAuthFn<T> {
     (
       accessToken: string,

diff --git a/src/types.ts b/src/types.ts
@@ -9,12 +9,22 @@ import {VerifyFunction} from './strategies';
 export * from './strategies/types';
 
 export interface IAuthClient {
+  clientId: string;
+  clientSecret: string;
+  redirectUrl?: string;
+}
+
+export interface IAuthSecureClient {
   clientId: string;
   clientSecret: string;
   clientType: ClientType;
   redirectUrl?: string;
 }
 
+export interface IAuthSecureClientConfig {
+  secureClient: boolean;
+}
+
 export interface IAuthUser {
   id?: number | string;
   username: string;