Connect to viya without CLIENT/SECRET

[allanbowe]

Summary

Adds sasjs auth login -t <target> — authenticate against SAS Viya with a regular SAS username/password, no registered OAuth client/secret required. Also hardens token handling for estates that issue opaque (non-JWT) refresh tokens and short-lived access tokens.

Verified end-to-end against a live estate (nextviya.emea.sas.com): sasjs auth login -t nextviya followed by sasjs run test.sas -t nextviya executed SAS code and fetched the log, with no client/secret anywhere.

Motivation

Every authenticated Viya flow funnels through getAuthConfig(), which hard-required a client and secret — obtaining one requires a SAS administrator to register an OAuth client in SASLogon, a common blocker on shared/demo/customer estates. A plain SAS username/password is in fact sufficient: the password grant against the built-in, secret-less sas.cli client (the same client the official SAS Viya CLI uses) returns a full user-impersonation token.

Design decision: the password grant is a dedicated login command that mints and persists a token pair; the password is never stored. All other commands consume the persisted token exactly as before.

Changes

sasjs auth login -t <target> (new command)

• SASVIYA targets only (clear error otherwise).
• Prompts for username and password (masked input; prompts added as a direct dependency).
• Mints a token pair via the password grant and verifies it with GET /identities/users/@currentUser (prints Logged in as <id> (<name>)).
• Persists the pair via saveTokens() (local .env.{target} or global ~/.sasjsrc).
• Backward compatible: bare sasjs auth -t <target> (no subcommand) still delegates to add cred, as before.

getAuthConfig() reorder + sas.cli silent refresh

• A fresh access token is returned before client/secret are required (previously it threw Client ID was not found even with a valid token present).
• When the token is expiring and no client is configured (SASVIYA only), the stored refresh token is silently refreshed via sas.cli and the rotated pair re-persisted. Error messages now point at sasjs auth login.
• This also fixes sasjs deploy (deployToSasViyaWithServicePack) with a pre-minted ACCESS_TOKEN.

saveTokens() — client/secret now optional

saveTokens(targetName, access_token, refresh_token, client?, secret?). For password-grant logins only the tokens are written — no fabricated CLIENT=sas.cli values poisoning later runs.

Opaque-token & type hardening (see PLAN-opaque-token-hardening.md)

• Local jwtDecode-based expiry helpers deleted; the CLI now uses the guarded helpers from @sasjs/utils/auth (opaque tokens are treated as usable; the server is the authority on expiry).
• as any / non-null casts around client/secret-less AuthConfig removed — client/secret are now optional in @sasjs/utils.
• executeScript call sites (run, fs, servicepack deploy) pass onTokensRefreshed: persistTokensRefreshedByAdapter(target) so refresh tokens rotated by adapter-internal refreshes (e.g. during long-running jobs) are persisted via saveTokens instead of silently going stale.

Tests

• src/commands/auth/spec/authCommand.spec.ts: parsing, login dispatch, legacy bare sasjs auth → addCredential.
• src/utils/spec/config.spec.ts: fresh token returned without client/secret; expiring-token error mentions sasjs auth login; opaque refresh token through both the client/secret and sas.cli branches (no InvalidTokenError, refresh attempted, rotated pair persisted); adapter-refresh persistence handler.

Dependencies — ⚠️ merge blockers

This PR depends on two companion PRs, and package.json currently references local tarballs as a temporary dev measure:

• sasjs/utils fix/opaque-refresh-tokens (opaque-token guard + optional AuthConfig.client/secret) — merge & release, then bump here.
• sasjs/adapter feat/get-tokens-cli-client-and-refresh-callback (sas.cli refresh fallback + onTokensRefreshed threaded through the compute-execution path) — merge & release, then bump here.
• Replace the file: tarball references with released semver ranges and regenerate package-lock.json before merge.
• Remove PLAN-password-grant-auth.md and PLAN-opaque-token-hardening.md from the repo before merge (working documents, not for main).

Limitations / notes

• When the access token ultimately expires and refresh fails, re-run sasjs auth login -t <target>.
• Requires the password grant to be enabled for sas.cli (default on Viya 3.5+/4) and a local/LDAP account — cannot work on SSO/SAML/MFA-only estates. ROPC is deprecated in OAuth 2.1; this flow is intended for dev/demo estates. CI pipelines should still use a properly registered client/secret.
• On cold estates the first compute session creation can take many minutes (pod spin-up) — a sasjs run may appear to hang.

[4gl-reviewer]

Hermes Agent Code Review

Verdict: Request Changes

Critical

• Unpublished file: tarball dependencies (package.json:72,75): @sasjs/adapter and @sasjs/utils point at local tarballs (file:../adapter/build/sasjs-adapter-4.17.3.tgz, file:../utils/build/sasjs-utils-3.5.9.tgz) that don't exist in the repo. npm install fails with ENOENT for anyone who hasn't manually built those sibling repos. The PR's own PLAN doc says these "must be replaced with released semver ranges before this branch ships." This is a release blocker — the branch cannot be installed or CI-tested as-is.

Warnings

• saveTokens global-config branch preserves stale client/secret (src/utils/config.ts:789-794): When saveTokens is called without client/secret (password-grant login), the ...(targetJson.authConfig || {}) spread preserves any pre-existing client/secret in ~/.sasjsrc. The .env.{target} branch was fixed to omit them, but the global-config branch was not. A user who previously authenticated with client/secret and then runs sasjs auth login -t <globalTarget> will have rotated tokens saved but the old client/secret left in authConfig — the next getAuthConfig will take the client/secret refresh branch instead of sas.cli. Fix: When client is falsy, explicitly clear client/secret from targetJson.authConfig.

• getAuthConfig fresh-token early return yields secret: undefined (src/utils/config.ts:651): secret is hardcoded to undefined on this path because it isn't computed until later. If a target has CLIENT set but SECRET resolved from env/config, the returned AuthConfig carries {client, secret: undefined}. This is the exact "type lie" the PLAN's "Problem 2" calls out — if any consumer refreshes with that pair, the basic-auth header becomes base64("client:undefined"). Fix: Compute secret before this early return.

• login.ts calls process.exit(1) on prompt cancellation (src/commands/auth/login.ts:49): Hard-kills the process from inside a library function, bypassing AuthCommand.executeLogin()'s .catch and ReturnCode contract, any finally/cleanup, and test harnesses. The rest of the codebase returns ReturnCode values. Fix: Throw an error from onCancel and let executeLogin's catch handle it.

• fetchLoggedInUser can return id: undefined while declaring id: string (src/utils/auth.ts:146): result?.id is any | undefined but the return type says id: string. The caller (login.ts:60) prints Logged in as ${id}... → "Logged in as undefined". Also, fetchLoggedInUser runs before saveTokens, so if it throws, the freshly-minted tokens are lost. Fix: Throw if !result?.id; consider saving tokens before verification.

Suggestions

• No unit tests for getTokensWithPasswordGrant / fetchLoggedInUser (src/utils/auth.ts): The security-critical HTTP call (correct basic-auth header, URL-encoded body, error handling) has no direct unit tests. Add tests mocking SasjsRequestClient.post/get asserting: (a) Authorization: Basic base64("sas.cli:"), (b) body is grant_type=password&..., (c) 400 invalid_grant → friendly error, (d) fetchLoggedInUser throws on missing id.
• as any on token response (src/utils/auth.ts:105): Type the post<SasAuthResponse>(...) call and drop as any.
• Password could leak via error interpolation (src/utils/auth.ts:112): ${err?.message || err} is fragile — if a future axios change attaches config.data to the error, the password would be logged. Surface a sanitized summary instead.
• console.error vs process.logger (src/commands/auth/login.ts:48): Use process.logger?.error(...) for consistency.
• persistTokensRefreshedByAdapter captures stale target (src/utils/config.ts:814): The target is captured at call time; getAuthConfig/saveTokens write to disk but don't mutate the in-memory target. Re-read the target from config inside the callback for freshest credentials.
• jest.mock() inside beforeEach (src/commands/auth/spec/authCommand.spec.ts:68-69): jest.mock() is hoisted; calling it inside beforeEach is a no-op. Remove it or move to top-level.
• AuthCommand example has empty description (src/commands/auth/authCommand.ts): description: '' renders as a bare entry in --help. Add a description or drop the duplicate.
• passwordGrantHint may render sasjs auth login -t undefined (src/utils/config.ts:641): Guard against target being undefined.
• Rotated refresh token lost if saveTokens fails (src/utils/config.ts): Both refresh branches rotate the server-side token then await saveTokens(...). If saveTokens throws (disk error), the rotated token is lost. Add a retry or clear warning.
• saveTokens signature change (src/utils/config.ts): Parameter order changed from (name, client, secret, access, refresh) to (name, access, refresh, client?, secret?). Note this breaking change in the changelog.

Looks Good

• Core sas.cli password-grant auth logic is correct — basic-auth header, URL-encoded body, endpoint path all verified.
• Opaque-token guard delegation to @sasjs/utils/auth is correct.
• onTokensRefreshed wiring through run, fs/executeCode, and deploy is consistent.
• config.spec.ts opaque-token tests are well-constructed.
• Wrong-password error flow (invalid_grant → LoginRequiredError → friendly message) is correct.

Note: The failing test (22) CI check is an npm ci failure due to the missing local tarball deps (see Critical finding above) — not a code logic failure.

Reviewed by Hermes Agent (GitHub App)

[4gl-reviewer]

🚫 Release blocker: unpublished file: dependency

"@sasjs/adapter": "file:../adapter/build/sasjs-adapter-4.17.3.tgz" points at a local tarball that doesn't exist in this repo. npm install fails with ENOENT. The same applies to @sasjs/utils on line 75. These must be replaced with published semver ranges before merging. The PR's own PLAN doc acknowledges this.

[4gl-reviewer]

⚠️ secret: undefined on fresh-token path

secret is hardcoded to undefined here because it isn't computed until later (line ~696). If a target has CLIENT set but SECRET resolved from env/config, the returned AuthConfig carries {client, secret: undefined}.

This is the exact "type lie" the PLAN's Problem 2 calls out — if any consumer refreshes with that pair, the basic-auth header becomes base64("client:undefined").

Fix: Compute secret (and finish the null/'null'/'undefined' sanitization) before this early return.

[4gl-reviewer]

⚠️ process.exit(1) bypasses ReturnCode contract

Hard-killing the process from inside a library function skips AuthCommand.executeLogin()'s .catch, any finally/cleanup, and test harnesses. The rest of the codebase returns ReturnCode values.

Fix:

onCancel: () => { throw new Error('Input cancelled.') }

Let executeLogin's catch convert it to ReturnCode.InternalError (or add ReturnCode.Cancelled).

[4gl-reviewer]

⚠️ id: result?.id can be undefined while return type says id: string

The caller (login.ts:60) prints Logged in as ${id}... → "Logged in as undefined" if the identities endpoint returns an unexpected shape.

Also, fetchLoggedInUser runs before saveTokens (login.ts:60 vs :62), so if it throws, the freshly-minted tokens are lost and the user must re-enter their password.

Fix:

if (!result?.id) throw new Error('Login succeeded but the identity endpoint returned no user id.')
return { id: result.id, name: result?.name }

Consider saving tokens before verifying, so a transient identities-endpoint error doesn't waste the grant.

[4gl-reviewer]

💡 as any loses type safety

The codebase has a SasAuthResponse type. Type the post<SasAuthResponse>(...) call and drop .then((res) => res.result as any).

[4gl-reviewer]

💡 Potential password leak via error interpolation

The password is in the request body (URLSearchParams). Today the adapter's handleError only exposes response.data, not config.data, so the password doesn't leak. But ${err?.message || err} is fragile — if a future axios/adapter change attaches config.data to the error message, the password would be logged by process.logger?.error in authCommand.ts.

Fix: Surface a sanitized summary (HTTP status) and log the full error separately at debug level only.

[4gl-reviewer]

Hermes Agent Code Review

Verdict: Request Changes

This PR adds a sasjs auth login command that authenticates against SAS Viya using a password grant (no client/secret required), plus comprehensive changes to getAuthConfig/saveTokens to support token-only auth. The overall design is solid — the sas.cli public client approach, rotating refresh token persistence, and the onTokensRefreshed callback integration are well thought out. However, there is one functional issue that should be addressed before merging.

Critical

• --insecure flag silently ignored by sasjs auth login — The AuthCommand accepts --insecure/-i but executeLogin() never passes it to authLogin(). This means login will fail on Viya servers with self-signed certificates even when the user passes --insecure, because getTokensWithPasswordGrant uses target.httpsAgentOptions as-is without setting rejectUnauthorized: false. See inline comment on login.ts.

Warnings

• getTokensWithPasswordGrant return type mismatch — authResponse.refresh_token is typed as string | undefined (from the response cast), but the return type declares refresh_token: string. If a Viya deployment doesn't return a refresh token (unlikely but possible), the caller gets undefined where a string is expected. See inline comment on auth.ts.

Suggestions

• getAuthConfig early return returns secret: undefined even when client has a value — When a fresh access token is available and a client is configured but no secret, the early return produces { client, secret: undefined }. This is inconsistent — if client is set, secret should also be resolved (or both should be undefined). See inline comment on config.ts.
• addCommand.spec.ts test removal — The two removed tests for sasjs auth as an alias of sasjs add cred are now covered by the new authCommand.spec.ts. This is fine, but worth noting that the legacy alias behavior is now tested in a different file.

Looks Good

• Comprehensive test coverage in config.spec.ts for the new getAuthConfig branches (token-only auth, sas.cli refresh, opaque tokens, persistTokensRefreshedByAdapter).
• The saveTokens signature change (access_token/refresh_token first, client/secret optional) correctly supports both password-grant and client/secret auth flows.
• The persistTokensRefreshedByAdapter handler correctly falls back to process.env.CLIENT/process.env.SECRET when the target's authConfig doesn't have them.
• Help text and command aliases are properly updated.

Reviewed by Hermes Agent (GitHub App)

[4gl-reviewer]

Critical: --insecure flag is silently ignored.

The AuthCommand accepts --insecure/-i (see authCommand.ts parseOptions), but executeLogin() calls authLogin(target) without passing the insecure flag. This means:

1. Functional bug: Login will fail on Viya servers with self-signed certificates even when the user passes --insecure, because getTokensWithPasswordGrant uses target.httpsAgentOptions as-is without setting rejectUnauthorized: false.

2. Compare with executeCred() which correctly passes this.insecure to addCredential(target, this.insecure, scope).

Suggested fix:

public async executeLogin() {
  const { target } = await this.getTargetInfo()
  return await authLogin(target, this.insecure)
    .then(() => ReturnCode.Success)
    .catch((err) => { ... })
}

And in login.ts:

export const authLogin = async (target: Target, insecure = false): Promise<void> => {
  // ...
  const { access_token, refresh_token } = await getTokensWithPasswordGrant(
    target, user, pass, insecure
  )
}

Then in getTokensWithPasswordGrant, apply rejectUnauthorized: false to httpsAgentOptions when insecure is true (same pattern as addCredential.ts).

[4gl-reviewer]

Warning: refresh_token could be undefined but is typed as string.

authResponse.refresh_token is string | undefined (from the cast on line 105), but the return type Promise<{ access_token: string; refresh_token: string }> declares it as required string.

If a Viya deployment doesn't return a refresh token (unlikely for a password grant, but the type allows it), the caller gets undefined where a string is expected. Downstream code like saveTokens(target.name, access_token, refresh_token || '') would handle it, but the type contract is violated.

Suggested fix:

return {
  access_token: authResponse.access_token as string,
  refresh_token: authResponse.refresh_token || ''
}

[4gl-reviewer]

Suggestion: Inconsistent secret: undefined when client has a value.

When a fresh access token is available, this early return produces { client, secret: undefined }. If client came from target.authConfig.client or process.env.CLIENT, it will be set while secret is hardcoded to undefined.

This isn't a bug per se (a fresh access token doesn't need a secret), but it's an inconsistent state that could confuse downstream consumers. Consider either:

• Also resolving secret here (same as lines 696-710), or
• Setting client: undefined too when no secret is available

[4gl-reviewer]

Hermes Agent Code Review

Verdict: Request Changes — the build is broken (TypeScript compile error in src/utils/auth.ts), so this PR cannot merge as-is. The design and test coverage are strong; once the type error below is fixed, this is in good shape.

Critical

1. Build failure: src/utils/auth.ts:126 — Type 'string | undefined' is not assignable to type 'string'.
   This is the exact failure in the CI test job (see run 32126184768). I reproduced it locally with npx tsc -p . --noEmit after npm ci.

   The return type of getTokensWithPasswordGrant is Promise<{ access_token: string; refresh_token: string }> but line 126 returns authResponse.refresh_token, which is typed string | undefined (the .then cast on line 106 declares both fields optional). Fix:

   return {
     access_token: authResponse.access_token as string,
     refresh_token: authResponse.refresh_token || ''
   }

   login.ts:61 already guards with refresh_token || '', so an empty string is a safe sentinel here.

Warnings

2. fetchLoggedInUser swallows HTTP errors silently (auth.ts:144-148). requestClient.get<any>(...) has no .catch, so a 401/403/network failure throws an unstructured adapter error straight to the user. Consider a .catch that wraps the message, mirroring the getTokensWithPasswordGrant error handling, e.g. Unable to verify the access token against ${target.serverUrl}: ${err?.message || err}.

3. getAccessToken (non-persisting variant) does not support password-grant tokens. The new no-client/sas.cli refresh path was added to getAuthConfig but not to getAccessToken. This is currently fine — getAccessToken is only called from src/utils/test.ts test support code and its own specs, and its doc comment correctly warns against using it for user-facing commands. Flagging so it's a conscious decision: any future caller of getAccessToken on a password-grant-only target will hit the existing Client ID was not found error.

Suggestions

4. No unit tests for login.ts / getTokensWithPasswordGrant / fetchLoggedInUser. authCommand.spec.ts mocks authLogin entirely, so the password-grant HTTP call, the Basic ${basicAuth} header construction, opaque-token handling, and fetchLoggedInUser's identity lookup are all untested. config.spec.ts tests the getAuthConfig branches well, but the actual SasjsRequestClient.post('/SASLogon/oauth/token', ...) path in auth.ts has zero coverage. Consider a spec that mocks SasjsRequestClient and asserts the grant_type/Authorization header and the missing-access-token error branch.

5. prompts used for password input but getString for username (login.ts:35-51). Minor inconsistency — getString echoes input and has no masking, which is correct for the username, but worth confirming the intent is intentional (it is: only the password needs masking).

Looks Good

• Token rotation persistence is handled correctly and consistently: getAuthConfig, persistTokensRefreshedByAdapter, and saveTokens all persist the rotated refresh token, preventing the stale single-use token problem on the next CLI invocation. The doc comments explaining why persistence is mandatory are excellent.
• The onTokensRefreshed: persistTokensRefreshedByAdapter(target) wiring across run.ts, executeCode.ts, and executeDeployScriptSasViya.ts is consistent.
• The 300s safety margin for isAccessTokenExpiring with a clear comment explaining the double-refresh bug is a solid fix.
• Moving isAccessTokenExpiring/isRefreshTokenExpiring to @sasjs/utils/auth and using jest.mock with jest.requireActual defaults is the right approach for the compiled-module spy problem.
• The saveTokens signature change from (targetName, client, secret, access_token, refresh_token) → (targetName, access_token, refresh_token, client?, secret?) is clean — all callers updated, and the conditional clientSecretContent correctly omits CLIENT=/SECRET= lines for password-grant targets.
• Removing the auth→add alias and registering auth as its own command with a login subcommand is the correct command-factory wiring; the legacy sasjs auth (no subcommand) behaviour is preserved via executeCred.

Reviewed by Hermes Agent (GitHub App)

[4gl-reviewer]

🔴 Build break — this is the CI failure. authResponse.refresh_token is string | undefined (see the cast on line 112: { access_token?: string; refresh_token?: string }), but the function's return type declares refresh_token: string. npx tsc -p . fails here, and the CI test job fails at the Build Project step.

Fix:

refresh_token: authResponse.refresh_token || ''

login.ts:61 already guards with refresh_token || '', so an empty-string sentinel is safe.

[4gl-reviewer]

Note for reviewers reading the diff: the *** here is a diff-rendering artifact; the source on disk reads Authorization: \Basic ${basicAuth}`(verified withod -c). The Basic-auth header for the sas.clipublic client (password grant, no secret) is correctly formed asbase64('sas.cli:')`.

[4gl-reviewer]

No .catch here — a 401/403 or network error from /identities/users/@currentUser will throw a raw adapter error to the user. Consider wrapping it like getTokensWithPasswordGrant does, e.g. Unable to verify the access token against ${target.serverUrl}: ${err?.message || err}.

[4gl-reviewer]

saveTokens(target.name, access_token, refresh_token || '') — good, the || '' guard handles the empty-string sentinel from getTokensWithPasswordGrant once the type fix lands.

[4gl-reviewer]

This no-client/sas.cli refresh branch is only in getAuthConfig, not in getAccessToken. That's fine for now since getAccessToken is only used by test support code (src/utils/test.ts), and its doc comment correctly warns against using it for user-facing commands. Flagging so the asymmetry is a conscious decision.

[4gl-reviewer]

Good addition. One thing to verify: persistTokensRefreshedByAdapter reads target.authConfig?.client || process.env.CLIENT. For a password-grant-only target both will be undefined, so saveTokens is called with client=undefined, secret=undefined and correctly omits the CLIENT=/SECRET= lines from .env.{target}. The test at config.spec.ts:431 ('should persist tokens refreshed internally by the adapter without dropping client/secret') covers the with-client path; consider adding the no-client path too.

[github-actions]

Coverage report

Total coverage

Status	Category	Percentage	Covered / Total
🟡	Statements	75.17% (+1.13% 🔼)	3449/4588
🟡	Branches	62.49% (+1.73% 🔼)	1396/2234
🟡	Functions	74.97% (+1.06% 🔼)	698/931
🟢	Lines	83.74% (+1.01% 🔼)	8775/10479

Status of coverage: 🟢 - ok, 🟡 - slightly more than threshold, 🔴 - under the threshold

Show new covered files 🌑

Coverage of new files

Status	Filename	Statements	Branches	Functions	Lines
🟢	src/commands/auth/login.ts	100%	100%	100%	100%
🟢	src/commands/auth/authCommand.ts	90.91%	66.67%	100%	100%

Status of coverage: 🟢 - ok, 🟡 - slightly more than threshold, 🔴 - under the threshold

Show files with reduced coverage 🔻

Reduced coverage

Status	Filename	Statements	Branches	Functions	Lines
🟢	src/commands/compile/compileSingleFile.ts	95.83% (-2.08% 🔻)	84% (-4% 🔻)	100%	100%
🟢	src/types/command/commandFactory.ts	10.71%	100%	3.85%	100%

Status of coverage: 🟢 - ok, 🟡 - slightly more than threshold, 🔴 - under the threshold

Report generated by 🧪jest coverage report action from 2354fb1

[4gl-reviewer]

Hermes Agent Code Review

Verdict: Comment (lean toward Approve — feature works and is well-tested; one edge-case regression worth addressing)

This PR adds sasjs auth login (ROPC password grant against the built-in sas.cli client) and hardens token handling for opaque/non-JWT refresh tokens and short-lived access tokens. The design is sound, the password is never persisted, and the new test coverage is genuinely good (parsing, dispatch, legacy alias, opaque-refresh via both the client/secret and sas.cli branches, and adapter-refresh persistence).

I verified locally: tsc --noEmit is clean, and authCommand.spec, config.spec (getAuthConfig + opaque suites), addCommand.spec, help.spec, run and fs specs all pass. The two @sasjs/utils@3.6.0 / @sasjs/adapter@4.18.0 companion deps are published, and the file: tarball + PLAN-*.md merge blockers called out in the PR body are already resolved at this HEAD.

Warnings

1. getAuthConfig early return drops secret for client/secret targets (src/utils/config.ts:646-653). When a fresh access token is present, the function returns client (the real registered client) but hardcodes secret: undefined. Pre-PR, secret was always populated because client/secret were required up-front. For a client/secret target whose token is fresh at command start but expires during a long-running job, the adapter's mid-execution refresh (@sasjs/adapter auth/getTokens.ts) does client || 'sas.cli' and secret || ''. Because client is truthy here, the sas.cli fallback is not taken, so the adapter refreshes with realClient + '' (empty secret) — which fails for confidential clients. The PR's own description notes cold-estate jobs can run "many minutes", so this path is plausible. Suggested fix: compute secret (with the same 'null'/'undefined' sanitization) before the early return and return it instead of undefined:

let secret = target?.authConfig?.secret ? target.authConfig.secret : process.env.SECRET
secret = secret && (secret.trim() === 'null' || secret.trim() === 'undefined') ? undefined : secret
// ... early return:
eturn { access_token, refresh_token: refresh_token || '', client, secret }

Suggestions

2. getTokensWithPasswordGrant error fallback (src/utils/auth.ts:114): ${err?.message || err} — if an axios error ever had a falsy .message, the || err branch would stringify the full error object, which can include config.data (the URL-encoded body containing the password). Axios errors always carry a .message, so this is low-risk, but defensively prefer err?.message ?? 'Unknown error' to avoid ever stringifying the raw error.

3. getAccessToken lacks the sas.js auth login hint and sas.cli fallback (src/utils/config.ts:900). It's documented as cleanup-only and the sole non-test caller is src/utils/test.ts, so this is fine today — but its Client ID was not found message is now inconsistent with getAuthConfig's. If a future user-facing command reuses getAccessToken, the UX gap resurfaces. Worth a one-line hint or a note in the doc comment.

4. Help output (src/commands/help/help.ts:371): auth now appears both in the commands list (with its description) and in the aliases list with an empty alias set (* auth : ). Minor cosmetic duplication — consider dropping the auth entry from the aliases array since it's no longer an alias.

Looks Good

• Password is never stored; only the token pair is persisted via saveTokens. Prompt input is masked (type: 'password').
• The sas.cli silent-refresh branch correctly re-persists the rotated pair (Viya single-use refresh tokens) and does not fabricate CLIENT=sas.cli values.
• saveTokens parameter reorder (now access_token, refresh_token, client?, secret?) — all 4 call sites updated consistently.
• persistTokensRefreshedByAdapter sanitizes 'null'/'undefined' string values consistently with getAuthConfig, and preserves configured client/secret while not adding CLIENT=/SECRET= lines for password-grant-only targets.
• New tests cover the important branches; the jest.mock('@sasjs/utils/auth', ...) shim with requireActual defaults is a clean way to handle the non-configurable getters.
• Companion packages are released and the tarball/PLAN-doc merge blockers are already cleared.

Reviewed by Hermes Agent (GitHub App)

[4gl-reviewer]

⚠️ Warning — secret dropped for client/secret targets. This early return sets secret: undefined even when the target has a configured client/secret. Pre-PR, getAuthConfig always returned a populated secret (it threw otherwise).

For a client/secret target with a fresh token at command start, the returned authConfig has client (truthy) but secret: undefined. If the token then expires mid-execution during a long-running job, the adapter's internal refresh (@sasjs/adapter getTokens) does client || 'sas.cli' + secret || ''. Because client is truthy, the sas.cli fallback is skipped and it refreshes with realClient + '' (empty secret) — which fails for confidential clients.

Suggested fix: compute secret (with the same 'null'/'undefined' sanitization used at lines 696-702) before this early return and return it here instead of undefined.

[4gl-reviewer]

💡 The || err fallback could stringify a full axios error object (which may include config.data — the URL-encoded request body containing the password) in the unlikely case .message is falsy. Axios errors always have a .message, so this is very low risk, but err?.message ?? 'Unknown error' would be defensive and avoid any chance of leaking the password into the error text.

[4gl-reviewer]

💡 auth is now a first-class command (it has its own entry in the commands array above with a description). Listing it again here in the aliases array with an empty alias set produces a slightly confusing * auth : line under "Alias commands:". Consider removing this entry since auth is no longer an alias of add.

[4gl-reviewer]

Hermes Agent Code Review

Verdict: Comment (lean toward Approve — the 6 previously-flagged items are addressed; one consistency issue remains)

This follow-up commit (0fd2eaa8) resolves the open review items: tarball deps replaced with published semver ranges, PLAN docs removed, fetchLoggedInUser hardened with error handling + null-id guard, authLogin now honours --insecure, and the fresh-token early return now passes through the configured secret. New unit tests cover all of these. All 16 new tests pass locally (authCommand.spec, auth.spec, getAuthConfig block in config.spec).

Warnings

• Inconsistent access-token expiry threshold on the early-return path (src/utils/config.ts:654): the early return uses !isAccessTokenExpiring(access_token) (default 3600 s margin), while the later refresh path uses isAccessTokenExpiring(access_token, 300) (300 s margin). The PR's own comment at lines 713–721 explains the 300 s margin is necessary because some Viya estates issue 1 h-TTL access tokens — with the 3600 s default a brand-new 1 h token is immediately considered "expiring". On the early-return path this means a fresh 1 h token minted by sasjs auth login (TTL ≈ 3600 ≤ 3600) is treated as expiring, so the early return is skipped and the token falls through to the sas.cli refresh branch — unnecessarily refreshing a brand-new token and burning a single-use refresh token. This contradicts the stated goal of avoiding double-refresh. Suggested fix: if (access_token && !isAccessTokenExpiring(access_token, 300)) {.

Suggestions

• Redundant empty aliases entry (src/types/command/commandAliases.ts:2): ['add', []] is an entry with an empty aliases array; every other entry has a non-empty array. Harmless, but could be removed since add is registered directly in commandFactory.ts.

Looks Good

• fetchLoggedInUser error handling and null-id guard with matching tests (auth.spec.ts).
• --insecure flag threaded through authCommand → authLogin, mirroring addCredential.
• secret now correctly returned on the fresh-token early return when client/secret are configured (with test).
• Deps now reference published @sasjs/adapter@^4.18.0 / @sasjs/utils@^3.6.0 — no file: tarballs remain.
• PLAN working documents are no longer in the diff.

Reviewed by Hermes Agent (GitHub App)

[4gl-reviewer]

Warning — inconsistent expiry threshold. This early return uses the default 3600 s margin, but the refresh path below (line 722) uses isAccessTokenExpiring(access_token, 300). For a Viya estate that issues 1 h-TTL access tokens (the exact scenario the comment at lines 713–721 describes), a brand-new token from sasjs auth login has TTL ≈ 3600, so isAccessTokenExpiring(access_token) (3600 default) returns true and this early return is skipped. The token then falls through to the sas.cli refresh branch, unnecessarily refreshing a fresh token and consuming a single-use refresh token.

Suggested fix: if (access_token && !isAccessTokenExpiring(access_token, 300)) {

[4gl-reviewer]

Suggestion. ['add', []] is an entry with an empty aliases array — every other entry has a non-empty array. Since add is registered directly in commandFactory.ts, this entry could be removed for consistency.

[sasjs-dev]

Addressed both remaining items from the latest review:

1. Inconsistent expiry threshold — Fixed in commit c21b51a. The early return now uses isAccessTokenExpiring(access_token, 300) to match the refresh path. This was pushed after the review on 0fd2eaa, so the warning was stale.

2. Empty aliases entry for add — Removed [add, []] from commandAliases.ts in commit d431d48. add is registered directly in commandFactory.ts and unalias("add") falls back to returning "add" when not in the alias map. Also removed the matching { name: "add", aliases: [] } from the hardcoded aliases list in help.ts.

Both CI workflows (Node.js CI + Windows mocked tests) are green on the latest HEAD (22ea37a).

[4gl-reviewer]

Warning — untested orchestration: authLogin itself has no unit tests. authCommand.spec.ts mocks it and only verifies it's called; auth.spec.ts tests getTokensWithPasswordGrant and fetchLoggedInUser in isolation. The orchestration logic here — non-Viya rejection (line 27), missing serverUrl guard (line 35), --insecure httpsAgentOptions mutation (line 42), and the saveTokens call with refresh_token || '' fallback (line 86) — is all untested. Consider adding a spec that mocks the imports (prompts, getTokensWithPasswordGrant, fetchLoggedInUser, saveTokens) and exercises: (a) non-Viya throws, (b) missing serverUrl throws, (c) insecure sets rejectUnauthorized: false, (d) successful login calls saveTokens with the right args, (e) cancelled prompt throws.

[4gl-reviewer]

Suggestion — inconsistent error hint: getAuthConfig now appends passwordGrantHint ("Alternatively, run 'sasjs auth login -t …'") to its Client ID / Secret not-found errors, but getAccessToken (lines 937, 950) does not. While getAccessToken is documented as for one-shot cleanup calls (test.ts), a password-grant user who triggers it will get an unhelpful "Client ID was not found" with no hint to re-login. Consider adding the same hint here for consistency, or at minimum referencing sasjs auth login.

[4gl-reviewer]

Suggestion — test gap for no-client + expired-refresh path: The no-client branch (line 672) handles !refresh_token || isRefreshTokenExpiring(refresh_token) by throwing with the hint. The test at line ~268 covers the no-refresh-token case (no refresh_token in authConfig), but there's no test for the case where a refresh token exists but isRefreshTokenExpiring returns true (e.g. mock isRefreshTokenExpiring to true with a token present). Worth adding to confirm the same error path is hit.

[4gl-reviewer]

Hermes Agent Code Review

Verdict: Comment — no blocking issues; the implementation is solid and well-documented. A few warnings and suggestions below.

---

🔴 Critical

None.

⚠️ Warnings

1. authLogin orchestration is untested (src/commands/auth/login.ts:23)
   The authLogin function itself has no unit tests. authCommand.spec.ts mocks it; auth.spec.ts tests getTokensWithPasswordGrant and fetchLoggedInUser in isolation. The orchestration — non-Viya rejection, missing serverUrl guard, --insecure httpsAgentOptions mutation, and saveTokens call — is all untested. See inline comment for suggested test cases.

2. Token refresh during long-running sasjs job execute isn't persisted (src/commands/job/internal/execute/viya.ts:102 — not in diff, pre-existing)
   startComputeJob with waitForResult: true polls the job, and the adapter refreshes tokens internally during polling. But startComputeJob doesn't accept onTokensRefreshed (unlike executeScript), so the rotated refresh token is lost when the process exits. This is a pre-existing gap, but it's now more impactful for password-grant users: they have no client/secret fallback, so a stale refresh token on the next CLI invocation means they must re-run sasjs auth login manually. Worth tracking as a follow-up.

💡 Suggestions

1. getAccessToken error messages lack the sasjs auth login hint (src/utils/config.ts:937, 950)
   getAuthConfig now appends a helpful passwordGrantHint to its Client ID / Secret errors, but getAccessToken does not. While getAccessToken is documented as for one-shot cleanup, a password-grant user who triggers it gets an unhelpful error with no hint to re-login. See inline comment.

2. Test gap: no-client + expired refresh token path (src/utils/config.ts:676)
   The no-client branch throws when !refresh_token || isRefreshTokenExpiring(refresh_token). Tests cover the no-refresh-token case but not the case where a refresh token exists but is expiring. See inline comment.

3. console.debug in @sasjs/utils/auth for opaque tokens
   The isTokenExpiring function in the dependency calls console.debug('isTokenExpiring: token is not a decodable JWT...') for every opaque token check. This will print to stdout in CI/CD pipelines and can't be silenced via process.logger. Consider routing through process.logger?.debug or removing the call in a future @sasjs/utils release.

4. getAccessToken still doesn't support the no-client/refresh path
   getAccessToken (line 902) requires client/secret for token refresh, unlike getAuthConfig which now has the sas.cli password-grant refresh path. This is documented as intentional (one-shot cleanup), but the asymmetry means any future caller of getAccessToken on a password-grant target will fail. The doc comment is good, but consider adding a @deprecated or @internal tag to discourage new usage.

✅ Looks Good

• Security: Password is prompted with prompts (masked password type), sent in the request body via URLSearchParams (not URL params), never stored, and never appears in error messages. The sas.cli public client uses Basic base64(sas.cli:) with empty secret — standard OAuth2 password grant for a public client. No credential logging or leakage found.
• Token rotation persistence: persistTokensRefreshedByAdapter is correctly wired into all three executeScript Viya call sites (run, deploy, fs/executeCode). The saveTokens parameter reordering (client/secret now optional) is clean, and the undefined-stripping via JSON.stringify for global config is well-documented.
• Opaque token handling: The @sasjs/utils/auth isTokenExpiring correctly catches InvalidTokenError and treats opaque tokens as usable, letting the server reject if expired. The 300s safety margin (vs. the old 3600s default) for isAccessTokenExpiring is well-reasoned and prevents the double-refresh issue on 1h-token Viya estates.
• Command structure: Clean separation of auth login (password grant) from legacy auth (alias of add cred). The commandAliases.ts / commandFactory.ts / commandBase.ts changes are minimal and correct.
• Test coverage: 39 config tests + 14 auth/command tests, all passing. Good coverage of the no-client refresh path, opaque token handling, and persistTokensRefreshedByAdapter with/without client/secret.

---

Reviewed by Hermes Agent (GitHub App)

[4gl-reviewer]

Hermes Agent Code Review

Verdict: Comment — the 3 new commits since the last review (22ea37a) are solid refinements with no blocking issues. The refresh_token: authResponse.refresh_token as string → refresh_token || '' change is a genuine correctness fix (the old cast would have returned undefined-as-string when a Viya deployment omits the refresh token). The 300s threshold is now consistent across the early-return and refresh paths. The try/catch refactor in authCommand.ts and removal of the empty ['add', []] alias entry are clean.

I re-ran the new test suites locally:

• src/commands/auth/spec/authCommand.spec.ts — 6/6 pass
• src/utils/spec/auth.spec.ts — 8/8 pass (incl. the new getTokensWithPasswordGrant suite covering Basic-auth header, grant_type, 401 handling, CertificateError passthrough, missing-access-token, and the refresh_token || '' default)
• src/utils/spec/config.spec.ts -t getAuthConfig — 7/7 pass

(One unrelated failure in saveToLocalConfig from an adm-zip/createTestMinimalApp environmental issue — not introduced by this PR.)

🔴 Critical

None.

⚠️ Warnings (carried from prior review, still applicable)

1. authLogin orchestration remains untested (src/commands/auth/login.ts:23). The new commits added good coverage for getTokensWithPasswordGrant and fetchLoggedInUser, but authLogin itself — the non-Viya rejection, the missing-serverUrl guard, the --insecure httpsAgentOptions mutation, and the saveTokens call — is still only exercised via mocks in authCommand.spec.ts. Adding a spec that mocks the three dependencies and drives the guard branches would close the last meaningful gap.

2. Long-running sasjs job execute doesn't persist rotated refresh tokens (pre-existing, not in diff). startComputeJob doesn't accept onTokensRefreshed unlike executeScript (now wired in run.ts, executeCode.ts, executeDeployScriptSasViya.ts). For password-grant users with no client/secret fallback, a stale refresh token forces a manual re-login. Worth a follow-up issue.

💡 Suggestions

1. getAccessToken error messages still lack the sasjs auth login hint (src/utils/config.ts:938, 951). getAuthConfig appends passwordGrantHint to its Client ID/Secret errors; getAccessToken does not. It's documented as one-shot/test-only, but the asymmetry is now more user-visible. Adding the hint (or a @deprecated/@internal tag) would be a small consistency win.

2. @sasjs/utils/auth console.debug for opaque tokens (dependency). isTokenExpiring prints console.debug('isTokenExpiring: token is not a decodable JWT...') for every opaque refresh-token check, which leaks to stdout in CI and can't be silenced via process.logger. Consider routing through process.logger?.debug in a future @sasjs/utils release.

✅ Looks Good

• refresh_token || '' default correctly normalizes the omitted-refresh-token case so downstream saveTokens/config code treats it as a plain string.
• sanitizeEnvValue extraction is a clean DRY of the repeated trim() === 'null' | 'undefined' checks across getAuthConfig, getAccessToken, and persistTokensRefreshedByAdapter.
• Removing the empty ['add', []] alias entry (and the matching help entry) is correct now that auth is a first-class command — no dead config left behind.
• try/catch in executeLogin/executeCred reads more clearly than the prior .then()/.catch() chains.

Reviewed by Hermes Agent (GitHub App)

[4gl-reviewer]

⚠️ authLogin itself has no direct unit tests — authCommand.spec.ts mocks it and auth.spec.ts tests getTokensWithPasswordGrant/fetchLoggedInUser in isolation. The guard branches here (non-Viya rejection at line 27, missing serverUrl at line 34, --insecure httpsAgentOptions mutation at line 40, and the saveTokens call at line 87) are worth a focused spec that mocks the three dependencies and drives each branch.

[4gl-reviewer]

💡 getAuthConfig now appends passwordGrantHint to its Client ID / Secret errors (lines 704–715), but getAccessToken (this block, throwing at line 938/951) does not. A password-grant user who hits this path (e.g. via test support code) gets an unhelpful error with no hint to re-run sasjs auth login. Consider appending the same hint for consistency, or adding a @internal/@deprecated JSDoc tag to discourage new callers.

[4gl-reviewer]

Minor UX edge case: if a caller passes a target without a name set, the hint renders as sasjs auth login -t undefined. target itself is guaranteed truthy here (already dereferenced at line 904 and passed to getSASjs(target) at line 929), so the target?. chain could be target. — but guarding target.name against undefined would be the more useful fix, e.g. sasjs auth login -t ${target.name || '<target>'}'. Low priority since getAccessToken is documented one-shot/test-only and user-facing commands go through getAuthConfig.

[4gl-reviewer]

Hermes Agent Code Review

Verdict: 💬 COMMENT (reviewing incremental commits since last review @ 22ea37a)

This review covers the two new commits since the prior review pass: c024fc3 (password-grant hint in getAccessToken error messages) and 2354fb1 (new authLogin orchestration unit tests).

Looks Good

• src/commands/auth/spec/login.spec.ts — 12 well-structured unit tests covering every guard branch of authLogin: non-Viya rejection, missing serverUrl, --insecure httpsAgentOptions mutation (both true/false), saveTokens arg contract, refresh_token default-to-empty fallback, and error propagation from both fetchLoggedInUser and getTokensWithPasswordGrant. The username validator and password-prompt onCancel callback are also exercised. Mocking strategy (barrel-level jest.mock('../../../utils') so spies match the references login.ts resolves at runtime) is sound. All 12 tests pass locally.
• src/utils/config.ts — The passwordGrantHint is correctly scoped to the two "client/secret not found" error paths in getAccessToken (the token-refresh flow where a missing client/secret is exactly the scenario the password grant sidesteps). It is not added to the unrelated "no access token" path, which is correct.
• src/utils/spec/config.spec.ts — Both error-branch assertions updated to verify the new hint text (/sasjs auth login -t viya/). The target.name: 'viya' additions make the regex assertions meaningful. 10 tests pass locally.

Suggestions (non-blocking)

• config.ts:931 — target?.name can render as ... -t undefined for a nameless target. See inline comment.
• login.spec.ts:137 — the negative assertion expect(callTarget.httpsAgentOptions?.rejectUnauthorized).not.toBe(false) passes for any value that isn't false (including undefined and true). It does correctly guard against the insecure flag leaking when insecure=false, so this is acceptable; tightening to .toBeUndefined() would make the intent explicit.

No security, correctness, or performance concerns in the incremental changes. The broader PR's core auth implementation was covered by prior reviews.

Reviewed by Hermes Agent (GitHub App)

all issues addressed