fix: better scratch org create authentication and lint fixes (#1147)

src/config/ttlConfig.ts

@@ -44,6 +44,11 @@ export class TTLConfig<T extends TTLConfig.Options, P extends JsonMap> extends C
   }
 
   protected async init(): Promise<void> {
+    // Normally, this is done in super.init() but we don't call it to prevent
+    // redundant read() calls.
+    if (this.hasEncryption()) {
+      await this.initCrypto();
+    }
     const contents = await this.read(this.options.throwOnNotFound);
     const date = new Date().getTime();
 

src/org/authInfo.ts

@@ -1115,7 +1115,7 @@ export class AuthInfo extends AsyncOptionalCreatable<AuthInfo.Options> {
     // Exchange the auth code for an access token and refresh token.
     let authFields: TokenResponse;
     try {
-      this.logger.info(`Exchanging auth code for access token using loginUrl: ${options.loginUrl}`);
+      this.logger.debug(`Exchanging auth code for access token using loginUrl: ${options.loginUrl}`);
       authFields = await oauth2.requestToken(ensure(options.authCode));
     } catch (err) {
       const msg = err instanceof Error ? `${err.name}::${err.message}` : typeof err === 'string' ? err : 'UNKNOWN';

src/org/scratchOrgCache.ts

@@ -24,6 +24,7 @@ export type CachedOptions = {
 };
 
 export class ScratchOrgCache extends TTLConfig<TTLConfig.Options, CachedOptions> {
+  protected static readonly encryptedKeys = ['clientSecret'];
   public static getFileName(): string {
     return 'scratch-create-cache.json';
   }
@@ -34,6 +35,7 @@ export class ScratchOrgCache extends TTLConfig<TTLConfig.Options, CachedOptions>
       isState: true,
       filename: ScratchOrgCache.getFileName(),
       stateFolder: Global.SF_STATE_FOLDER,
+      encryptedKeys: ScratchOrgCache.encryptedKeys,
       ttl: Duration.days(1),
     };
   }

src/org/scratchOrgCreate.ts

@@ -111,7 +111,7 @@ export const scratchOrgResume = async (jobId: string): Promise<ScratchOrgCreateR
     emit({ stage: 'send request' }),
   ]);
   logger.debug(`resuming scratch org creation for jobId: ${jobId}`);
-  const cached = cache.get(jobId);
+  const cached = cache.get(jobId, true);
 
   if (!cached) {
     throw messages.createError('CacheMissError', [jobId]);
@@ -128,6 +128,11 @@ export const scratchOrgResume = async (jobId: string): Promise<ScratchOrgCreateR
     tracksSource,
   } = cached;
 
+  const signupTargetLoginUrl = signupTargetLoginUrlConfig ?? (await getSignupTargetLoginUrl());
+  if (signupTargetLoginUrl) {
+    logger.debug(`resuming org create with LoginUrl override= ${signupTargetLoginUrl}`);
+  }
+
   const hubOrg = await Org.create({ aliasOrUsername: hubUsername });
   const soi = await queryScratchOrgInfo(hubOrg, jobId);
 
@@ -145,7 +150,7 @@ export const scratchOrgResume = async (jobId: string): Promise<ScratchOrgCreateR
         scratchOrgInfoComplete: soi,
         hubOrg,
         clientSecret,
-        signupTargetLoginUrlConfig,
+        signupTargetLoginUrl,
         retry: 0,
       });
 
@@ -202,7 +207,7 @@ export const scratchOrgCreate = async (options: ScratchOrgCreateOptions): Promis
   /** epoch milliseconds */
   const startTimestamp = Date.now();
 
-  logger.debug('scratchOrgCreate');
+  logger.debug('preparing scratch org signup request...');
   await emit({ stage: 'prepare request' });
   const {
     hubOrg,
@@ -252,7 +257,7 @@ export const scratchOrgCreate = async (options: ScratchOrgCreateOptions): Promis
   const settings = await settingsGenerator.extract(scratchOrgInfo);
   logger.debug(`the scratch org def file has settings: ${settingsGenerator.hasSettings()}`);
 
-  const [scratchOrgInfoRequestResult, signupTargetLoginUrlConfig] = await Promise.all([
+  const [scratchOrgInfoRequestResult, signupTargetLoginUrl] = await Promise.all([
     // creates the scratch org info in the devhub
     requestScratchOrgCreation(hubOrg, scratchOrgInfo, settingsGenerator),
     getSignupTargetLoginUrl(),
@@ -289,7 +294,7 @@ export const scratchOrgCreate = async (options: ScratchOrgCreateOptions): Promis
     scratchOrgInfoComplete: soi,
     hubOrg,
     clientSecret,
-    signupTargetLoginUrlConfig,
+    signupTargetLoginUrl,
     retry: retry || 0,
   });
 
@@ -343,7 +348,13 @@ const getSignupTargetLoginUrl = async (): Promise<string | undefined> => {
   try {
     const project = await SfProject.resolve();
     const projectJson = await project.resolveProjectConfig();
-    return projectJson.signupTargetLoginUrl as string;
+    const signupTargetLoginUrl = projectJson.signupTargetLoginUrl;
+    if (signupTargetLoginUrl) {
+      Logger.childFromRoot('getSignupTargetLoginUrl').debug(
+        `Found signupTargetLoginUrl in project file: ${signupTargetLoginUrl as string}`
+      );
+      return signupTargetLoginUrl as string;
+    }
   } catch {
     // a project isn't required for org:create
   }

src/org/scratchOrgInfoApi.ts

@@ -5,8 +5,8 @@
  * For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
  */
 
+import { inspect } from 'node:util';
 import { env, Duration, upperFirst, omit } from '@salesforce/kit';
-
 import { AnyJson } from '@salesforce/ts-types';
 import { OAuth2Config, SaveResult } from '@jsforce/jsforce-node';
 import { retryDecorator, RetryError } from 'ts-retry-promise';
@@ -43,13 +43,13 @@ const errorCodes = Messages.loadMessages('@salesforce/core', 'scratchOrgErrorCod
  *
  * @param scratchOrgInfoComplete The completed ScratchOrgInfo
  * @param hubOrgLoginUrl the hun org login url
- * @param signupTargetLoginUrlConfig the login url
+ * @param signupTargetLoginUrl the login url
  * @returns {string}
  */
 const getOrgInstanceAuthority = function (
   scratchOrgInfoComplete: ScratchOrgInfo,
   hubOrgLoginUrl: string,
-  signupTargetLoginUrlConfig?: string
+  signupTargetLoginUrl?: string
 ): string {
   const createdOrgInstance = scratchOrgInfoComplete.SignupInstance;
 
@@ -65,7 +65,7 @@ const getOrgInstanceAuthority = function (
     altUrl = scratchOrgInfoComplete.LoginUrl;
   }
 
-  return signupTargetLoginUrlConfig ?? altUrl;
+  return signupTargetLoginUrl ?? altUrl;
 };
 
 /**
@@ -79,7 +79,7 @@ const buildOAuth2Options = async (options: {
   scratchOrgInfoComplete: ScratchOrgInfo;
   clientSecret?: string;
   retry?: number;
-  signupTargetLoginUrlConfig?: string;
+  signupTargetLoginUrl?: string;
 }): Promise<{
   options: OAuth2Config;
   retries: number;
@@ -92,11 +92,12 @@ const buildOAuth2Options = async (options: {
     loginUrl: getOrgInstanceAuthority(
       options.scratchOrgInfoComplete,
       options.hubOrg.getField(Org.Fields.LOGIN_URL),
-      options.signupTargetLoginUrlConfig
+      options.signupTargetLoginUrl
     ),
   };
 
   logger.debug(`isJwtFlow: ${isJwtFlow}`);
+  logger.debug(`using resolved loginUrl: ${oauth2Options.loginUrl}`);
 
   if (isJwtFlow && !process.env.SFDX_CLIENT_SECRET) {
     oauth2Options.privateKeyFile = options.hubOrg.getConnection().getAuthInfoFields().privateKey;
@@ -190,7 +191,7 @@ export const queryScratchOrgInfo = async (hubOrg: Org, id: string): Promise<Scra
  * scratchOrgInfoComplete - The completed ScratchOrgInfo which should contain an access token.
  * hubOrg - the environment hub org
  * clientSecret - The OAuth client secret. May be null for JWT OAuth flow.
- * signupTargetLoginUrlConfig - Login url
+ * signupTargetLoginUrl - Login url override
  * retry - auth retry attempts
  *
  * @returns {Promise<AuthInfo>}
@@ -199,10 +200,10 @@ export const authorizeScratchOrg = async (options: {
   scratchOrgInfoComplete: ScratchOrgInfo;
   hubOrg: Org;
   clientSecret?: string;
-  signupTargetLoginUrlConfig?: string;
+  signupTargetLoginUrl?: string;
   retry?: number;
 }): Promise<AuthInfo> => {
-  const { scratchOrgInfoComplete, hubOrg, clientSecret, signupTargetLoginUrlConfig, retry } = options;
+  const { scratchOrgInfoComplete, hubOrg, clientSecret, signupTargetLoginUrl, retry } = options;
   await emit({ stage: 'authenticate', scratchOrgInfo: scratchOrgInfoComplete });
   const logger = await Logger.child('authorizeScratchOrg');
   logger.debug(`scratchOrgInfoComplete: ${JSON.stringify(scratchOrgInfoComplete, null, 4)}`);
@@ -217,17 +218,47 @@ export const authorizeScratchOrg = async (options: {
     clientSecret,
     scratchOrgInfoComplete,
     retry,
-    signupTargetLoginUrlConfig,
+    signupTargetLoginUrl,
   });
 
-  const authInfo = await getAuthInfo({
-    hubOrg,
-    username: scratchOrgInfoComplete.SignupUsername,
-    oauth2Options: oAuth2Options.options,
-    retries: oAuth2Options.retries,
-    timeout: oAuth2Options.timeout,
-    delay: oAuth2Options.delay,
-  });
+  let authInfo: AuthInfo;
+
+  try {
+    // This will use the authCode from the scratch org signup to exchange for an auth token via OAuth.
+    authInfo = await getAuthInfo({
+      hubOrg,
+      username: scratchOrgInfoComplete.SignupUsername,
+      oauth2Options: oAuth2Options.options,
+      retries: oAuth2Options.retries,
+      timeout: oAuth2Options.timeout,
+      delay: oAuth2Options.delay,
+    });
+  } catch (err1) {
+    // If we didn't already try authenticating with the LoginUrl from ScratchOrgInfo object,
+    // try the oauth flow again using it now.
+    if (scratchOrgInfoComplete.LoginUrl && oAuth2Options.options.loginUrl !== scratchOrgInfoComplete.LoginUrl) {
+      logger.debug(
+        `Auth failed with loginUrl ${oAuth2Options.options.loginUrl} so trying with ${scratchOrgInfoComplete.LoginUrl}`
+      );
+      oAuth2Options.options = { ...oAuth2Options.options, ...{ loginUrl: scratchOrgInfoComplete.LoginUrl } };
+      try {
+        authInfo = await getAuthInfo({
+          hubOrg,
+          username: scratchOrgInfoComplete.SignupUsername,
+          oauth2Options: oAuth2Options.options,
+          retries: oAuth2Options.retries,
+          timeout: oAuth2Options.timeout,
+          delay: oAuth2Options.delay,
+        });
+      } catch (err2) {
+        // Log this error but throw the original error
+        logger.debug(`Auth failed with ScratchOrgInfo.LoginUrl ${scratchOrgInfoComplete.LoginUrl}\n${inspect(err2)}`);
+        throw err1;
+      }
+    } else {
+      throw err1;
+    }
+  }
 
   await authInfo.save({
     devHubUsername: hubOrg.getUsername(),

test/unit/config/ttlConfig.test.ts

@@ -11,6 +11,7 @@ import { Duration, sleep } from '@salesforce/kit';
 import { TTLConfig } from '../../../src/config/ttlConfig';
 import { TestContext } from '../../../src/testSetup';
 import { Global } from '../../../src/global';
+import { ScratchOrgCache } from '../../../src/org/scratchOrgCache';
 
 describe('TTLConfig', () => {
   const $$ = new TestContext();
@@ -38,6 +39,37 @@ describe('TTLConfig', () => {
     }
   }
 
+  class TestScratchOrgCache extends ScratchOrgCache {
+    public hasCryptoInitialized(): boolean {
+      return !!this.crypto;
+    }
+    public shouldEncryptKey(key: string): boolean {
+      return !!this.isCryptoKey(key);
+    }
+  }
+
+  describe('ScratchOrgCache', () => {
+    describe('set', () => {
+      it('should timestamp every entry', async () => {
+        const config = await TestScratchOrgCache.create();
+        config.set('123', { hubUsername: 'foo' });
+        const entry = config.get('123');
+        expect(entry).to.have.property('timestamp');
+        expect(config.hasCryptoInitialized()).to.be.true;
+      });
+      it('should encrypt clientSecret', async () => {
+        const clientSecret = '4947FFFDE29D89CFC3F';
+        const config = await TestScratchOrgCache.create();
+        config.set('123', { clientSecret });
+        expect(config.shouldEncryptKey('clientSecret')).to.be.true;
+        const nonDecryptedEntry = config.get('123');
+        expect(nonDecryptedEntry).to.have.property('clientSecret').and.not.equal(clientSecret);
+        const decryptedEntry = config.get('123', true);
+        expect(decryptedEntry).to.have.property('clientSecret', clientSecret);
+      });
+    });
+  });
+
   describe('set', () => {
     it('should timestamp every entry', async () => {
       const config = await TestConfig.create();