Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(singleton): allow oauth listener to be called with the latest config #13642

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

fix(singleton): allow oauth listener to be called with the latest config #13642

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bf080f6
fix: allow oauth notifier to be called with the latest config
israx Jul 24, 2024
59d5c84
chore: add unit tests
israx Jul 24, 2024
e2ed448
chore: update unit test
israx Jul 24, 2024
7fd2662
chore: bump bundle size
israx Jul 25, 2024
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
64 changes: 63 additions & 1 deletion packages/core/__tests__/singleton/Singleton.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,12 @@ import { Amplify } from '../../src/singleton';
import { AMPLIFY_SYMBOL, Hub } from '../../src/Hub';
import { AuthClass as Auth } from '../../src/singleton/Auth';
import { decodeJWT } from '../../src/singleton/Auth/utils';
import { CredentialsAndIdentityId } from '../../src/singleton/Auth/types';
import {
CognitoUserPoolConfig,
CredentialsAndIdentityId,
} from '../../src/singleton/Auth/types';
import { ResourcesConfig, fetchAuthSession } from '../../src';
import { ADD_OAUTH_LISTENER } from '../../src/libraryUtils';

Object.assign(global, { TextDecoder, TextEncoder });

Expand Down Expand Up @@ -255,6 +259,64 @@ describe('Amplify.configure() and Amplify.getConfig()', () => {
expect(config3).not.toBe(config);
expect(config3).not.toBe(config2);
});

describe('with oAuthListener', () => {
const mockCognitoConfig: CognitoUserPoolConfig = {
userPoolId: 'xxxxxxxx',
userPoolClientId: 'xxxxxxx',
};
const mockOAuthConfig: CognitoUserPoolConfig = {
...mockCognitoConfig,
loginWith: {
oauth: {
domain: 'domain',
redirectSignIn: ['localhost:3000'],
redirectSignOut: ['localhost:3000'],
scopes: ['profile'],
responseType: 'code',
},
},
};
const mockInvalidOAuthConfig: CognitoUserPoolConfig = {
...mockCognitoConfig,
loginWith: {
oauth: {
domain: 'invalidDomain',
redirectSignIn: ['localhost:3000'],
redirectSignOut: ['localhost:3000'],
scopes: ['profile'],
responseType: 'code',
},
},
};
const mockOAuthlistener = jest.fn();

beforeEach(() => {
Amplify[ADD_OAUTH_LISTENER](mockOAuthlistener);
jest.useFakeTimers();
});

afterAll(() => {
jest.useRealTimers();
});
it('should not call oAuthListener when OAuth config is not defined', async () => {
Amplify.configure({ Auth: { Cognito: mockCognitoConfig } });
expect(mockOAuthlistener).not.toHaveBeenCalled();
});

it('should call oAuthListener when OAuth config and oAuthListener are defined', async () => {
Amplify.configure({ Auth: { Cognito: mockOAuthConfig } });
jest.runAllTimers();
expect(mockOAuthlistener).toHaveBeenCalledWith(mockOAuthConfig);
});

it('should call oAuthListener with the latest config', async () => {
Amplify.configure({ Auth: { Cognito: mockInvalidOAuthConfig } });
Amplify.configure({ Auth: { Cognito: mockOAuthConfig } });
jest.runAllTimers();
expect(mockOAuthlistener).toHaveBeenCalledWith(mockOAuthConfig);
});
});
});

describe('Session tests', () => {
Expand Down
40 changes: 26 additions & 14 deletions packages/core/src/singleton/Amplify.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@ import {
} from './types';
import { AuthClass } from './Auth';
import { ADD_OAUTH_LISTENER } from './constants';
import { OAuthListener } from './Auth/types';

export class AmplifyClass {
private oAuthListener:
| ((authConfig: AuthConfig['Cognito']) => void)
| undefined = undefined;
private oAuthListener: OAuthListener | undefined = undefined;

resourcesConfig: ResourcesConfig;
libraryOptions: LibraryOptions;
private listenerTimeout?: ReturnType<typeof setTimeout>;

/**
* Cross-category Auth utilities.
Expand Down Expand Up @@ -53,7 +53,6 @@ export class AmplifyClass {
libraryOptions?: LibraryOptions,
): void {
const resolvedResourceConfig = parseAmplifyConfig(resourcesConfig);

this.resourcesConfig = resolvedResourceConfig;

if (libraryOptions) {
Expand All @@ -75,7 +74,10 @@ export class AmplifyClass {
AMPLIFY_SYMBOL,
);

this.notifyOAuthListener();
const config = this.resourcesConfig.Auth?.Cognito;
if (config?.loginWith?.oauth && this.oAuthListener) {
this.notifyOAuthListener(this, config, this.oAuthListener);
}
}

/**
Expand All @@ -98,17 +100,27 @@ export class AmplifyClass {
}
}

private notifyOAuthListener() {
if (
!this.resourcesConfig.Auth?.Cognito.loginWith?.oauth ||
!this.oAuthListener
) {
return;
private notifyOAuthListener(
amplify: AmplifyClass,
config: AuthConfig['Cognito'],
oAuthListener: OAuthListener,
Comment on lines +104 to +106
Copy link
Member

Choose a reason for hiding this comment

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

Making this pure would make sense if it were removed completely from the class, but since it is a private member of the class, this seems confusing. For example, it's not really clear to me why oAuthListener is passed separately from this. The clearListenerTimeout is then also tacked onto the instance but it's obfuscated here.

) {
if (amplify.listenerTimeout) {
amplify.clearListenerTimeout();
}
// The setTimeout will only be called as long as an oAuthLister and oauth config
// are defined. The code executed by the time out will be only fired once.
Comment on lines +111 to +112
Copy link
Member

Choose a reason for hiding this comment

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

This sounds like it only works with certain preconditions, e.g. assuming Amplify.configure() gets called back to back. While this trick seemed to be working, I'm hesitant to make this a solution...

With Amplify Gen2, amplify_outputs.json can be overridden from the backend.ts (reference). With Amplify Gen1, should be able to change the url in amplifyconfiguration.json as well, would be these be legit workarounds to solve what the PR is trying to solve?

Copy link
Member

Choose a reason for hiding this comment

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

I have to echo this sentiment. When I looked at this PR this was my initial concern as well. Code after the configure call that may have previously executed after the this.oAuthListener call was made may now execute after the call due to it being pulled out of the current execution cycle. While I'm not certain that is practically concerning, it feels quite risky.

amplify.listenerTimeout = setTimeout(() => {
oAuthListener(config);
// the listener should only be notified once with a valid oauth config
amplify.oAuthListener = undefined;
amplify.clearListenerTimeout();
});
}

this.oAuthListener(this.resourcesConfig.Auth?.Cognito);
// the listener should only be notified once with a valid oauth config
this.oAuthListener = undefined;
private clearListenerTimeout() {
clearTimeout(this.listenerTimeout);
this.listenerTimeout = undefined;
}
}

Expand Down
2 changes: 2 additions & 0 deletions packages/core/src/singleton/Auth/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,3 +259,5 @@ type AuthFlowType =
| 'CUSTOM_WITH_SRP'
| 'CUSTOM_WITHOUT_SRP'
| 'USER_PASSWORD_AUTH';

export type OAuthListener = (authConfig: AuthConfig['Cognito']) => void;
Loading