-
Notifications
You must be signed in to change notification settings - Fork 224
create identity client interface #6634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| import {ApplicationToken, IdentityToken} from '../../session/schema.js' | ||
alexanderMontague marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| import {ExchangeScopes} from '../../session/exchange.js' | ||
| import {API} from '../../api.js' | ||
|
|
||
| export abstract class IdentityClient { | ||
| abstract requestAccessToken(scopes: string[]): Promise<IdentityToken> | ||
|
|
||
| abstract exchangeAccessForApplicationTokens( | ||
| identityToken: IdentityToken, | ||
| scopes: ExchangeScopes, | ||
| store?: string, | ||
| ): Promise<{[x: string]: ApplicationToken}> | ||
alexanderMontague marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| abstract refreshAccessToken(currentToken: IdentityToken): Promise<IdentityToken> | ||
|
|
||
| clientId(): string { | ||
alexanderMontague marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return '' | ||
| } | ||
|
|
||
| applicationId(_api: API): string { | ||
| return '' | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| import {IdentityClient} from './identity-client.js' | ||
| import {ApplicationToken, IdentityToken} from '../../session/schema.js' | ||
| import {ExchangeScopes} from '../../session/exchange.js' | ||
|
|
||
| export class IdentityMockClient extends IdentityClient { | ||
| async requestAccessToken(_scopes: string[]): Promise<IdentityToken> { | ||
| return {} as IdentityToken | ||
| } | ||
|
|
||
| async exchangeAccessForApplicationTokens( | ||
| _identityToken: IdentityToken, | ||
| _scopes: ExchangeScopes, | ||
| _store?: string, | ||
| ): Promise<{[x: string]: ApplicationToken}> { | ||
| return {} | ||
| } | ||
|
|
||
| async refreshAccessToken(_currentToken: IdentityToken): Promise<IdentityToken> { | ||
| return {} as IdentityToken | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| import {IdentityClient} from './identity-client.js' | ||
| import {ApplicationToken, IdentityToken} from '../../session/schema.js' | ||
| import {ExchangeScopes} from '../../session/exchange.js' | ||
|
|
||
| export class IdentityServiceClient extends IdentityClient { | ||
| async requestAccessToken(_scopes: string[]): Promise<IdentityToken> { | ||
| return {} as IdentityToken | ||
| } | ||
|
|
||
| async exchangeAccessForApplicationTokens( | ||
| _identityToken: IdentityToken, | ||
| _scopes: ExchangeScopes, | ||
| _store?: string, | ||
| ): Promise<{[x: string]: ApplicationToken}> { | ||
| return {} | ||
| } | ||
|
|
||
| async refreshAccessToken(_currentToken: IdentityToken): Promise<IdentityToken> { | ||
| return {} as IdentityToken | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| import {Environment} from '../../context/service.js' | ||
| import {describe, expect, test, vi, beforeEach} from 'vitest' | ||
|
|
||
| const mockServiceEnvironment = vi.fn() | ||
| const mockIsRunning2024 = vi.fn() | ||
|
|
||
| vi.mock('../../context/service.js', async () => { | ||
| const actual = await vi.importActual('../../context/service.js') | ||
| return { | ||
| ...actual, | ||
| serviceEnvironment: (...args: unknown[]) => mockServiceEnvironment(...args), | ||
| } | ||
| }) | ||
|
|
||
| vi.mock('../../../../public/node/vendor/dev_server/dev-server-2024.js', async () => { | ||
| const actual = await vi.importActual('../../../../public/node/vendor/dev_server/dev-server-2024.js') | ||
| return { | ||
| ...actual, | ||
| isRunning2024: (...args: unknown[]) => mockIsRunning2024(...args), | ||
| } | ||
| }) | ||
|
|
||
| describe('getIdentityClient', () => { | ||
| beforeEach(async () => { | ||
| mockServiceEnvironment.mockReset() | ||
| mockIsRunning2024.mockReset() | ||
| vi.resetModules() | ||
| }) | ||
|
|
||
| test('returns IdentityServiceClient when environment is Production', async () => { | ||
| mockServiceEnvironment.mockReturnValue(Environment.Production) | ||
| mockIsRunning2024.mockReturnValue(false) | ||
|
|
||
| const {getIdentityClient} = await import('./instance.js') | ||
|
|
||
| const instance = getIdentityClient() | ||
|
|
||
| expect(instance.constructor.name).toBe('IdentityServiceClient') | ||
| }) | ||
|
|
||
| test('returns IdentityServiceClient when environment is Local and identity service is running', async () => { | ||
| mockServiceEnvironment.mockReturnValue(Environment.Local) | ||
| mockIsRunning2024.mockReturnValue(true) | ||
|
|
||
| const {getIdentityClient} = await import('./instance.js') | ||
|
|
||
| const instance = getIdentityClient() | ||
|
|
||
| expect(instance.constructor.name).toBe('IdentityServiceClient') | ||
| expect(mockIsRunning2024).toHaveBeenCalledWith('identity') | ||
| }) | ||
|
|
||
| test('returns IdentityMockClient when environment is Local and identity service is not running', async () => { | ||
| mockServiceEnvironment.mockReturnValue(Environment.Local) | ||
| mockIsRunning2024.mockReturnValue(false) | ||
|
|
||
| const {getIdentityClient} = await import('./instance.js') | ||
|
|
||
| const instance = getIdentityClient() | ||
|
|
||
| expect(instance.constructor.name).toBe('IdentityMockClient') | ||
| expect(mockIsRunning2024).toHaveBeenCalledWith('identity') | ||
| }) | ||
|
|
||
| test('returns the same instance on subsequent calls (singleton pattern)', async () => { | ||
| mockServiceEnvironment.mockReturnValue(Environment.Production) | ||
| mockIsRunning2024.mockReturnValue(false) | ||
|
|
||
| const {getIdentityClient} = await import('./instance.js') | ||
|
|
||
| const firstInstance = getIdentityClient() | ||
| const secondInstance = getIdentityClient() | ||
|
|
||
| expect(firstInstance).toBe(secondInstance) | ||
| expect(mockServiceEnvironment).toHaveBeenCalledTimes(1) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| import {IdentityClient} from './identity-client.js' | ||
| import {IdentityMockClient} from './identity-mock-client.js' | ||
| import {IdentityServiceClient} from './identity-service-client.js' | ||
| import {Environment, serviceEnvironment} from '../../context/service.js' | ||
| import {isRunning2024} from '../../../../public/node/vendor/dev_server/dev-server-2024.js' | ||
|
|
||
| let _identityClient: IdentityClient | undefined | ||
|
|
||
| export function getIdentityClient() { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really wanted this to live in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think is OK to have this here 👌 |
||
| if (!_identityClient) { | ||
| const isLocal = serviceEnvironment() === Environment.Local | ||
| const identityServiceRunning = isRunning2024('identity') | ||
| const client = isLocal && !identityServiceRunning ? new IdentityMockClient() : new IdentityServiceClient() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not just return client and get rid of the mutable variable?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just lends itself to the singleton pattern better. Saves re-instantiating on every call, but the clients are deterministic so we could do this too. Follows existing patterns, I don't feel too strongly about this. LMK what you think
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My opinion is that the tradeoff isn't worth it -- it introduces sequencing where the variable can be undefined, which has to be accounted for in the type as well as in all processes it's called from. The perf gains on compute will be near-zero, and may never actually become concrete unless you're calling identity multiple times in the same command lifecycle but different execution context.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How would you ever access the undefined variable?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The mutable variable is private, only the I think is ok, to have a singleton here. Is safe and I think there is any con about it. If eventually we decide that this client will have logic to prevent multiple refresh tokens operations from running at the same time, then we need to ensure everyone is using the same instance. (is an example, this is controlled somewhere else now) |
||
| _identityClient = client | ||
| } | ||
|
|
||
| return _identityClient | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.