Add logs and comments, and tidy tests

Author: lisajian

packages/auth/src/core/auth/auth_impl.ts

@@ -63,6 +63,7 @@ import { _getUserLanguage } from '../util/navigator';
 import { _getClientVersion } from '../util/version';
 import { HttpHeader } from '../../api';
 import { AuthMiddlewareQueue } from './middleware';
+import { _logWarn } from '../util/log';
 
 interface AsyncAction {
   (): Promise<void>;
@@ -650,11 +651,6 @@ export class AuthImpl implements AuthInternal, _FirebaseService {
 
     // If the App Check service exists, add the App Check token in the headers
     const appCheckToken = await this._getAppCheckToken();
-    // TODO: What do we want to do if there is an error getting the token?
-    // Context: appCheck.getToken() will never throw even if an error happened.
-    // In the error case, a dummy token will be returned along with an error field describing
-    // the error. In general, we shouldn't care about the error condition and just use
-    // the token (actual or dummy) to send requests.
     if (appCheckToken) {
       headers[HttpHeader.X_FIREBASE_APP_CHECK] = appCheckToken;
     }
@@ -665,6 +661,15 @@ export class AuthImpl implements AuthInternal, _FirebaseService {
     const appCheckTokenResult = await this.appCheckServiceProvider
       .getImmediate({ optional: true })
       ?.getToken();
+    if (appCheckTokenResult?.error) {
+      // Context: appCheck.getToken() will never throw even if an error happened.
+      // In the error case, a dummy token will be returned along with an error field describing
+      // the error. In general, we shouldn't care about the error condition and just use
+      // the token (actual or dummy) to send requests.
+      _logWarn(
+        `Error while retrieving App Check token: ${appCheckTokenResult.error}`
+      );
+    }
     return appCheckTokenResult?.token;
   }
 }

packages/auth/src/core/util/handler.ts

@@ -121,6 +121,7 @@ export async function _getRedirectUrl(
     ? `${FIREBASE_APP_CHECK_FRAGMENT_ID}=${encodeURIComponent(appCheckToken)}`
     : '';
 
+  // Start at index 1 to skip the leading '&' in the query string
   return `${getHandlerBase(auth)}?${querystring(paramsDict).slice(
     1
   )}#${appCheckTokenFragment}`;

packages/auth/src/core/util/log.ts

@@ -37,6 +37,12 @@ export function _logDebug(msg: string, ...args: string[]): void {
   }
 }
 
+export function _logWarn(msg: string, ...args: string[]): void {
+  if (logClient.logLevel <= LogLevel.WARN) {
+    logClient.warn(`Auth (${SDK_VERSION}): ${msg}`, ...args);
+  }
+}
+
 export function _logError(msg: string, ...args: string[]): void {
   if (logClient.logLevel <= LogLevel.ERROR) {
     logClient.error(`Auth (${SDK_VERSION}): ${msg}`, ...args);

packages/auth/src/platform_browser/popup_redirect.test.ts

@@ -151,7 +151,7 @@ describe('platform_browser/popup_redirect', () => {
       const matches = (popupUrl as string).match(/.*?#(.*)/);
       expect(matches).not.to.be.null;
       const fragment = matches![1];
-      expect(fragment).not.to.include('fac');
+      expect(fragment).to.be.empty;
     });
 
     it('does not add the App Check token in the url fragment if controller unavailable', async () => {
@@ -165,7 +165,7 @@ describe('platform_browser/popup_redirect', () => {
       const matches = (popupUrl as string).match(/.*?#(.*)/);
       expect(matches).not.to.be.null;
       const fragment = matches![1];
-      expect(fragment).not.to.include('fac');
+      expect(fragment).to.be.empty;
     });
 
     it('throws an error if apiKey is unspecified', async () => {
@@ -259,7 +259,7 @@ describe('platform_browser/popup_redirect', () => {
       const matches = newWindowLocation.match(/.*?#(.*)/);
       expect(matches).not.to.be.null;
       const fragment = matches![1];
-      expect(fragment).not.to.include('fac');
+      expect(fragment).to.be.empty;
     });
 
     it('does not add the App Check token in the url fragment if controller unavailable', async () => {
@@ -279,7 +279,7 @@ describe('platform_browser/popup_redirect', () => {
       const matches = newWindowLocation.match(/.*?#(.*)/);
       expect(matches).not.to.be.null;
       const fragment = matches![1];
-      expect(fragment).not.to.include('fac');
+      expect(fragment).to.be.empty;
     });
 
     it('throws an error if authDomain is unspecified', async () => {