implement identity client body #6635
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Coverage report
Show files with reduced coverage 🔻
Test suite run success3364 tests passing in 1378 suites. Report generated by 🧪jest coverage report action from 78b33dc |
|
|
||
| clientId(): string { | ||
| return '' | ||
| const environment = serviceEnvironment() |
There was a problem hiding this comment.
I think these methods are ripe for replace conditional with polymorphism:
serviceEnvironment().clientId()There was a problem hiding this comment.
I'll think about this a bit more, but its unfortunately more nuanced since we actually might want to use this client even in local development. Its now a case of:
- Prod -->
IdentityClient - Local + Identity service running -->
IdentityClient - Local + no identity -->
MockIdentityClient
There was a problem hiding this comment.
That could still be encapsulated behind the polymorphic interface though. If anything it's potentially even another argument in favor of it. I'm not sure if typescript enums allow definition of behaviour, but we could use a class instead (or whatever other language feature is best used to handle it)
// Conceptual pseudo code, may not actually work with enums but a conversion to a class or other type would certainly allow this kind of polymorphic definition
export enum Environment {
Local = 'local' {
function clientId() {
identityRunning() ? "value" : "other_value"
}
function applicationId(api) {
switch(api) { ... }
}
},
Production = 'production' {
function clientId() {
"production_value"
}
function applicationId(api) {
switch(api) { ... }
}
},
}
// usage
serviceEnvironment().clientId()
serviceEnvironment().applicationId(env)(this obviously isn't a huge deal at all, it's just a possible minor improvement to how we store this config data and allows you to avoid worrying about the else blocks and default blocks from within switch statements. I wouldn't burn much time on it tho)
|
|
||
| abstract refreshAccessToken(currentToken: IdentityToken): Promise<IdentityToken> | ||
|
|
||
| clientId(): string { |
There was a problem hiding this comment.
A smell to look out for. If nobody actually calls clientId against the abstract type (in this case your base client), then it probably doesn't belong in the abstract. You can probably just leave the two concrete implementations without having it live in the abstract - unless there is a use case coming up where it matters in the abstract
There was a problem hiding this comment.
yep good call. This can probably just be an abstract definition and the implementation can live in each client
There was a problem hiding this comment.
If nobody calls the clientId() method against the IdentityClient base type though, you don't get any value from it being defined as abstract. Abstract methods are best when the root type wants to force a child to implement something because that something matters to the contract of the base type. If the clientId is simply an implementation detail of "other things" in the child type, then there is no value in having the parent enforce the existence of a clientId
Another test of this to show it's not necessary: if you delete clientId() from the base type, nothing breaks or complains.
alexanderMontague
force-pushed
the
11-18-create_indentity_client_interface
branch
from
6ec2d7e to
bd49456
Compare
alexanderMontague
force-pushed
the
11-18-implement_identity_client_body
branch
from
157e36e to
c83c012
Compare
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
alexanderMontague
force-pushed
the
11-18-implement_identity_client_body
branch
from
c83c012 to
78b33dc
Compare
alexanderMontague
force-pushed
the
11-18-create_indentity_client_interface
branch
from
bd49456 to
ec62db8
Compare
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationspackages/cli-kit/dist/private/node/clients/identity/identity-client.d.tsimport { IdentityToken } from '../../session/schema.js';
import { TokenRequestResult } from '../../session/exchange.js';
import { API } from '../../api.js';
import { Result } from '../../../../public/node/result.js';
export declare abstract class IdentityClient {
abstract requestAccessToken(scopes: string[]): Promise<IdentityToken>;
abstract tokenRequest(params: {
[key: string]: string;
}): Promise<Result<TokenRequestResult, {
error: string;
store?: string;
}>>;
abstract refreshAccessToken(currentToken: IdentityToken): Promise<IdentityToken>;
abstract clientId(): string;
applicationId(api: API): string;
}
packages/cli-kit/dist/private/node/clients/identity/identity-mock-client.d.tsimport { IdentityClient } from './identity-client.js';
import { ApplicationToken, IdentityToken } from '../../session/schema.js';
import { ExchangeScopes, TokenRequestResult } from '../../session/exchange.js';
import { Result } from '../../../../public/node/result.js';
export declare class IdentityMockClient extends IdentityClient {
private readonly mockUserId;
private readonly authTokenPrefix;
requestAccessToken(_scopes: string[]): Promise<IdentityToken>;
exchangeAccessForApplicationTokens(_identityToken: IdentityToken, _scopes: ExchangeScopes, _store?: string): Promise<{
[x: string]: ApplicationToken;
}>;
tokenRequest(params: {
[key: string]: string;
}): Promise<Result<TokenRequestResult, {
error: string;
store?: string;
}>>;
refreshAccessToken(_currentToken: IdentityToken): Promise<IdentityToken>;
clientId(): string;
private readonly generateTokens;
private getFutureDate;
private getCurrentUnixTimestamp;
private encodeTokenPayload;
}
packages/cli-kit/dist/private/node/clients/identity/identity-service-client.d.tsimport { IdentityClient } from './identity-client.js';
import { IdentityToken } from '../../session/schema.js';
import { TokenRequestResult } from '../../session/exchange.js';
import { Result } from '../../../../public/node/result.js';
export declare class IdentityServiceClient extends IdentityClient {
requestAccessToken(scopes: string[]): Promise<IdentityToken>;
tokenRequest(params: {
[key: string]: string;
}): Promise<Result<TokenRequestResult, {
error: string;
store?: string;
}>>;
refreshAccessToken(currentToken: IdentityToken): Promise<IdentityToken>;
clientId(): string;
/**
* ========================
* Private Instance Methods
* ========================
*/
/**
* Initiate a device authorization flow.
* This will return a DeviceAuthorizationResponse containing the URL where user
* should go to authorize the device without the need of a callback to the CLI.
*
* Also returns a `deviceCode` used for polling the token endpoint in the next step.
*
* @param scopes - The scopes to request
* @returns An object with the device authorization response.
*/
private requestDeviceAuthorization;
/**
* Poll the Oauth token endpoint with the device code obtained from a DeviceAuthorizationResponse.
* The endpoint will return `authorization_pending` until the user completes the auth flow in the browser.
* Once the user completes the auth flow, the endpoint will return the identity token.
*
* Timeout for the polling is defined by the server and is around 600 seconds.
*
* @param code - The device code obtained after starting a device identity flow
* @param interval - The interval to poll the token endpoint
* @returns The identity token
*/
private pollForDeviceAuthorization;
}
packages/cli-kit/dist/private/node/clients/identity/instance.d.tsimport { IdentityClient } from './identity-client.js';
export declare function getIdentityClient(): IdentityClient;
Existing type declarationspackages/cli-kit/dist/private/node/session/device-authorization.d.ts@@ -1,4 +1,5 @@
import { IdentityToken } from './schema.js';
+import { Response } from 'node-fetch';
export interface DeviceAuthorizationResponse {
deviceCode: string;
userCode: string;
@@ -29,4 +30,17 @@ export declare function requestDeviceAuthorization(scopes: string[]): Promise<De
* @param interval - The interval to poll the token endpoint
* @returns The identity token
*/
-export declare function pollForDeviceAuthorization(code: string, interval?: number): Promise<IdentityToken>;
\ No newline at end of file
+export declare function pollForDeviceAuthorization(code: string, interval?: number): Promise<IdentityToken>;
+export declare function convertRequestToParams(queryParams: {
+ client_id: string;
+ scope: string;
+}): string;
+/**
+ * Build a detailed error message for JSON parsing failures from the authorization service.
+ * Provides context-specific error messages based on response status and content.
+ *
+ * @param response - The HTTP response object
+ * @param responseText - The raw response body text
+ * @returns Detailed error message about the failure
+ */
+export declare function buildAuthorizationParseErrorMessage(response: Response, responseText: string): string;
\ No newline at end of file
packages/cli-kit/dist/private/node/session/exchange.d.ts@@ -1,7 +1,7 @@
import { ApplicationToken, IdentityToken } from './schema.js';
import { API } from '../api.js';
import { Result } from '../../../public/node/result.js';
-import { ExtendableError } from '../../../public/node/error.js';
+import { AbortError, ExtendableError } from '../../../public/node/error.js';
export declare class InvalidGrantError extends ExtendableError {
}
export declare class InvalidRequestError extends ExtendableError {
@@ -65,4 +65,16 @@ export declare function exchangeDeviceCodeForAccessToken(deviceCode: string): Pr
export declare function requestAppToken(api: API, token: string, scopes?: string[], store?: string): Promise<{
[x: string]: ApplicationToken;
}>;
+export interface TokenRequestResult {
+ access_token: string;
+ expires_in: number;
+ refresh_token: string;
+ scope: string;
+ id_token?: string;
+}
+export declare function tokenRequestErrorHandler({ error, store }: {
+ error: string;
+ store?: string;
+}): AbortError | InvalidGrantError | InvalidRequestError;
+export declare function buildIdentityToken(result: TokenRequestResult, existingUserId?: string, existingAlias?: string): IdentityToken;
export {};
\ No newline at end of file
packages/cli-kit/dist/public/node/vendor/dev_server/dev-server-2024.d.ts@@ -4,6 +4,7 @@ export declare function createServer(projectName: string): {
url: (options?: HostOptions) => string;
};
declare function assertRunning2024(projectName: string): void;
+export declare function isRunning2024(projectName: string): boolean;
declare let assertRunningOverride: typeof assertRunning2024 | undefined;
export declare function setAssertRunning(override: typeof assertRunningOverride): void;
export {};
\ No newline at end of file
|
2 participants
WHY are these changes introduced?
Part of: shop/issues-develop#21594
WHAT is this pull request doing?