PR feedback #2

AzureAD · Aug 3, 2020 · c47b2dd · c47b2dd
1 parent f0f8c7a
commit c47b2dd
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 15 deletions.
diff --git a/lib/msal-common/src/error/ClientAuthError.ts b/lib/msal-common/src/error/ClientAuthError.ts
@@ -153,7 +153,7 @@ export const ClientAuthErrorMessage = {
     },
     invalidClientCredential: {
         code: "invalid_client_credential",
-        desc: "Client credential (secret, certificate, or assertion) must not be empty when creating a confidential client"
+        desc: "Client credential (secret, certificate, or assertion) must not be empty when creating a confidential client. An application should at most have one credential"
     }
 };
 

diff --git a/lib/msal-node/src/client/ConfidentialClientApplication.ts b/lib/msal-node/src/client/ConfidentialClientApplication.ts
@@ -36,26 +36,35 @@ export class ConfidentialClientApplication extends ClientApplication {
     }
 
     private setClientCredential(configuration: Configuration): void {
-        if (!StringUtils.isEmpty(configuration.auth!.clientSecret!)) {
+
+        const clientSecretNotEmpty = !StringUtils.isEmpty(configuration.auth.clientSecret!);
+        const clientAssertionNotEmpty = !StringUtils.isEmpty(configuration.auth.clientAssertion!);
+        const certificate = configuration.auth.clientCertificate!;
+        const certificateNotEmpty = !StringUtils.isEmpty(certificate.thumbprint) || !StringUtils.isEmpty(certificate.privateKey);
+
+        // Check that at most one credential is set on the application 
+        if (
+            clientSecretNotEmpty && clientAssertionNotEmpty ||
+            clientAssertionNotEmpty && certificateNotEmpty ||
+            clientSecretNotEmpty && certificateNotEmpty) {
+                throw ClientAuthError.createInvalidCredentialError();
+        }
+
+        if (clientSecretNotEmpty) {
             this.clientSecret = configuration.auth.clientSecret!;
             return;
         }
 
-        if (!StringUtils.isEmpty(configuration.auth.clientAssertion!)) {
+        if (clientAssertionNotEmpty) {
             this.clientAssertion = ClientAssertion.fromAssertion(configuration.auth.clientAssertion!);
             return;
         }
 
-        if (configuration.auth.clientCertificate != null) {
-            const certificate = configuration.auth.clientCertificate;
-            if (StringUtils.isEmpty(certificate.thumbprint) || StringUtils.isEmpty(certificate.privateKey)) {
-                throw ClientAuthError.createInvalidCredentialError();
-            }
-
+        if (!certificateNotEmpty) {
+            throw ClientAuthError.createInvalidCredentialError();
+        } else {
             this.clientAssertion = ClientAssertion.fromCertificate(certificate.thumbprint, certificate.privateKey);
-            return; 
         }
-        throw ClientAuthError.createInvalidCredentialError();
     }
 }
 
diff --git a/lib/msal-node/test/client/ConfidentialClientApplication.spec.ts b/lib/msal-node/test/client/ConfidentialClientApplication.spec.ts
@@ -1,11 +1,15 @@
 import { ConfidentialClientApplication } from './../../src/client/ConfidentialClientApplication';
-import { Authority, ClientConfiguration, AuthorizationCodeRequest, AuthorityFactory, AuthorizationCodeClient, RefreshTokenRequest, RefreshTokenClient } from '@azure/msal-common';
+import { Authority, ClientConfiguration, AuthorizationCodeRequest, AuthorityFactory, AuthorizationCodeClient, RefreshTokenRequest, RefreshTokenClient, StringUtils } from '@azure/msal-common';
 import { TEST_CONSTANTS } from '../utils/TestConstants';
 import { Configuration } from "../../src/config/Configuration";
 import { mocked } from 'ts-jest/utils';
 
 jest.mock('@azure/msal-common');
 
+mocked(StringUtils.isEmpty).mockImplementation((str) => {
+    return (typeof str === "undefined" || !str || 0 === str.length);
+});
+
 describe('ConfidentialClientApplication', () => {
     const authority: Authority = {
         resolveEndpointsAsync: () => {
@@ -37,12 +41,12 @@ describe('ConfidentialClientApplication', () => {
             clientSecret: TEST_CONSTANTS.CLIENT_SECRET
         }
     };
-    
+
     test('exports a class', () => {
         const authApp = new ConfidentialClientApplication(appConfig);
         expect(authApp).toBeInstanceOf(ConfidentialClientApplication);
     });
-    
+
     test('acquireTokenByAuthorizationCode', async () => {
         const request: AuthorizationCodeRequest = {
             scopes: TEST_CONSTANTS.DEFAULT_GRAPH_SCOPE,
@@ -59,7 +63,7 @@ describe('ConfidentialClientApplication', () => {
             expect.objectContaining(expectedConfig)
         );
     });
-    
+
     test('acquireTokenByRefreshToken', async () => {
         const request: RefreshTokenRequest = {
             scopes: TEST_CONSTANTS.DEFAULT_GRAPH_SCOPE,