Skip to content

feat(rest-api): Support configurable OAuth client credentials - #4792

Open
kfelternv wants to merge 4 commits into
NVIDIA:mainfrom
kfelternv:feat-nicocli-oauth-config
Open

feat(rest-api): Support configurable OAuth client credentials#4792
kfelternv wants to merge 4 commits into
NVIDIA:mainfrom
kfelternv:feat-nicocli-oauth-config

Conversation

@kfelternv

Copy link
Copy Markdown
Contributor

nicocli client-credentials login uses a fixed OAuth request, which prevents environments from supplying required scopes, endpoint parameters, or HTTP Basic client authentication. This adds those configuration fields while preserving the existing openid scope and form-posted credentials by default.

Related issues

Resolves #4686

Type of Change

  • Add - New feature or capability
  • Change - Changes in existing functionality
  • Fix - Bug fixes
  • Remove - Removed features or deprecated functionality
  • Internal - Internal changes (refactoring, tests, docs, etc.)

Breaking Changes

  • This PR contains breaking changes

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed
  • No testing required (docs, internal refactor, etc.)

Additional Notes

The supported client authentication methods are client_secret_post and client_secret_basic. Reserved OAuth fields and credential parameters cannot be set through token_parameters.

@copy-pr-bot

copy-pr-bot Bot commented Aug 10, 2026

Copy link
Copy Markdown

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 17a4d3a6-4624-4809-bacd-424d860d4f47

📥 Commits

Reviewing files that changed from the base of the PR and between 070d55c and 12f2ee6.

📒 Files selected for processing (1)
  • rest-api/cli/tui/commands_test.go

Summary by CodeRabbit

  • New Features

    • Added configurable OIDC scopes and token parameters for client-credentials authentication.
    • Added support for client_secret_basic and client_secret_post authentication.
    • Added validation for reserved OAuth token parameters.
  • Bug Fixes

    • Improved error handling for invalid API responses, configurations, and numeric command inputs.
    • Improved handling of configured and explicitly provided client credentials.
  • Documentation

    • Updated configuration examples and environment-variable guidance for the new OIDC options.
  • Tests

    • Added coverage for authentication, validation, persistence, and configuration scenarios.

Walkthrough

The CLI adds configurable OIDC client-credentials scopes, token parameters, and client authentication methods. It also reports malformed responses, validates numeric input, and improves terminal and REPL cleanup behavior.

Changes

CLI authentication and validation

Layer / File(s) Summary
OIDC configuration contract
rest-api/cli/pkg/config.go, rest-api/cli/pkg/config_test.go, rest-api/cli/README.md
ConfigOIDC stores client authentication method, scopes, and token parameters. Save and reload operations validate parsing and preserve these fields.
Configurable token request flow
rest-api/cli/pkg/auth.go, rest-api/cli/pkg/auth_test.go, rest-api/cli/README.md
Client-credentials requests apply configured scopes and parameters, support form and Basic authentication, reject reserved overrides, and preserve default behavior.
Strict command input and response handling
rest-api/cli/tui/commands.go, rest-api/cli/tui/commands_test.go
Interactive commands return malformed JSON errors, reject non-integer prefix lengths, and retain label argument validation.
CLI state and cleanup behavior
rest-api/cli/pkg/site_bootstrap.go, rest-api/cli/tui/generated_commands.go, rest-api/cli/tui/repl.go, rest-api/cli/tui/term.go
REPL history state is removed. Scope capitalization avoids deprecated APIs. Terminal restoration failures are reported. Equivalent quoting and interpolation behavior remains.
Review and test guidance
rest-api/AGENTS.md
Review guidance covers lint cleanup, configuration override tests, protocol boundary tests, and table-driven tests.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Mergeability Score: ⚪ Minimal · up to 12f2e

The PR adds configurable OAuth client-credential options while preserving existing defaults, and no actionable merge-blocking risk remains after normal checks and review.

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant ConfigOIDC
  participant TokenEndpoint
  CLI->>ConfigOIDC: Load scopes and token parameters
  CLI->>TokenEndpoint: Send form or Basic-auth token request
  TokenEndpoint-->>CLI: Return token response
  CLI->>CLI: Persist access token
Loading
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning Several TUI, bootstrap, terminal, agent-guidance, and unrelated error-handling changes are outside the OAuth client-credentials objective. Remove the unrelated TUI, bootstrap, terminal, and agent-guidance changes, or move them into separate pull requests.
Docstring Coverage ⚠️ Warning Docstring coverage is 9.38% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: configurable OAuth client credentials.
Description check ✅ Passed The description directly explains configurable OAuth client-credentials requests and the related compatibility and testing work.
Linked Issues check ✅ Passed The changes satisfy the linked issue requirements for scopes, token parameters, authentication methods, compatibility, tests, and documentation [#4686].
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@thossain-nv thossain-nv added the rest-api Add this label when an issue or PR concerns NICo REST API label Aug 10, 2026 — with ChatGPT Codex Connector
@kfelternv

kfelternv commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Verification outdated and removed; see updated verification.

@kfelternv
kfelternv marked this pull request as ready for review August 13, 2026 03:10
@kfelternv
kfelternv requested a review from a team as a code owner August 13, 2026 03:10
@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown

🔐 TruffleHog Secret Scan

No secrets or credentials found!

Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉

🔗 View scan details

🕐 Last updated: 2026-08-13 03:27:03 UTC | Commit: fec5b5e

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@rest-api/cli/pkg/auth.go`:
- Around line 419-421: Update loginWithOIDCCmd’s client-credentials grant
configuration to preserve Scopes, TokenParameters, and ClientAuthMethod from
cfg.Auth.OIDC, while still overriding TokenURL, ClientID, and ClientSecret with
the CLI-provided values before calling clientCredentialsGrant.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 74fbed22-49ab-4579-b978-3888cc925ed6

📥 Commits

Reviewing files that changed from the base of the PR and between 7d3ffbd and 63ff0f5.

📒 Files selected for processing (5)
  • rest-api/cli/README.md
  • rest-api/cli/pkg/auth.go
  • rest-api/cli/pkg/auth_test.go
  • rest-api/cli/pkg/config.go
  • rest-api/cli/pkg/config_test.go

Comment thread rest-api/cli/pkg/auth.go Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 63ff0f5a98

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread rest-api/cli/pkg/auth.go Outdated
Comment thread rest-api/cli/pkg/auth.go Outdated
Comment thread rest-api/cli/pkg/auth_test.go Outdated
@kfelternv

kfelternv commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Verification outdated and removed; see updated verification.

@kfelternv

Copy link
Copy Markdown
Contributor Author

What changed

This PR lets nicocli client-credentials login configure OAuth scopes, extra token parameters, and HTTP Basic client authentication. I exercised the custom request, the unchanged default request, invalid configuration rejection, explicit CLI overrides, and token persistence.

Scenario and setup

I tested commit 12f2ee62c56ca448a952a855a9f59fca140b4fb9. A local HTTP token endpoint recorded each form and Authorization header, then returned {"access_token":"verified-token","expires_in":3600}. The focused CLI tests, go vet ./cli/..., strict golangci-lint for ./cli/..., and CLI revive checks passed. NICo REST CI run 31666386651 passed, including the TruffleHog scan, Go lint, tests, builds, generated-file checks, OpenAPI checks, Helm validation, and image builds.

Verification

Step 1: Custom scopes, token parameter, and HTTP Basic authentication

Why this step exists: This tests the new configuration through the real nicocli login command and verifies that the returned token is persisted.

Runnable command:

CONFIG_FILE=oauth-basic.yaml
NICO_CONFIG="$CONFIG_FILE" go run ./rest-api/cli/cmd/cli login

The configuration used scopes: [carbide, offline_access], client_auth_method: client_secret_basic, and token_parameters: {audience: nico}.

Observed result:

Login successful. Token saved to oauth-basic.yaml
request form: audience=nico, grant_type=client_credentials, scope="carbide offline_access"
HTTP Basic credentials: client-id:client-secret
saved token: verified-token
saved expiry: set

Why this proves the behavior: The CLI sent the configured scopes and audience, moved the client credentials to HTTP Basic authentication, and saved the response.

Step 2: Default client-credentials request

Why this step exists: This verifies that configurations without the new fields keep the existing request contract.

Runnable command:

CONFIG_FILE=oauth-default.yaml
NICO_CONFIG="$CONFIG_FILE" go run ./rest-api/cli/cmd/cli login

Observed result:

Login successful. Token saved to oauth-default.yaml
request form: client_id=default-id, client_secret=default-secret, grant_type=client_credentials, scope=openid
Authorization header: absent
saved token: verified-token
saved expiry: set

Why this proves the behavior: The CLI preserved the openid scope and form-posted client credentials when the new fields were omitted.

Step 3: Reserved token parameter rejection

Why this step exists: This verifies that extra parameters cannot replace OAuth credential or grant fields.

Runnable command:

CONFIG_FILE=oauth-reserved.yaml
NICO_CONFIG="$CONFIG_FILE" go run ./rest-api/cli/cmd/cli login

The configuration used token_parameters: {client_secret: replacement}.

Observed result:

Error: OIDC login failed: reserved token parameter "client_secret" cannot be configured
exit status: 1
HTTP requests received: 0

Why this proves the behavior: The CLI rejected the reserved override before contacting the token endpoint.

Step 4: Unsupported client authentication method rejection

Why this step exists: This verifies that the configuration accepts only the documented authentication methods.

Runnable command:

CONFIG_FILE=oauth-unsupported.yaml
NICO_CONFIG="$CONFIG_FILE" go run ./rest-api/cli/cmd/cli login

The configuration used client_auth_method: private_key_jwt.

Observed result:

Error: OIDC login failed: unsupported client_auth_method "private_key_jwt"
exit status: 1
HTTP requests received: 0

Why this proves the behavior: The CLI rejected an unsupported authentication method before contacting the token endpoint.

Step 5: Explicit client ID override with configured options

Why this step exists: This verifies that an explicit CLI client ID overrides the configured ID without dropping configured scopes, token parameters, or authentication mode.

Runnable command:

NICO_CONFIG=oauth-basic.yaml ./nicocli login --client-id 'override:id'

The configuration used scope: carbide, audience: nico, client_auth_method: client_secret_basic, and a client secret containing + and % characters.

Observed result:

Login successful. Token saved to oauth-basic.yaml
request form: audience=nico, grant_type=client_credentials, scope=carbide
HTTP Basic payload: override%3Aid:client%2Bsecret%25
saved token: verified-token
saved expiry: set

Why this proves the behavior: The CLI applied the explicit client ID, preserved the configured request options, encoded HTTP Basic credentials per OAuth requirements, and saved the token.

Step 6: Strict CLI static analysis

Why this step exists: The repository lint wrapper reports analyzer findings with a zero issues exit code, so its success alone does not prove that changed packages are clean.

Runnable command:

go tool golangci-lint run ./cli/... --issues-exit-code 1 --output.text.path=stdout

Observed result:

exit status: 0
analyzer findings: 0

Why this proves the behavior: errcheck, govet, ineffassign, staticcheck, unused, and the configured analyzers returned no CLI findings when findings were configured to fail the command.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rest-api Add this label when an issue or PR concerns NICo REST API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(nicocli): support configurable OAuth client-credentials requests

2 participants