[7.x] Do not treat HTTP 500 error as possible reason for the expired/…

…missing tokens. (elastic#56758)
flash1293 · Feb 5, 2020 · 8df263b · 8df263b
1 parent 433de85
commit 8df263b
Show file tree

Hide file tree

Showing 5 changed files with 2 additions and 163 deletions.
diff --git a/x-pack/plugins/security/server/authentication/providers/kerberos.test.ts b/x-pack/plugins/security/server/authentication/providers/kerberos.test.ts
@@ -419,44 +419,6 @@ describe('KerberosAuthenticationProvider', () => {
       expect(authenticationResult.authResponseHeaders).toEqual({ 'WWW-Authenticate': 'Negotiate' });
     });
 
-    it('fails with `Negotiate` challenge if both access and refresh token documents are missing and backend supports Kerberos.', async () => {
-      const request = httpServerMock.createKibanaRequest({ headers: {} });
-      const tokenPair = { accessToken: 'missing-token', refreshToken: 'missing-refresh-token' };
-
-      mockScopedClusterClient(
-        mockOptions.client,
-        sinon.match({ headers: { authorization: `Bearer ${tokenPair.accessToken}` } })
-      )
-        .callAsCurrentUser.withArgs('shield.authenticate')
-        .rejects({
-          statusCode: 500,
-          body: { error: { reason: 'token document is missing and must be present' } },
-        });
-
-      mockScopedClusterClient(
-        mockOptions.client,
-        sinon.match({
-          headers: { authorization: `Negotiate ${Buffer.from('__fake__').toString('base64')}` },
-        })
-      )
-        .callAsCurrentUser.withArgs('shield.authenticate')
-        .rejects(
-          ElasticsearchErrorHelpers.decorateNotAuthorizedError(
-            new (errors.AuthenticationException as any)('Unauthorized', {
-              body: { error: { header: { 'WWW-Authenticate': 'Negotiate' } } },
-            })
-          )
-        );
-
-      mockOptions.tokens.refresh.withArgs(tokenPair.refreshToken).resolves(null);
-
-      const authenticationResult = await provider.authenticate(request, tokenPair);
-
-      expect(authenticationResult.failed()).toBe(true);
-      expect(authenticationResult.error).toHaveProperty('output.statusCode', 401);
-      expect(authenticationResult.authResponseHeaders).toEqual({ 'WWW-Authenticate': 'Negotiate' });
-    });
-
     it('succeeds if `authorization` contains a valid token.', async () => {
       const user = mockAuthenticatedUser();
       const request = httpServerMock.createKibanaRequest({

diff --git a/x-pack/plugins/security/server/authentication/providers/saml.test.ts b/x-pack/plugins/security/server/authentication/providers/saml.test.ts
@@ -942,84 +942,6 @@ describe('SAMLAuthenticationProvider', () => {
       );
     });
 
-    it('re-capture URL for non-AJAX requests if access token document is missing.', async () => {
-      const request = httpServerMock.createKibanaRequest({ path: '/s/foo/some-path' });
-      const state = {
-        username: 'user',
-        accessToken: 'expired-token',
-        refreshToken: 'expired-refresh-token',
-      };
-
-      mockScopedClusterClient(
-        mockOptions.client,
-        sinon.match({ headers: { authorization: `Bearer ${state.accessToken}` } })
-      )
-        .callAsCurrentUser.withArgs('shield.authenticate')
-        .rejects({
-          statusCode: 500,
-          body: { error: { reason: 'token document is missing and must be present' } },
-        });
-
-      mockOptions.tokens.refresh.withArgs(state.refreshToken).resolves(null);
-
-      const authenticationResult = await provider.authenticate(request, state);
-
-      sinon.assert.notCalled(mockOptions.client.callAsInternalUser);
-
-      expect(authenticationResult.redirected()).toBe(true);
-      expect(authenticationResult.redirectURL).toBe(
-        '/mock-server-basepath/api/security/saml/capture-url-fragment'
-      );
-      expect(authenticationResult.state).toEqual({ redirectURL: '/base-path/s/foo/some-path' });
-    });
-
-    it('initiates SAML handshake for non-AJAX requests if access token document is missing and request path is too large.', async () => {
-      const request = httpServerMock.createKibanaRequest({
-        path: `/s/foo/${'some-path'.repeat(10)}`,
-      });
-      const state = {
-        username: 'user',
-        accessToken: 'expired-token',
-        refreshToken: 'expired-refresh-token',
-      };
-
-      mockOptions.client.callAsInternalUser.withArgs('shield.samlPrepare').resolves({
-        id: 'some-request-id',
-        redirect: 'https://idp-host/path/login?SAMLRequest=some%20request%20',
-      });
-
-      mockScopedClusterClient(
-        mockOptions.client,
-        sinon.match({ headers: { authorization: `Bearer ${state.accessToken}` } })
-      )
-        .callAsCurrentUser.withArgs('shield.authenticate')
-        .rejects({
-          statusCode: 500,
-          body: { error: { reason: 'token document is missing and must be present' } },
-        });
-
-      mockOptions.tokens.refresh.withArgs(state.refreshToken).resolves(null);
-
-      const authenticationResult = await provider.authenticate(request, state);
-
-      sinon.assert.calledWithExactly(mockOptions.client.callAsInternalUser, 'shield.samlPrepare', {
-        body: {
-          acs: `test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml`,
-        },
-      });
-
-      expect(mockOptions.logger.warn).toHaveBeenCalledTimes(1);
-      expect(mockOptions.logger.warn).toHaveBeenCalledWith(
-        'Max URL path size should not exceed 100b but it was 107b. URL is not captured.'
-      );
-
-      expect(authenticationResult.redirected()).toBe(true);
-      expect(authenticationResult.redirectURL).toBe(
-        'https://idp-host/path/login?SAMLRequest=some%20request%20'
-      );
-      expect(authenticationResult.state).toEqual({ requestId: 'some-request-id', redirectURL: '' });
-    });
-
     it('re-capture URL for non-AJAX requests if refresh token is expired.', async () => {
       const request = httpServerMock.createKibanaRequest({ path: '/s/foo/some-path' });
       const state = {

diff --git a/x-pack/plugins/security/server/authentication/providers/token.test.ts b/x-pack/plugins/security/server/authentication/providers/token.test.ts
@@ -300,36 +300,6 @@ describe('TokenAuthenticationProvider', () => {
       expect(authenticationResult.error).toEqual(refreshError);
     });
 
-    it('redirects non-AJAX requests to /login and clears session if token document is missing', async () => {
-      const request = httpServerMock.createKibanaRequest({ path: '/some-path' });
-      const tokenPair = { accessToken: 'foo', refreshToken: 'bar' };
-
-      mockScopedClusterClient(
-        mockOptions.client,
-        sinon.match({ headers: { authorization: `Bearer ${tokenPair.accessToken}` } })
-      )
-        .callAsCurrentUser.withArgs('shield.authenticate')
-        .rejects({
-          statusCode: 500,
-          body: { error: { reason: 'token document is missing and must be present' } },
-        });
-
-      mockOptions.tokens.refresh.withArgs(tokenPair.refreshToken).resolves(null);
-
-      const authenticationResult = await provider.authenticate(request, tokenPair);
-
-      sinon.assert.calledOnce(mockOptions.tokens.refresh);
-
-      expect(request.headers).not.toHaveProperty('authorization');
-      expect(authenticationResult.redirected()).toBe(true);
-      expect(authenticationResult.redirectURL).toBe(
-        '/base-path/login?next=%2Fbase-path%2Fsome-path'
-      );
-      expect(authenticationResult.user).toBeUndefined();
-      expect(authenticationResult.state).toEqual(null);
-      expect(authenticationResult.error).toBeUndefined();
-    });
-
     it('redirects non-AJAX requests to /login and clears session if token cannot be refreshed', async () => {
       const request = httpServerMock.createKibanaRequest({ path: '/some-path' });
       const tokenPair = { accessToken: 'foo', refreshToken: 'bar' };

diff --git a/x-pack/plugins/security/server/authentication/tokens.test.ts b/x-pack/plugins/security/server/authentication/tokens.test.ts
@@ -41,10 +41,6 @@ describe('Tokens', () => {
       { statusCode: 401 },
       ElasticsearchErrorHelpers.decorateNotAuthorizedError(new Error()),
       new errors.AuthenticationException(),
-      {
-        statusCode: 500,
-        body: { error: { reason: 'token document is missing and must be present' } },
-      },
     ];
     for (const error of expirationErrors) {
       expect(Tokens.isAccessTokenExpiredError(error)).toBe(true);

diff --git a/x-pack/plugins/security/server/authentication/tokens.ts b/x-pack/plugins/security/server/authentication/tokens.ts
@@ -150,21 +150,10 @@ export class Tokens {
   /**
    * Tries to determine whether specified error that occurred while trying to authenticate request
    * using access token happened because access token is expired. We treat all `401 Unauthorized`
-   * as such. Another use case that we should temporarily support (until elastic/elasticsearch#38866
-   * is fixed) is when token document has been removed and ES responds with `500 Internal Server Error`.
+   * as such.
    * @param err Error returned from Elasticsearch.
    */
   public static isAccessTokenExpiredError(err?: any) {
-    const errorStatusCode = getErrorStatusCode(err);
-    return (
-      errorStatusCode === 401 ||
-      (errorStatusCode === 500 &&
-        !!(
-          err &&
-          err.body &&
-          err.body.error &&
-          err.body.error.reason === 'token document is missing and must be present'
-        ))
-    );
+    return getErrorStatusCode(err) === 401;
   }
 }