Skip to content

Address (some) of the tree shaking issues w/ ReCaptcha #3277

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

Merged
merged 2 commits into from
Jun 24, 2020
Merged

Address (some) of the tree shaking issues w/ ReCaptcha #3277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
218e63b
Address (some) of the recaptcha tree-shaking issues
sam-gc Jun 24, 2020
d3e7170
Formatting
sam-gc Jun 24, 2020
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
11 changes: 9 additions & 2 deletions packages-exp/auth-exp/src/core/user/additional_user_info.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,14 +181,21 @@ describe('core/user/additional_user_info', () => {
});

it('for missing provider IDs in response but not in token', () => {
const idToken = 'algorithm.' + base64Encode(JSON.stringify({'firebase': {'sign_in_provider': 'facebook.com'}})) + '.signature';
const idToken =
'algorithm.' +
base64Encode(
JSON.stringify({
'firebase': { 'sign_in_provider': 'facebook.com' }
})
) +
'.signature';
const {
isNewUser,
providerId,
username,
profile
} = _fromIdTokenResponse(
idTokenResponse({ rawUserInfo: rawUserInfoWithLogin , idToken})
idTokenResponse({ rawUserInfo: rawUserInfoWithLogin, idToken })
)!;
expect(isNewUser).to.be.false;
expect(providerId).to.eq(ProviderId.FACEBOOK);
Expand Down
4 changes: 2 additions & 2 deletions packages-exp/auth-exp/src/platform_browser/recaptcha/recaptcha_loader.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import { AUTH_WINDOW } from '../auth_window';
import * as jsHelpers from '../load_js';
import {
_JSLOAD_CALLBACK,
MOCK_RECAPTCHA_LOADER,
MockReCaptchaLoaderImpl,
ReCaptchaLoader,
ReCaptchaLoaderImpl
} from './recaptcha_loader';
Expand All @@ -52,7 +52,7 @@ describe('platform-browser/recaptcha/recaptcha_loader', () => {

describe('MockLoader', () => {
it('returns a MockRecaptcha instance', async () => {
expect(await MOCK_RECAPTCHA_LOADER.load(auth)).to.be.instanceOf(
expect(await new MockReCaptchaLoaderImpl().load(auth)).to.be.instanceOf(
MockReCaptcha
);
});
Expand Down
5 changes: 1 addition & 4 deletions packages-exp/auth-exp/src/platform_browser/recaptcha/recaptcha_loader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,13 +123,10 @@ export class ReCaptchaLoaderImpl implements ReCaptchaLoader {
}
}

class MockReCaptchaLoaderImpl implements ReCaptchaLoader {
export class MockReCaptchaLoaderImpl implements ReCaptchaLoader {
async load(auth: Auth): Promise<Recaptcha> {
return new MockReCaptcha(auth);
}

clearedOneInstance(): void {}
}

export const MOCK_RECAPTCHA_LOADER: ReCaptchaLoader = new MockReCaptchaLoaderImpl();
export const RECAPTCHA_LOADER: ReCaptchaLoader = new ReCaptchaLoaderImpl();
24 changes: 10 additions & 14 deletions packages-exp/auth-exp/src/platform_browser/recaptcha/recaptcha_verifier.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import { Endpoint } from '../../api';
import { Auth } from '../../model/auth';
import { AUTH_WINDOW } from '../auth_window';
import { Parameters, Recaptcha } from './recaptcha';
import { _JSLOAD_CALLBACK, MOCK_RECAPTCHA_LOADER } from './recaptcha_loader';
import { _JSLOAD_CALLBACK, ReCaptchaLoader } from './recaptcha_loader';
import { MockReCaptcha } from './recaptcha_mock';
import { RecaptchaVerifier } from './recaptcha_verifier';

Expand All @@ -41,6 +41,7 @@ describe('platform_browser/recaptcha/recaptcha_verifier.ts', () => {
let container: HTMLElement;
let verifier: RecaptchaVerifier;
let parameters: Parameters;
let recaptchaLoader: ReCaptchaLoader;

beforeEach(async () => {
fetch.setUp();
Expand All @@ -54,6 +55,7 @@ describe('platform_browser/recaptcha/recaptcha_verifier.ts', () => {
mockEndpoint(Endpoint.GET_RECAPTCHA_PARAM, {
recaptchaSiteKey: 'recaptcha-key'
});
recaptchaLoader = verifier._recaptchaLoader;
});

afterEach(() => {
Expand All @@ -64,7 +66,7 @@ describe('platform_browser/recaptcha/recaptcha_verifier.ts', () => {
context('#render', () => {
it('caches the promise if not completed and returns if called multiple times', () => {
// This will force the loader to never return so the render promise never completes
sinon.stub(MOCK_RECAPTCHA_LOADER, 'load').returns(new Promise(() => {}));
sinon.stub(recaptchaLoader, 'load').returns(new Promise(() => {}));
const renderPromise = verifier.render();
expect(verifier.render()).to.eq(renderPromise);
});
Expand All @@ -81,17 +83,15 @@ describe('platform_browser/recaptcha/recaptcha_verifier.ts', () => {
});

it('sets loads the recaptcha per the app language code', async () => {
sinon.spy(MOCK_RECAPTCHA_LOADER, 'load');
sinon.spy(recaptchaLoader, 'load');
await verifier.render();
expect(MOCK_RECAPTCHA_LOADER.load).to.have.been.calledWith(auth, 'fr');
expect(recaptchaLoader.load).to.have.been.calledWith(auth, 'fr');
});

it('calls render on the underlying recaptcha widget', async () => {
const widget = new MockReCaptcha(auth);
sinon.spy(widget, 'render');
sinon
.stub(MOCK_RECAPTCHA_LOADER, 'load')
.returns(Promise.resolve(widget));
sinon.stub(recaptchaLoader, 'load').returns(Promise.resolve(widget));
await verifier.render();
expect(widget.render).to.have.been.calledWith(
container.children[0],
Expand All @@ -100,7 +100,7 @@ describe('platform_browser/recaptcha/recaptcha_verifier.ts', () => {
});

it('in case of error, resets render promise', async () => {
sinon.stub(MOCK_RECAPTCHA_LOADER, 'load').returns(Promise.reject('nope'));
sinon.stub(recaptchaLoader, 'load').returns(Promise.reject('nope'));
const promise = verifier.render();
await expect(promise).to.be.rejectedWith('nope');
expect(verifier.render()).not.to.eq(promise);
Expand All @@ -111,9 +111,7 @@ describe('platform_browser/recaptcha/recaptcha_verifier.ts', () => {
let recaptcha: Recaptcha;
beforeEach(() => {
recaptcha = new MockReCaptcha(auth);
sinon
.stub(MOCK_RECAPTCHA_LOADER, 'load')
.returns(Promise.resolve(recaptcha));
sinon.stub(recaptchaLoader, 'load').returns(Promise.resolve(recaptcha));
});

it('returns immediately if response is available', async () => {
Expand Down Expand Up @@ -161,9 +159,7 @@ describe('platform_browser/recaptcha/recaptcha_verifier.ts', () => {
context('#reset', () => {
it('calls reset on the underlying widget', async () => {
const recaptcha = new MockReCaptcha(auth);
sinon
.stub(MOCK_RECAPTCHA_LOADER, 'load')
.returns(Promise.resolve(recaptcha));
sinon.stub(recaptchaLoader, 'load').returns(Promise.resolve(recaptcha));
sinon.spy(recaptcha, 'reset');
await verifier.render();
verifier._reset();
Expand Down
31 changes: 17 additions & 14 deletions packages-exp/auth-exp/src/platform_browser/recaptcha/recaptcha_verifier.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,15 @@ import { initializeAuth } from '../../core/auth/auth_impl';
import { AuthErrorCode } from '../../core/errors';
import { assert } from '../../core/util/assert';
import { _isHttpOrHttps } from '../../core/util/location';
import { ApplicationVerifier } from '../../model/application_verifier';
import { Auth } from '../../model/auth';
import { AUTH_WINDOW } from '../auth_window';
import { Parameters, Recaptcha } from './recaptcha';
import {
MOCK_RECAPTCHA_LOADER,
RECAPTCHA_LOADER,
ReCaptchaLoader
MockReCaptchaLoaderImpl,
ReCaptchaLoader,
ReCaptchaLoaderImpl
} from './recaptcha_loader';
import { ApplicationVerifier } from '../../model/application_verifier';

const DEFAULT_PARAMS: Parameters = {
theme: 'light',
Expand All @@ -53,7 +53,7 @@ export class RecaptchaVerifier
private readonly tokenChangeListeners = new Set<TokenCallback>();
private renderPromise: Promise<number> | null = null;

private readonly recaptchaLoader: ReCaptchaLoader;
readonly _recaptchaLoader: ReCaptchaLoader;
private recaptcha: Recaptcha | null = null;

constructor(
Expand All @@ -75,9 +75,9 @@ export class RecaptchaVerifier
this.container = container;
this.parameters.callback = this.makeTokenCallback(this.parameters.callback);

this.recaptchaLoader = this.auth.settings.appVerificationDisabledForTesting
? MOCK_RECAPTCHA_LOADER
: RECAPTCHA_LOADER;
this._recaptchaLoader = this.auth.settings.appVerificationDisabledForTesting
? new MockReCaptchaLoaderImpl()
: new ReCaptchaLoaderImpl();

this.validateStartingState();
// TODO: Figure out if sdk version is needed
Expand All @@ -86,7 +86,7 @@ export class RecaptchaVerifier
async verify(): Promise<string> {
this.assertNotDestroyed();
const id = await this.render();
const recaptcha = this.assertedRecaptcha;
const recaptcha = this.getAssertedRecaptcha();

const response = recaptcha.getResponse(id);
if (response) {
Expand Down Expand Up @@ -134,14 +134,14 @@ export class RecaptchaVerifier
_reset(): void {
this.assertNotDestroyed();
if (this.widgetId !== null) {
this.assertedRecaptcha.reset(this.widgetId);
this.getAssertedRecaptcha().reset(this.widgetId);
}
}

clear(): void {
this.assertNotDestroyed();
this.destroyed = true;
this.recaptchaLoader.clearedOneInstance();
this._recaptchaLoader.clearedOneInstance();
if (!this.isInvisible) {
this.container.childNodes.forEach(node => {
this.container.removeChild(node);
Expand Down Expand Up @@ -193,7 +193,10 @@ export class RecaptchaVerifier
container = guaranteedEmpty;
}

this.widgetId = this.assertedRecaptcha.render(container, this.parameters);
this.widgetId = this.getAssertedRecaptcha().render(
container,
this.parameters
);
}

return this.widgetId;
Expand All @@ -203,7 +206,7 @@ export class RecaptchaVerifier
assert(_isHttpOrHttps() && !isWorker(), this.appName);

await domReady();
this.recaptcha = await this.recaptchaLoader.load(
this.recaptcha = await this._recaptchaLoader.load(
this.auth,
this.auth.languageCode || undefined
);
Expand All @@ -213,7 +216,7 @@ export class RecaptchaVerifier
this.parameters.sitekey = siteKey;
}

private get assertedRecaptcha(): Recaptcha {
private getAssertedRecaptcha(): Recaptcha {
assert(this.recaptcha, this.appName);
return this.recaptcha;
}
Expand Down