Using the server's basePath when building the SAML ACS (#51391)

* Using the server's basePath when building the SAML ACS * Fixing getACS call
elastic · Nov 25, 2019 · 74fd765 · 74fd765
1 parent d2b7f96
commit 74fd765
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 18 deletions.
diff --git a/x-pack/plugins/security/server/authentication/providers/saml.test.ts b/x-pack/plugins/security/server/authentication/providers/saml.test.ts
@@ -537,7 +537,11 @@ describe('SAMLAuthenticationProvider', () => {
         sinon.assert.calledWithExactly(
           mockOptions.client.callAsInternalUser,
           'shield.samlPrepare',
-          { body: { acs: 'test-protocol://test-hostname:1234/base-path/api/security/v1/saml' } }
+          {
+            body: {
+              acs: 'test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml',
+            },
+          }
         );
 
         expect(mockOptions.logger.warn).not.toHaveBeenCalled();
@@ -572,7 +576,11 @@ describe('SAMLAuthenticationProvider', () => {
         sinon.assert.calledWithExactly(
           mockOptions.client.callAsInternalUser,
           'shield.samlPrepare',
-          { body: { acs: 'test-protocol://test-hostname:1234/base-path/api/security/v1/saml' } }
+          {
+            body: {
+              acs: 'test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml',
+            },
+          }
         );
 
         expect(mockOptions.logger.warn).toHaveBeenCalledTimes(1);
@@ -610,7 +618,11 @@ describe('SAMLAuthenticationProvider', () => {
         sinon.assert.calledWithExactly(
           mockOptions.client.callAsInternalUser,
           'shield.samlPrepare',
-          { body: { acs: 'test-protocol://test-hostname:1234/base-path/api/security/v1/saml' } }
+          {
+            body: {
+              acs: 'test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml',
+            },
+          }
         );
 
         expect(mockOptions.logger.warn).toHaveBeenCalledTimes(1);
@@ -686,7 +698,11 @@ describe('SAMLAuthenticationProvider', () => {
         sinon.assert.calledWithExactly(
           mockOptions.client.callAsInternalUser,
           'shield.samlPrepare',
-          { body: { acs: 'test-protocol://test-hostname:1234/base-path/api/security/v1/saml' } }
+          {
+            body: {
+              acs: 'test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml',
+            },
+          }
         );
 
         expect(authenticationResult.failed()).toBe(true);
@@ -752,7 +768,9 @@ describe('SAMLAuthenticationProvider', () => {
       const authenticationResult = await provider.authenticate(request);
 
       sinon.assert.calledWithExactly(mockOptions.client.callAsInternalUser, 'shield.samlPrepare', {
-        body: { acs: `test-protocol://test-hostname:1234/base-path/api/security/v1/saml` },
+        body: {
+          acs: `test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml`,
+        },
       });
 
       expect(authenticationResult.redirected()).toBe(true);
@@ -773,7 +791,9 @@ describe('SAMLAuthenticationProvider', () => {
       const authenticationResult = await provider.authenticate(request, null);
 
       sinon.assert.calledWithExactly(mockOptions.client.callAsInternalUser, 'shield.samlPrepare', {
-        body: { acs: `test-protocol://test-hostname:1234/base-path/api/security/v1/saml` },
+        body: {
+          acs: `test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml`,
+        },
       });
 
       expect(authenticationResult.failed()).toBe(true);
@@ -983,7 +1003,9 @@ describe('SAMLAuthenticationProvider', () => {
       const authenticationResult = await provider.authenticate(request, state);
 
       sinon.assert.calledWithExactly(mockOptions.client.callAsInternalUser, 'shield.samlPrepare', {
-        body: { acs: `test-protocol://test-hostname:1234/base-path/api/security/v1/saml` },
+        body: {
+          acs: `test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml`,
+        },
       });
 
       expect(mockOptions.logger.warn).toHaveBeenCalledTimes(1);
@@ -1053,7 +1075,9 @@ describe('SAMLAuthenticationProvider', () => {
       const authenticationResult = await provider.authenticate(request, state);
 
       sinon.assert.calledWithExactly(mockOptions.client.callAsInternalUser, 'shield.samlPrepare', {
-        body: { acs: `test-protocol://test-hostname:1234/base-path/api/security/v1/saml` },
+        body: {
+          acs: `test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml`,
+        },
       });
 
       expect(mockOptions.logger.warn).toHaveBeenCalledTimes(1);
@@ -1184,7 +1208,7 @@ describe('SAMLAuthenticationProvider', () => {
         {
           body: {
             queryString: 'SAMLRequest=xxx%20yyy',
-            acs: 'test-protocol://test-hostname:1234/base-path/api/security/v1/saml',
+            acs: 'test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml',
           },
         }
       );
@@ -1287,7 +1311,7 @@ describe('SAMLAuthenticationProvider', () => {
         {
           body: {
             queryString: 'SAMLRequest=xxx%20yyy',
-            acs: 'test-protocol://test-hostname:1234/base-path/api/security/v1/saml',
+            acs: 'test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml',
           },
         }
       );
@@ -1312,7 +1336,7 @@ describe('SAMLAuthenticationProvider', () => {
         {
           body: {
             queryString: 'SAMLRequest=xxx%20yyy',
-            acs: 'test-protocol://test-hostname:1234/base-path/api/security/v1/saml',
+            acs: 'test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml',
           },
         }
       );
@@ -1364,7 +1388,7 @@ describe('SAMLAuthenticationProvider', () => {
         {
           body: {
             queryString: 'SAMLRequest=xxx%20yyy',
-            acs: 'test-protocol://test-hostname:1234/base-path/api/security/v1/saml',
+            acs: 'test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml',
           },
         }
       );

diff --git a/x-pack/plugins/security/server/authentication/providers/saml.ts b/x-pack/plugins/security/server/authentication/providers/saml.ts
@@ -525,7 +525,7 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider {
 
     try {
       // Prefer realm name if it's specified, otherwise fallback to ACS.
-      const preparePayload = this.realm ? { realm: this.realm } : { acs: this.getACS(request) };
+      const preparePayload = this.realm ? { realm: this.realm } : { acs: this.getACS() };
 
       // This operation should be performed on behalf of the user with a privilege that normal
       // user usually doesn't have `cluster:admin/xpack/security/saml/prepare`.
@@ -572,7 +572,7 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider {
     this.logger.debug('Single logout has been initiated by the Identity Provider.');
 
     // Prefer realm name if it's specified, otherwise fallback to ACS.
-    const invalidatePayload = this.realm ? { realm: this.realm } : { acs: this.getACS(request) };
+    const invalidatePayload = this.realm ? { realm: this.realm } : { acs: this.getACS() };
 
     // This operation should be performed on behalf of the user with a privilege that normal
     // user usually doesn't have `cluster:admin/xpack/security/saml/invalidate`.
@@ -592,10 +592,10 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider {
   /**
    * Constructs and returns Kibana's Assertion consumer service URL.
    */
-  private getACS(request: KibanaRequest) {
-    return `${this.options.getServerBaseURL()}${this.options.basePath.get(
-      request
-    )}/api/security/v1/saml`;
+  private getACS() {
+    return `${this.options.getServerBaseURL()}${
+      this.options.basePath.serverBasePath
+    }/api/security/v1/saml`;
   }
 
   /**