[Connectors] `ConnectorTokenClient` improvements #131955

ymao1 · 2022-05-10T16:53:42Z

Resolves #131231

Summary

Miscellaneous fixes to connector token client. Left comments on specific items.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

…g rejected promise

…ated

…sAt date

… token returns 4xx error

…ors/connector-token-client-updates

ymao1 · 2022-05-10T18:27:36Z

x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.ts

@@ -219,6 +214,24 @@ export class ConnectorTokenClient {
      );
      return { hasErrors: true, connectorToken: null };
    }
+
+    if (isNaN(Date.parse(connectorTokensResult[0].attributes.expiresAt))) {


If Date.parse(expiresAt) is NaN, then checking whether the token had expired would always return false, causing us to keep using the same access token repeatedly.

ymao1 · 2022-05-10T18:28:35Z

x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.ts

+        return Promise.reject(error);
+      }
+    );
+    axiosInstance.interceptors.response.use(


Added response interceptor to check for 4xx errors and delete any stored access tokens for this connector. Previously, if using an access token would result in a 4xx error, we would still continue to use it until it expired.

Tested this by:

Modifying x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.ts to print out the access token that's used (added console log on line 137)

Creating an OAuth servicenow connector via API using QA credentials

Testing it using test connector tab. See the access token being logged and see that a connector_token SO is created

Perform another test and see that the same access token is used as before

Using instructions here https://docs.servicenow.com/bundle/rome-platform-administration/page/administer/security/task/t_RevokeOAuthToken.html to revoke the token

Perform another test using test connector tab. See error returned and verify that no more connector_token SO exists for this connector ID

Perform another test using test connector tab. See that a new token should have been requested and verify that a new connector_token SO has been created.

ymao1 · 2022-05-10T18:28:41Z

x-pack/plugins/actions/server/builtin_action_types/lib/send_email.ts

@@ -113,6 +114,22 @@ async function sendEmailWithExchange(
    Authorization: accessToken,
  };

+  const axiosInstance = axios.create();
+  axiosInstance.interceptors.response.use(


Added response interceptor to check for 4xx errors and delete any stored access tokens for this connector. Previously, if using an access token would result in a 4xx error, we would still continue to use it until it expired.

Tested this by:

Modifying x-pack/plugins/actions/server/builtin_action_types/lib/send_email.ts to print out the access token that's used (added console log on line 108)

Creating an MS Exchange email connector (ask for OAuth credentials if needed) with from: responseops@elastictest267.onmicrosoft.com

Testing it using test connector tab. See the access token being logged and see that a connector_token SO is created

Perform another test and see that the same access token is used as before

Update the connector to change the from address to ying@elastictest267.onmicrosoft.com. You will have to reenter the client secret

Perform another test using test connector tab. See error returned and verify that no more connector_token SO exists for this connector ID

ymao1 · 2022-05-10T18:29:08Z

x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.ts

@@ -134,11 +134,28 @@ export const getAxiosInstance = ({
          tokenUrl: `${snServiceUrl}/oauth_token.do`,
          connectorTokenClient,
        });
+        if (!accessToken) {


Checking for null access token and throwing error

ymao1 · 2022-05-10T18:29:26Z

x-pack/plugins/actions/server/builtin_action_types/servicenow/utils.ts

        axiosConfig.headers.Authorization = accessToken;
        return axiosConfig;
      },
      (error) => {
-        Promise.reject(error);
+        return Promise.reject(error);


Forgot to correctly return the promise rejection 🙈

ymao1 · 2022-05-10T18:31:52Z

x-pack/plugins/actions/server/builtin_action_types/lib/get_oauth_jwt_access_token.ts

@@ -72,6 +72,9 @@ export const getOAuthJwtAccessToken = async ({
      keyId: jwtKeyId,
    });

+    // Save the time before requesting token so we can use it to calculate expiration
+    const requestTokenStart = Date.now();


using the time before requesting the token to calculate the expiration time of the token. previously, the expiration was calculated using a Date.now() inside connectorTokenClient.updateOrReplace, which occurs after the token has been requested and received. If there's any delay in processing between when the token was requested and when it was persisted to the connector_token saved object, it was possible to set an expiration that was past when the token would actually expire.

ymao1 · 2022-05-10T18:31:59Z

...plugins/actions/server/builtin_action_types/lib/get_oauth_client_credentials_access_token.ts

@@ -62,6 +62,9 @@ export const getOAuthClientCredentialsAccessToken = async ({
  }

  if (connectorToken === null || Date.parse(connectorToken.expiresAt) <= Date.now()) {
+    // Save the time before requesting token so we can use it to calculate expiration
+    const requestTokenStart = Date.now();


using the time before requesting the token to calculate the expiration time of the token. previously, the expiration was calculated using a Date.now() inside connectorTokenClient.updateOrReplace, which occurs after the token has been requested and received. If there's any delay in processing between when the token was requested and when it was persisted to the connector_token saved object, it was possible to set an expiration that was past when the token would actually expire.

elasticmachine · 2022-05-10T18:32:40Z

Pinging @elastic/response-ops (Team:ResponseOps)

azasypkin

Changes in utils.ts LGTM!

cnasikas · 2022-05-11T10:16:47Z

x-pack/plugins/actions/server/builtin_action_types/lib/connector_token_client.test.ts

@@ -375,6 +416,7 @@ describe('updateOrReplace()', () => {
      connectorId: '1',
      token: null,
      newToken: 'newToken',
+      tokenRequestDate: Date.now(),


I am curious, how do the test are working with different dates on each run?

Good catch! It looks like the existing unit tests are testing just whether certain values are passed to create and exclude testing the date. I will add a test to ensure that tokenRequestDate is used when it's passed in.

…ors/connector-token-client-updates

mikecote

I finished reviewing the code today, LGTM! Will pull locally and test tomorrow 👍

mikecote

Tested locally and changes LGTM!

kibana-ci · 2022-05-12T16:27:59Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💔 Build #44502 failed 23fd5a6
💚 Build #44131 succeeded a9f3c34
💚 Build #44032 succeeded 3673c23
💚 Build #43794 succeeded 3cc6bf8

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @ymao1

* Throwing error if service now access token is null. Properly returning rejected promise * Setting time to calculate token expiration to before the token is created * Returning null access token if stored access token has invalid expiresAt date * Adding response interceptor to delete connector token if using access token returns 4xx error * Adding test for tokenRequestDate * Handling 4xx errors in the response * Fixing unit tests * Fixing types

ymao1 added 5 commits May 9, 2022 15:01

Throwing error if service now access token is null. Properly returnin…

4cc3386

…g rejected promise

Setting time to calculate token expiration to before the token is cre…

4d197fb

…ated

Returning null access token if stored access token has invalid expire…

e2ec8df

…sAt date

Adding response interceptor to delete connector token if using access…

787376a

… token returns 4xx error

Merge branch 'main' of https://github.com/elastic/kibana into connect…

3cc6bf8

…ors/connector-token-client-updates

ymao1 changed the title ~~Connectors/connector token client updates~~ [Connectors] ConnectorTokenClient improvements May 10, 2022

ymao1 commented May 10, 2022

View reviewed changes

ymao1 self-assigned this May 10, 2022

ymao1 added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Actions/ConnectorsManagement Issues related to Connectors Management UX v8.3.0 labels May 10, 2022

ymao1 marked this pull request as ready for review May 10, 2022 18:32

ymao1 requested a review from a team as a code owner May 10, 2022 18:32

ymao1 requested review from cnasikas and azasypkin May 10, 2022 18:32

azasypkin approved these changes May 11, 2022

View reviewed changes

cnasikas reviewed May 11, 2022

View reviewed changes

cnasikas approved these changes May 11, 2022

View reviewed changes

ymao1 added 4 commits May 11, 2022 08:35

Merge branch 'main' of https://github.com/elastic/kibana into connect…

1e324ba

…ors/connector-token-client-updates

Adding test for tokenRequestDate

3673c23

Handling 4xx errors in the response

603a206

Fixing unit tests

a9f3c34

mikecote self-requested a review May 11, 2022 20:02

mikecote reviewed May 11, 2022

View reviewed changes

mikecote approved these changes May 12, 2022

View reviewed changes

ymao1 added 2 commits May 12, 2022 11:08

Merging in main

23fd5a6

Fixing types

ab56dc5

ymao1 merged commit b932dca into elastic:main May 12, 2022

kibanamachine added the backport:skip This commit does not require backporting label May 12, 2022

ymao1 deleted the connectors/connector-token-client-updates branch May 12, 2022 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Connectors] `ConnectorTokenClient` improvements #131955

[Connectors] `ConnectorTokenClient` improvements #131955

ymao1 commented May 10, 2022 •

edited

Loading

ymao1 May 10, 2022

ymao1 May 10, 2022

ymao1 May 11, 2022

ymao1 May 10, 2022

ymao1 May 11, 2022

ymao1 May 10, 2022

ymao1 May 10, 2022

ymao1 May 10, 2022

ymao1 May 10, 2022

elasticmachine commented May 10, 2022

azasypkin left a comment

cnasikas May 11, 2022

ymao1 May 11, 2022

cnasikas May 11, 2022

mikecote left a comment

mikecote left a comment

kibana-ci commented May 12, 2022

[Connectors] ConnectorTokenClient improvements #131955

[Connectors] ConnectorTokenClient improvements #131955

Conversation

ymao1 commented May 10, 2022 • edited Loading

Summary

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented May 10, 2022

azasypkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikecote left a comment

Choose a reason for hiding this comment

mikecote left a comment

Choose a reason for hiding this comment

kibana-ci commented May 12, 2022

💚 Build Succeeded

Metrics [docs]

History

[Connectors] `ConnectorTokenClient` improvements #131955

[Connectors] `ConnectorTokenClient` improvements #131955

ymao1 commented May 10, 2022 •

edited

Loading