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

Initialize App Check debug mode in initializeAppCheck #5512

Merged
merged 6 commits into from
Sep 23, 2021
Merged

Initialize App Check debug mode in initializeAppCheck #5512

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2952e2f
Initialize debug mode on initializeAppCheck
hsubox76 Sep 14, 2021
86331a5
prettier
hsubox76 Sep 14, 2021
c5a90de
Address PR comments
hsubox76 Sep 17, 2021
76186e8
prettier
hsubox76 Sep 17, 2021
0c329f7
Address PR comments
hsubox76 Sep 20, 2021
675fb02
Fix debug initialize flag
hsubox76 Sep 22, 2021
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
9 changes: 3 additions & 6 deletions packages/app-check/src/api.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
*/
import '../test/setup';
import { expect } from 'chai';
import { match, spy, stub } from 'sinon';
import { spy, stub } from 'sinon';
import {
setTokenAutoRefreshEnabled,
initializeAppCheck,
Expand Down Expand Up @@ -128,7 +128,6 @@ describe('api', () => {
};
stub(indexeddb, 'writeDebugTokenToIndexedDB').callsFake(fakeWrite);
stub(indexeddb, 'readDebugTokenFromIndexedDB').resolves(token);
const logStub = stub(logger.logger, 'warn');
const consoleStub = stub(console, 'log');
self.FIREBASE_APPCHECK_DEBUG_TOKEN = true;
initializeAppCheck(app, {
Expand All @@ -139,13 +138,11 @@ describe('api', () => {
// written to indexedDB.
expect(await getDebugToken()).to.equal(token);
expect(consoleStub.args[0][0]).to.include(token);
expect(logStub).to.be.calledWith(match('is in debug mode'));
expect(logStub).to.be.calledWith(match(token));
self.FIREBASE_APPCHECK_DEBUG_TOKEN = undefined;
});
it('warns about debug mode on second call', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

What does it test?
I think we need a test to verify initializeDebugMode is called only once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wow, the test caught the fact that I was not setting the initialize property... thanks.

const logStub = stub(logger.logger, 'warn');
self.FIREBASE_APPCHECK_DEBUG_TOKEN = 'abcdefg';
const consoleStub = stub(console, 'log');
initializeAppCheck(app, {
provider: new ReCaptchaV3Provider(FAKE_SITE_KEY)
});
Expand All @@ -154,7 +151,7 @@ describe('api', () => {
});
const token = await getDebugToken();
expect(token).to.equal('abcdefg');
expect(logStub).to.be.calledWith(match('abcdefg'));
expect(consoleStub.args[0][0]).to.include(token);
self.FIREBASE_APPCHECK_DEBUG_TOKEN = undefined;
});

Expand Down
14 changes: 6 additions & 8 deletions packages/app-check/src/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ import {
} from './internal-api';
import { readTokenFromStorage } from './storage';
import { getDebugToken, initializeDebugMode, isDebugMode } from './debug';
import { logger } from './logger';

declare module '@firebase/component' {
interface NameServiceMapping {
Expand Down Expand Up @@ -67,14 +66,13 @@ export function initializeAppCheck(
// Log a warning when `initializeAppCheck()` is called in debug mode,
Copy link
Member

Choose a reason for hiding this comment

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

Comment needs update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

// and show the token.
if (isDebugMode()) {
logger.warn(
`App Check is in debug mode. To turn off debug mode, unset ` +
`the global variable FIREBASE_APPCHECK_DEBUG_TOKEN and ` +
`restart the app.`
// Do not block initialization to get the token for the message.
void getDebugToken().then(token =>
// Not using logger because I don't think we ever want this accidentally hidden.
console.log(
`App Check debug token: ${token}. You will need to add it to your app's App Check settings in the Firebase console for it to work.`
)
);
// Make this a separate console statement so user will at least have the
// first message if the token promise doesn't resolve in time.
void getDebugToken().then(token => logger.warn(`Debug token is ${token}.`));
}

if (provider.isInitialized()) {
Expand Down
4 changes: 0 additions & 4 deletions packages/app-check/src/storage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,6 @@ export async function readOrCreateDebugTokenFromStorage(): Promise<string> {
writeDebugTokenToIndexedDB(newToken).catch(e =>
logger.warn(`Failed to persist debug token to IndexedDB. Error: ${e}`)
);
// Not using logger because I don't think we ever want this accidentally hidden?
console.log(
`App Check debug token: ${newToken}. You will need to add it to your app's App Check settings in the Firebase console for it to work`
);
return newToken;
} else {
return existingDebugToken;
Expand Down