[Response Ops][Connectors] Add new user token SO#252501
[Response Ops][Connectors] Add new user token SO#252501jcger merged 23 commits intoelastic:connectors-auth-code-grantfrom
Conversation
| @@ -412,8 +416,7 @@ describe('delete()', () => { | |||
| ], | |||
| }; | |||
| unsecuredSavedObjectsClient.find.mockResolvedValueOnce(findResult); | |||
| const result = await connectorTokenClient.deleteConnectorTokens({ connectorId: '1' }); | |||
| expect(JSON.stringify(result)).toEqual(JSON.stringify([Symbol(), Symbol()])); | |||
| await connectorTokenClient.deleteConnectorTokens({ connectorId: '1' }); | |||
There was a problem hiding this comment.
we weren't using return result anywhere, we always call this function like in here. Therefore I felt confident to remove lines 415-416
…into issue-250979-user-token-so
…into issue-250979-user-token-so
There was a problem hiding this comment.
Pull request overview
This PR introduces support for per-user connector tokens (primarily OAuth tokens) by adding a new saved object type and refactoring the token client architecture. Previously, connector tokens were shared globally; now they can be stored per user via a profileUid.
Changes:
- Adds
user_connector_tokensaved object type with encryption support for personal OAuth credentials - Refactors
ConnectorTokenClientinto a facade pattern that delegates toSharedConnectorTokenClient(existing behavior) orUserConnectorTokenClient(new per-user tokens) - Updates token identifiers with
shared:andper-user:prefixes to enable routing between the two clients
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
x-pack/platform/plugins/shared/actions/server/types.ts |
Defines UserConnectorToken, OAuthPersonalCredentials, and UserConnectorOAuthToken types |
x-pack/platform/plugins/shared/actions/server/saved_objects/schemas/raw_user_connector_token/v1.ts |
Schema validation for user_connector_token saved objects |
x-pack/platform/plugins/shared/actions/server/saved_objects/model_versions/user_connector_token_model_versions.ts |
Model version definitions for user_connector_token |
x-pack/platform/plugins/shared/actions/server/saved_objects/mappings.ts |
Elasticsearch mappings for user_connector_token fields |
x-pack/platform/plugins/shared/actions/server/saved_objects/index.ts |
Registers user_connector_token as a saved object with encryption |
x-pack/platform/plugins/shared/actions/server/routes/oauth_callback.ts |
Adds user_connector_token to hidden types for OAuth callback handling |
x-pack/platform/plugins/shared/actions/server/plugin.ts |
Includes user_connector_token in hidden types for the plugin |
x-pack/platform/plugins/shared/actions/server/plugin.test.ts |
Test verification that user_connector_token is registered with encryption |
x-pack/platform/plugins/shared/actions/server/lib/user_connector_token_client.ts |
New client for managing per-user connector tokens |
x-pack/platform/plugins/shared/actions/server/lib/shared_connector_token_client.ts |
Extracted shared token logic from original ConnectorTokenClient |
x-pack/platform/plugins/shared/actions/server/lib/connector_token_client.ts |
Refactored as a facade that delegates to shared or user clients based on profileUid |
x-pack/platform/plugins/shared/actions/server/lib/get_oauth_authorization_code_access_token.ts |
Defensive type narrowing for refreshToken handling |
x-pack/platform/plugins/shared/actions/server/lib/connector_token_client.mock.ts |
Updated mock to use ConnectorTokenClientContract interface |
x-pack/platform/plugins/shared/stack_connectors/server/connector_types/microsoft_defender_endpoint/o_auth_token_manager.test.ts |
Updates import to use ConnectorTokenClientContract |
x-pack/platform/plugins/shared/stack_connectors/server/connector_types/microsoft_defender_endpoint/mocks.ts |
Adjusts mock implementation for new token client contract |
x-pack/platform/plugins/shared/stack_connectors/server/connector_types/crowdstrike/token_manager.test.ts |
Updates import and mock setup for new contract |
x-pack/platform/plugins/shared/actions/server/lib/user_connector_token_client.test.ts |
Comprehensive unit tests for UserConnectorTokenClient |
x-pack/platform/plugins/shared/actions/server/lib/shared_connector_token_client.test.ts |
Unit tests for SharedConnectorTokenClient |
x-pack/platform/plugins/shared/actions/server/lib/connector_token_client.test.ts |
Updated tests for facade delegation behavior |
x-pack/platform/plugins/shared/actions/server/constants/saved_objects.ts |
Exports USER_CONNECTOR_TOKEN_SAVED_OBJECT_TYPE constant |
packages/kbn-check-saved-objects-cli/current_mappings.json |
Adds mappings metadata for user_connector_token |
packages/kbn-check-saved-objects-cli/current_fields.json |
Adds indexed field list for user_connector_token |
x-pack/platform/plugins/shared/actions/server/lib/user_connector_token_client.ts
Outdated
Show resolved
Hide resolved
| .spyOn( | ||
| connectorTokenClient as unknown as { create: ConnectorTokenClientContract['create'] }, | ||
| 'create' | ||
| ) |
There was a problem hiding this comment.
The type assertion as unknown as { create: ConnectorTokenClientContract['create'] } is overly complex and obscures intent. Consider using jest.mocked(connectorTokenClient).create or defining a properly-typed mock at the call site instead of these nested type assertions.
There was a problem hiding this comment.
ConnectorTokenClient#create is an overloaded method (shared-token and per-user signatures). In this Jest setup, spyOn(...).mockImplementation(...) does not preserve overload inference reliably, so TypeScript rejects an otherwise valid test mock without additional narrowing. The as unknown as ConnectorTokenClientContract['create'] cast is therefore a test-only typing workaround to bind the mock implementation to the contract method type; it does not change runtime behavior, but we should keep this area under review and prefer a stricter typed helper when feasible.
.../plugins/shared/stack_connectors/server/connector_types/microsoft_defender_endpoint/mocks.ts
Show resolved
Hide resolved
...orm/plugins/shared/stack_connectors/server/connector_types/crowdstrike/token_manager.test.ts
Show resolved
Hide resolved
x-pack/platform/plugins/shared/actions/server/lib/connector_token_client.ts
Outdated
Show resolved
Hide resolved
x-pack/platform/plugins/shared/actions/server/lib/shared_connector_token_client.ts
Outdated
Show resolved
Hide resolved
…into issue-250979-user-token-so
c83b197 to
1c7b72e
Compare
d949bf5 to
9e0eccb
Compare
cnasikas
left a comment
There was a problem hiding this comment.
High-level approach LGMT! I will do a full review on the feature branch.
|
|
||
| private parseTokenId(id: string): { scope: 'per-user' | 'shared'; actualId: string } { | ||
| if (id.startsWith('per-user:')) { | ||
| return { scope: 'per-user', actualId: id.substring(9) }; |
There was a problem hiding this comment.
To avoid any mistakes, maybe it would be nice to have a constant and use replace. Example: id.replace(PER_USER_ID_PREFIX);
There was a problem hiding this comment.
What use case do you mean exactly? I would think that having this explicit startsWith checks makes it less error phrone than a generic replace
There was a problem hiding this comment.
Mostly because of the fixed number, it is easy to mess up. What about id.substring(PER_USER_ID_PREFIX.length)?
There was a problem hiding this comment.
I see that you did the same suggestion 🙂. All good!
x-pack/platform/plugins/shared/actions/server/lib/connector_token_client.ts
Show resolved
Hide resolved
Co-authored-by: Christos Nasikas <xristosnasikas@gmail.com>
⏳ Build in-progress, with failures
Failed CI Steps
Test Failures
History
|
paul-tavares
left a comment
There was a problem hiding this comment.
Changes to the Mocks/Tests for Crowdstrike + MS Defender Endpoint look good.
7bc6b62
into
elastic:connectors-auth-code-grant
## Description
Currently, all Kibana connectors use a shared service account for
authentication. This approach lacks per user level access support, as it
does not distinguish between individual users and service account user
levels of permission. To support more secure, flexible, and user-aware
integrations, we need to introduce per-user authentication for
connectors in Kibana, alongside the existing service account method.
## 2-step release
As there are changes that require a 2-step release, this PR won't add
`oauth_authorization_code` auth type to any connector type. Therefore,
it won't be usable for now. The changes that require a 2-step release
are:
- we are adding `refreshTokenExpiresAt` to AAD for `connector_token` SO
- we are adding `refreshToken` as an encrypted attribute for
`connector_token` SO
## Config to run this locally
```
uiSettings:
overrides:
'workflows:ui:enabled': true
server.publicBaseUrl: 'http://localhost:5601'
```
Also, the auth type needs to be used in a connector. Reach out privately
to get the necessary info.
## Involved PRs:
- #246655
- #251873
- #251717
- #252566
- #252104
- #252307
- #252262
- #252501
- #253606
- #254589
- #254916
- Rename rate limit kbn setting 15d2c19
- Fix refresh token 34708e5
---------
Co-authored-by: Sean Story <sean.story@elastic.co>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Lorena Bălan <lorena.balan@elastic.co>
Co-authored-by: Janki Salvi <117571355+js-jankisalvi@users.noreply.github.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Christos Nasikas <xristosnasikas@gmail.com>
Co-authored-by: Dennis Tismenko <dennis.tismenko@elastic.co>
## Description
Currently, all Kibana connectors use a shared service account for
authentication. This approach lacks per user level access support, as it
does not distinguish between individual users and service account user
levels of permission. To support more secure, flexible, and user-aware
integrations, we need to introduce per-user authentication for
connectors in Kibana, alongside the existing service account method.
## 2-step release
As there are changes that require a 2-step release, this PR won't add
`oauth_authorization_code` auth type to any connector type. Therefore,
it won't be usable for now. The changes that require a 2-step release
are:
- we are adding `refreshTokenExpiresAt` to AAD for `connector_token` SO
- we are adding `refreshToken` as an encrypted attribute for
`connector_token` SO
## Config to run this locally
```
uiSettings:
overrides:
'workflows:ui:enabled': true
server.publicBaseUrl: 'http://localhost:5601'
```
Also, the auth type needs to be used in a connector. Reach out privately
to get the necessary info.
## Involved PRs:
- elastic#246655
- elastic#251873
- elastic#251717
- elastic#252566
- elastic#252104
- elastic#252307
- elastic#252262
- elastic#252501
- elastic#253606
- elastic#254589
- elastic#254916
- Rename rate limit kbn setting 15d2c19
- Fix refresh token 34708e5
---------
Co-authored-by: Sean Story <sean.story@elastic.co>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Lorena Bălan <lorena.balan@elastic.co>
Co-authored-by: Janki Salvi <117571355+js-jankisalvi@users.noreply.github.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Christos Nasikas <xristosnasikas@gmail.com>
Co-authored-by: Dennis Tismenko <dennis.tismenko@elastic.co>
## Description
Currently, all Kibana connectors use a shared service account for
authentication. This approach lacks per user level access support, as it
does not distinguish between individual users and service account user
levels of permission. To support more secure, flexible, and user-aware
integrations, we need to introduce per-user authentication for
connectors in Kibana, alongside the existing service account method.
## 2-step release
As there are changes that require a 2-step release, this PR won't add
`oauth_authorization_code` auth type to any connector type. Therefore,
it won't be usable for now. The changes that require a 2-step release
are:
- we are adding `refreshTokenExpiresAt` to AAD for `connector_token` SO
- we are adding `refreshToken` as an encrypted attribute for
`connector_token` SO
## Config to run this locally
```
uiSettings:
overrides:
'workflows:ui:enabled': true
server.publicBaseUrl: 'http://localhost:5601'
```
Also, the auth type needs to be used in a connector. Reach out privately
to get the necessary info.
## Involved PRs:
- elastic#246655
- elastic#251873
- elastic#251717
- elastic#252566
- elastic#252104
- elastic#252307
- elastic#252262
- elastic#252501
- elastic#253606
- elastic#254589
- elastic#254916
- Rename rate limit kbn setting 15d2c19
- Fix refresh token 34708e5
---------
Co-authored-by: Sean Story <sean.story@elastic.co>
Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Lorena Bălan <lorena.balan@elastic.co>
Co-authored-by: Janki Salvi <117571355+js-jankisalvi@users.noreply.github.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Christos Nasikas <xristosnasikas@gmail.com>
Co-authored-by: Dennis Tismenko <dennis.tismenko@elastic.co>
This PR is part of a series of PRs, it will be merged into a feature branch
Summary
Closes #250979
User connector token SO
Added a new saved object type user_connector_token so connector tokens (e.g. OAuth) can be stored per user (by profileUid) instead of only globally. Includes mappings, model versions, and encrypted SO registration.
Token client split
Replaced the single implementation with two clients and a facade:
API
Existing ConnectorTokenClient methods now accept an optional profileUid. When present, operations are personal (user SO); when absent, they stay shared. Token IDs use prefixes personal: / shared: so the facade can route updates/deletes correctly.
Call sites
OAuth callback (and other callers that have a user context) can pass profileUid so tokens are stored per user. No change for callers that don’t pass profileUid.
Tests
Added unit tests for SharedConnectorTokenClient and UserConnectorTokenClient; updated ConnectorTokenClient tests for the facade and delegation. Removed the old assertion on deleteConnectorTokens return value because the implementation now returns Promise (refactor in this PR).
Misc
Explicit Promise return types and small cleanups (e.g. token id validation) in the shared/user clients.