Skip to content
This repository has been archived by the owner on Apr 11, 2024. It is now read-only.

Make scopes optional for mangaged install token exchange apps #1333

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 8 additions & 11 deletions packages/shopify-api/lib/__tests__/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ describe('Config object', () => {

expect(config.apiKey).toEqual(validParams.apiKey);
expect(config.apiSecretKey).toEqual(validParams.apiSecretKey);
expect(config.scopes.equals(validParams.scopes)).toBeTruthy();
expect(config.hostName).toEqual(validParams.hostName);
});

Expand All @@ -54,16 +53,6 @@ describe('Config object', () => {
expect(error.message).toContain('Missing values for: apiSecretKey');
}

invalid = {...validParams};
invalid.scopes = [];
try {
validateConfig(invalid);
fail('Initializing without scopes did not throw an exception');
} catch (error) {
expect(error).toBeInstanceOf(ShopifyErrors.ShopifyError);
expect(error.message).toContain('Missing values for: scopes');
}

invalid = {...validParams};
invalid.hostName = '';
try {
Expand Down Expand Up @@ -97,6 +86,14 @@ describe('Config object', () => {
validParams.isCustomStoreApp = false;
});

it('scopes can be not defined', () => {
delete (validParams as any).scopes;

expect(() => validateConfig(validParams)).not.toThrow(
ShopifyErrors.ShopifyError,
);
});

it("ignores a missing 'apiKey' when isCustomStoreApp is true", () => {
validParams.isCustomStoreApp = true;
validParams.adminApiAccessToken = 'token';
Expand Down
17 changes: 16 additions & 1 deletion packages/shopify-api/lib/auth/oauth/oauth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
import {logger, ShopifyLogger} from '../../logger';
import {DataType} from '../../clients/types';
import {fetchRequestFactory} from '../../utils/fetch-request';
import {AuthScopes} from '../scopes';

import {
SESSION_COOKIE_NAME,
Expand Down Expand Up @@ -68,6 +69,10 @@ export function begin(config: ConfigInterface): OAuthBegin {
config.isCustomStoreApp,
'Cannot perform OAuth for private apps',
);
throwIfScopesUndefined(
config.scopes,
'Apps that use OAuth must define the required scopes in the config',
);
Comment on lines +72 to +75

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throwIfScopesUndefined(
config.scopes,
'Apps that use OAuth must define the required scopes in the config',
);
if (!config.scopes) {
throw new ShopifyErrors.MissingRequiredArgument('Apps that use OAuth must define the required scopes in the config',);
}

I'd rather check the config.scopes nullability here (the throwIfScopesUndefined is not used elsewhere) to avoid using config.scopes!.toString() and rely on TS type checking


const log = logger(config);
log.info('Beginning OAuth', {shop, isOnline, callbackPath});
Expand Down Expand Up @@ -101,7 +106,8 @@ export function begin(config: ConfigInterface): OAuthBegin {

const query = {
client_id: config.apiKey,
scope: config.scopes.toString(),
// If scopes is undefined, threw an error
scope: config.scopes!.toString(),
redirect_uri: `${config.hostScheme}://${config.hostName}${callbackPath}`,
state,
'grant_options[]': isOnline ? 'per-user' : '',
Expand Down Expand Up @@ -255,3 +261,12 @@ function throwIfCustomStoreApp(
throw new ShopifyErrors.PrivateAppError(message);
}
}

function throwIfScopesUndefined(
scopes: string | AuthScopes | undefined,
message: string,
): void {
if (!scopes) {
throw new ShopifyErrors.MissingRequiredArgument(message);
}
}
4 changes: 2 additions & 2 deletions packages/shopify-api/lib/base-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export interface ConfigParams<
*/
apiSecretKey: string;
/**
* The scopes your app needs to access the API.
* The scopes your app needs to access the API. Not required if using Shopify managed installation.
*/
scopes?: string[] | AuthScopes;
/**
Expand Down Expand Up @@ -121,7 +121,7 @@ export type ConfigInterface<Params extends ConfigParams = ConfigParams> = Omit<
> & {
apiKey: string;
hostScheme: 'http' | 'https';
scopes: AuthScopes;
scopes?: AuthScopes;
isCustomStoreApp: boolean;
billing?: BillingConfig<Params['future']>;
logger: {
Expand Down
2 changes: 0 additions & 2 deletions packages/shopify-api/lib/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ export function validateConfig<Params extends ConfigParams>(
const config = {
apiKey: '',
apiSecretKey: '',
scopes: new AuthScopes([]),
hostName: '',
hostScheme: 'https',
apiVersion: LATEST_API_VERSION,
Expand All @@ -30,7 +29,6 @@ export function validateConfig<Params extends ConfigParams>(
const mandatory: (keyof Params)[] = ['apiSecretKey', 'hostName'];
if (!('isCustomStoreApp' in params) || !params.isCustomStoreApp) {
mandatory.push('apiKey');
mandatory.push('scopes');
}
if ('isCustomStoreApp' in params && params.isCustomStoreApp) {
if (
Expand Down
48 changes: 37 additions & 11 deletions packages/shopify-api/lib/session/__tests__/session.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,20 +58,46 @@ describe('isActive', () => {

expect(session.isActive(shopify.config.scopes)).toBeTruthy();
});
});

it('returns false if session is not active', () => {
const shopify = shopifyApi(testConfig());
it('returns true when scopes that passed in empty and scopes are not equal', () => {
const session = new Session({
id: 'active',
shop: 'active-shop',
state: 'test_state',
isOnline: true,
scope: 'test_scope',
accessToken: 'indeed',
expires: new Date(Date.now() + 86400),
});

const session = new Session({
id: 'not_active',
shop: 'inactive-shop',
state: 'not_same',
isOnline: true,
scope: 'test_scope',
expires: new Date(Date.now() - 1),
});
expect(session.isActive(shopify.config.scopes)).toBeFalsy();
expect(session.isActive('')).toBeTruthy();
});

it('returns false if session is not active', () => {
const shopify = shopifyApi(testConfig());

const session = new Session({
id: 'not_active',
shop: 'inactive-shop',
state: 'not_same',
isOnline: true,
scope: 'test_scope',
expires: new Date(Date.now() - 1),
});
expect(session.isActive(shopify.config.scopes)).toBeFalsy();
});

it('returns false if checking scopes and scopes are not equal', () => {
const session = new Session({
id: 'not_active',
shop: 'inactive-shop',
state: 'not_same',
isOnline: true,
scope: 'test_scope',
expires: new Date(Date.now() - 1),
});
expect(session.isActive('fake_scope')).toBeFalsy();
});

describe('isExpired', () => {
Expand Down
13 changes: 9 additions & 4 deletions packages/shopify-api/lib/session/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,13 +172,18 @@ export class Session {
}

/**
* Whether the session is active. Active sessions have an access token that is not expired, and has the given scopes.
* Whether the session is active. Active sessions have an access token that is not expired, and has has the given
* scopes if checkScopes is true.
*/
public isActive(scopes: AuthScopes | string | string[]): boolean {
const usingScopes =

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that you need to add an additional check if scopes is instance of AuthScopes

scopes instanceofAuthScopes ? scopes.toArray().length > 0 : scopes !== '' && typeof scopes !== 'undefined' && scopes !== null

you could extract it to a different method

scopes !== '' && typeof scopes !== 'undefined' && scopes !== null;
const isScopeChanged = this.isScopeChanged(scopes);
const hasAccessToken = Boolean(this.accessToken);
const isTokenNotExpired = !this.isExpired();

return (
!this.isScopeChanged(scopes) &&
Boolean(this.accessToken) &&
!this.isExpired()
(!usingScopes || !isScopeChanged) && hasAccessToken && isTokenNotExpired
);
}

Expand Down
Loading