Add support for a customizable MCP service account auth provider#1176
Add support for a customizable MCP service account auth provider#1176rapids-bot[bot] merged 41 commits intoNVIDIA:developfrom
Conversation
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
AnuradhaKaruppiah
added
feature request
WalkthroughAdds an MCP service-account auth provider (OAuth2 client‑credentials) with token caching and optional secondary header, registers it, updates header aggregation and per‑group default‑user routing, introduces docs/examples, and adds comprehensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Provider as MCPServiceAccountProvider
participant TokenClient as ServiceAccountTokenClient
participant OAuth2 as OAuth2 Server
participant Func as Service Token Fn (optional)
Client->>Provider: authenticate(user_id)
Provider->>TokenClient: get_access_token()
alt cached token valid
TokenClient-->>Provider: access_token (cached)
else fetch new token
TokenClient->>OAuth2: POST token_url (Basic Auth, form)
OAuth2-->>TokenClient: access_token, expires_in
TokenClient-->>Provider: access_token (fresh)
end
Provider->>Provider: build HeaderCred (Authorization: Bearer ...)
alt service_token configured
Provider->>Func: load & call (if dynamic)
Func-->>Provider: token or (header, token)
Provider->>Provider: append HeaderCred (custom header)
end
Provider-->>Client: AuthResult(credentials, token_expires_at)
sequenceDiagram
participant MCPClient
participant Group as MCPFunctionGroup
participant AuthProv as AuthProvider
participant Base as BaseClient
participant Session as Per-Session Client
MCPClient->>Group: invoke tool_call
Group->>Group: determine session_id (use _default_user_id when allowed)
Group->>AuthProv: authenticate / get credentials
rect rgba(220,240,255,0.8)
note over Group: aggregate credentials -> build headers
Group->>Group: iterate credentials: BearerTokenCred -> Authorization
Group->>Group: iterate credentials: HeaderCred -> custom header(s)
end
alt session_id == _default_user_id
Group->>Base: route via base client
else
Group->>Session: route via per-session client
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
|
@CodeRabbit review full |
|
@AnuradhaKaruppiah: I'll perform a full review of the PR changes. 🧠 Learnings used✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py (1)
115-129: Multi-credential header support implemented correctly.The updated logic properly iterates through all credentials and aggregates headers, supporting both standard Bearer tokens and custom header credentials. This enables the dual-header authentication pattern for service accounts.
The static analysis hint suggests moving the
return headersstatement (line 129) to anelseblock for clarity:for cred in auth_result.credentials: if isinstance(cred, BearerTokenCred): # Standard Bearer token token = cred.token.get_secret_value() headers["Authorization"] = f"Bearer {token}" elif isinstance(cred, HeaderCred): # Generic header credential (supports custom formats and service accounts) headers[cred.name] = cred.value.get_secret_value() - return headers + else: + return headers except Exception as e: logger.warning("Failed to get auth token: %s", e) return {}However, the current structure is acceptable as the
forloop will complete normally, and the return statement is straightforward.packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py (1)
67-99: Accessing private member violates encapsulation.Line 97 accesses
self._token_client._token_expires_at, which is a private member. Consider exposing this through a public property onServiceAccountTokenClientfor better encapsulation.In
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py, add:@property def token_expires_at(self) -> datetime | None: """Get the token expiration time.""" return self._token_expires_atThen update line 97:
return AuthResult( credentials=credentials, - token_expires_at=self._token_client._token_expires_at, + token_expires_at=self._token_client.token_expires_at, raw={}, )Note: The unused parameters (
builder,user_id,kwargs) are likely required by theAuthProviderBaseinterface and can be safely ignored.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
docs/source/workflows/mcp/index.md(2 hunks)docs/source/workflows/mcp/mcp-auth.md(1 hunks)docs/source/workflows/mcp/mcp-service-account-auth.md(1 hunks)examples/MCP/service_account_auth_mcp/README.md(1 hunks)examples/MCP/service_account_auth_mcp/configs/config-mcp-service-account-jira.yml(1 hunks)examples/MCP/service_account_auth_mcp/pyproject.toml(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/register.py(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/__init__.py(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py(6 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
**/*.py: In code comments use the abbreviations: nat (API namespace/CLI), nvidia-nat (package), NAT (env var prefixes); never use these abbreviations in documentation
Follow PEP 20 and PEP 8 for Python style
Run yapf with column_limit=120; yapf is used for formatting (run second)
Indent with 4 spaces (no tabs) and end each file with a single trailing newline
Use ruff (ruff check --fix) as a linter (not formatter) per pyproject.toml; fix warnings unless explicitly ignored
Respect Python naming schemes: snake_case for functions/variables, PascalCase for classes, UPPER_CASE for constants
When re-raising exceptions, use bare raise to preserve stack trace; log with logger.error(), not logger.exception()
When catching and logging without re-raising, use logger.exception() to capture full stack trace
Provide Google-style docstrings for every public module, class, function, and CLI command
Docstring first line must be a concise description ending with a period
Validate and sanitize all user input, especially in web or CLI interfaces
Prefer httpx with SSL verification enabled by default and follow OWASP Top-10 recommendations
Use async/await for I/O-bound work (HTTP, DB, file I/O)
Cache expensive computations with functools.lru_cache or an external cache when appropriate
Leverage NumPy vectorized operations when beneficial and feasible
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/__init__.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/register.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
{src/**/*.py,packages/*/src/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{src/**/*.py,packages/*/src/**/*.py}: All importable Python code must live under src/ or packages//src/
All public APIs must have Python 3.11+ type hints on parameters and return values
Prefer typing/collections.abc abstractions (e.g., Sequence over list)
Use typing.Annotated for units or metadata when useful
Treat pyright warnings as errors during development
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/__init__.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/register.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
packages/*/src/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
If a package contains Python code, it must have tests in a tests/ directory at the same level as pyproject.toml
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/__init__.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/register.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
{**/*.py,**/*.sh,**/*.md,**/*.toml,**/*.y?(a)ml,**/*.json,**/*.txt,**/*.ini,**/*.cfg,**/*.ipynb}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{**/*.py,**/*.sh,**/*.md,**/*.toml,**/*.y?(a)ml,**/*.json,**/*.txt,**/*.ini,**/*.cfg,**/*.ipynb}: Every file must start with the standard SPDX Apache-2.0 header
Confirm copyright years are up to date when a file is changed
All source files must include the SPDX Apache-2.0 header template (copy from an existing file)
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/__init__.pydocs/source/workflows/mcp/mcp-auth.mddocs/source/workflows/mcp/mcp-service-account-auth.mdexamples/MCP/service_account_auth_mcp/README.mdpackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/register.pydocs/source/workflows/mcp/index.mdpackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.pyexamples/MCP/service_account_auth_mcp/pyproject.tomlpackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.pyexamples/MCP/service_account_auth_mcp/configs/config-mcp-service-account-jira.ymlpackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/__init__.pydocs/source/workflows/mcp/mcp-auth.mddocs/source/workflows/mcp/mcp-service-account-auth.mdexamples/MCP/service_account_auth_mcp/README.mdpackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/register.pydocs/source/workflows/mcp/index.mdpackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.pyexamples/MCP/service_account_auth_mcp/pyproject.tomlpackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.pyexamples/MCP/service_account_auth_mcp/configs/config-mcp-service-account-jira.ymlpackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/__init__.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/register.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
{docs/source/**/*.md,**/README.@(md|ipynb)}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
{docs/source/**/*.md,**/README.@(md|ipynb)}: Use the full name “NVIDIA NeMo Agent toolkit” on first use in documentation, then “NeMo Agent toolkit”; in headings use “NeMo Agent Toolkit” (capital T)
Do not use deprecated names (Agent Intelligence toolkit, aiqtoolkit, AgentIQ, AIQ/aiq) in documentation unless explicitly referring to deprecated names
Never use “NAT”/“nat” abbreviations in documentation
Documentation must be clear/comprehensive; avoid TODOs/FIXMEs/placeholders; avoid offensive/outdated terms; ensure spelling is correct
Files:
docs/source/workflows/mcp/mcp-auth.mddocs/source/workflows/mcp/mcp-service-account-auth.mdexamples/MCP/service_account_auth_mcp/README.mddocs/source/workflows/mcp/index.md
docs/source/**/*.md
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
docs/source/**/*.md: Documentation sources are Markdown files under docs/source
Surround code entities with backticks in docs to avoid Vale false positives
Do not use words listed in ci/vale/styles/config/vocabularies/nat/reject.txt; accepted words in accept.txt are allowed
Files:
docs/source/workflows/mcp/mcp-auth.mddocs/source/workflows/mcp/mcp-service-account-auth.mddocs/source/workflows/mcp/index.md
docs/source/**/*
⚙️ CodeRabbit configuration file
This directory contains the source code for the documentation. All documentation should be written in Markdown format. Any image files should be placed in the
docs/source/_staticdirectory.
Files:
docs/source/workflows/mcp/mcp-auth.mddocs/source/workflows/mcp/mcp-service-account-auth.mddocs/source/workflows/mcp/index.md
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: - This directory contains example code and usage scenarios for the toolkit, at a minimum an example should
contain a README.md or file README.ipynb.
- If an example contains Python code, it should be placed in a subdirectory named
src/and should
contain apyproject.tomlfile. Optionally, it might also contain scripts in ascripts/directory.- If an example contains YAML files, they should be placed in a subdirectory named
configs/. - If an example contains sample data files, they should be placed in a subdirectory nameddata/, and should
be checked into git-lfs.
Files:
examples/MCP/service_account_auth_mcp/README.mdexamples/MCP/service_account_auth_mcp/pyproject.tomlexamples/MCP/service_account_auth_mcp/configs/config-mcp-service-account-jira.yml
{**/pyproject.toml,uv.lock}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
New dependencies must be added to both pyproject.toml (alphabetically) and uv.lock via ‘uv pip install --sync’
Files:
examples/MCP/service_account_auth_mcp/pyproject.toml
**/configs/**
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Configuration files consumed by code should be stored next to that code in a configs/ folder
Files:
examples/MCP/service_account_auth_mcp/configs/config-mcp-service-account-jira.yml
🧠 Learnings (7)
📚 Learning: 2025-09-17T05:20:03.228Z
Learnt from: AnuradhaKaruppiah
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 752
File: packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py:99-121
Timestamp: 2025-09-17T05:20:03.228Z
Learning: The AuthAdapter class in packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py has a known concurrency race condition where `self.auth_provider.config.auth_request = auth_request` can be racy under concurrent requests. This should be addressed by either refactoring AuthProviderBase.authenticate() to accept AuthRequest as a parameter or using an asyncio.Lock to serialize access.
Applied to files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/register.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
📚 Learning: 2025-09-17T05:34:04.696Z
Learnt from: AnuradhaKaruppiah
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 752
File: packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py:0-0
Timestamp: 2025-09-17T05:34:04.696Z
Learning: The user AnuradhaKaruppiah confirmed that request copying is not needed in the AuthAdapter.async_auth_flow method in packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py, suggesting that httpx handles request mutation safely or there are no concurrent usage concerns for this implementation.
Applied to files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
📚 Learning: 2025-10-22T22:02:12.903Z
Learnt from: CR
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-10-22T22:02:12.903Z
Learning: Applies to examples/*/pyproject.toml : Examples containing Python code should have a pyproject.toml
Applied to files:
examples/MCP/service_account_auth_mcp/pyproject.toml
📚 Learning: 2025-10-22T22:02:12.903Z
Learnt from: CR
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-10-22T22:02:12.903Z
Learning: Applies to packages/*/pyproject.toml : packages/*/pyproject.toml must declare a dependency on nvidia-nat or a package starting with nvidia-nat-
Applied to files:
examples/MCP/service_account_auth_mcp/pyproject.toml
📚 Learning: 2025-10-22T22:02:12.903Z
Learnt from: CR
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-10-22T22:02:12.903Z
Learning: Applies to packages/*/pyproject.toml : Each package under packages/ must contain a pyproject.toml
Applied to files:
examples/MCP/service_account_auth_mcp/pyproject.toml
📚 Learning: 2025-11-05T11:45:35.119Z
Learnt from: thepatrickchin
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 1152
File: examples/config_inheritance/pyproject.toml:1-25
Timestamp: 2025-11-05T11:45:35.119Z
Learning: In the examples/ directory, pyproject.toml files typically do not include SPDX license headers, with only one exception (adk_demo). This is an established pattern that differs from the general guideline requiring SPDX headers in all .toml files.
Applied to files:
examples/MCP/service_account_auth_mcp/pyproject.toml
📚 Learning: 2025-11-10T21:26:35.059Z
Learnt from: jiaxiangr
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 974
File: packages/nvidia_nat_all/pyproject.toml:39-39
Timestamp: 2025-11-10T21:26:35.059Z
Learning: In packages/nvidia_nat_all/pyproject.toml, workspace dependencies (nvidia-nat-* plugin packages) should NOT have version constraints because they are managed as workspace dependencies. Version constraints are only applied to the base nvidia-nat package and external dependencies, not to internal workspace packages.
Applied to files:
examples/MCP/service_account_auth_mcp/pyproject.toml
🪛 markdownlint-cli2 (0.18.1)
docs/source/workflows/mcp/mcp-service-account-auth.md
158-158: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
178-178: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
200-200: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
253-253: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
257-257: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
261-261: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
267-267: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Ruff (0.14.4)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py
137-137: Avoid specifying long messages outside the exception class
(TRY003)
139-139: Avoid specifying long messages outside the exception class
(TRY003)
141-142: Avoid specifying long messages outside the exception class
(TRY003)
145-145: Avoid specifying long messages outside the exception class
(TRY003)
147-147: Avoid specifying long messages outside the exception class
(TRY003)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py
49-49: Unused method argument: builder
(ARG002)
67-67: Unused method argument: user_id
(ARG002)
67-67: Unused method argument: kwargs
(ARG002)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py
129-129: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (12)
docs/source/workflows/mcp/index.md (1)
23-24: LGTM!The documentation updates clearly describe the new authentication capabilities, properly distinguishing between interactive OAuth2 and headless service account authentication.
docs/source/workflows/mcp/mcp-auth.md (1)
19-23: LGTM!The updated introduction clearly differentiates between interactive OAuth2 authentication and headless service account authentication, improving user guidance for choosing the appropriate authentication method.
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py (1)
49-59: LGTM!The token client initialization is correct, properly extracting the secret value and forwarding all necessary configuration parameters.
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py (1)
22-70: LGTM!The configuration model is well-designed with:
- Proper use of
SecretStrfor sensitive credentials- Clear field descriptions for all parameters
- Sensible defaults for optional fields
- Comprehensive docstring explaining use cases
The configuration supports both simple and complex authentication patterns (single-header and dual-header).
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/register.py (2)
20-21: LGTM!Imports are clean and correctly reference the new service account provider components.
30-33: LGTM!The service account provider registration follows the established pattern used by the OAuth2 provider, ensuring consistency in the authentication provider system.
packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_impl.py (6)
112-114: LGTM!Adding per-group default user ID and permission flag enables better isolation and control for different authentication providers without modifying the base authentication framework.
183-184: LGTM!The session ID logic now correctly references the per-group fields instead of the auth provider config, enabling proper isolation for different authentication providers.
272-272: LGTM!The base client routing check now uses the local
_default_user_idfield, consistent with the refactored session management approach.
440-440: LGTM!The default user check in the response function now uses the local field, maintaining consistency with the refactored session management.
511-513: Safe fallback logic for different auth provider types.The
getattrwith fallback toserver_urlhandles both OAuth2 providers (which havedefault_user_id) and service account providers (which may not). This enables proper base client isolation for different authentication patterns.
535-545: LGTM!The conditional logic properly initializes per-group defaults from the auth provider config when available, with sensible fallbacks when no auth provider is configured. This supports both OAuth2 and service account authentication patterns.
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/source/workflows/mcp/mcp-service-account-auth.md(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.pydocs/source/workflows/mcp/mcp-service-account-auth.md
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py
docs/source/**/*
⚙️ CodeRabbit configuration file
This directory contains the source code for the documentation. All documentation should be written in Markdown format. Any image files should be placed in the
docs/source/_staticdirectory.
Files:
docs/source/workflows/mcp/mcp-service-account-auth.md
🧠 Learnings (1)
📚 Learning: 2025-09-17T05:20:03.228Z
Learnt from: AnuradhaKaruppiah
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 752
File: packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py:99-121
Timestamp: 2025-09-17T05:20:03.228Z
Learning: The AuthAdapter class in packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py has a known concurrency race condition where `self.auth_provider.config.auth_request = auth_request` can be racy under concurrent requests. This should be addressed by either refactoring AuthProviderBase.authenticate() to accept AuthRequest as a parameter or using an asyncio.Lock to serialize access.
Applied to files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py
🧬 Code graph analysis (2)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py (4)
src/nat/authentication/interfaces.py (1)
AuthProviderBase(29-72)src/nat/data_models/authentication.py (2)
AuthResult(194-245)HeaderCred(122-128)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py (1)
MCPServiceAccountProviderConfig(24-84)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py (2)
ServiceAccountTokenClient(27-150)get_access_token(78-104)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py (1)
src/nat/data_models/authentication.py (1)
AuthProviderBaseConfig(31-37)
🪛 Ruff (0.14.4)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py
51-51: Unused method argument: builder
(ARG002)
69-69: Unused method argument: user_id
(ARG002)
69-69: Unused method argument: kwargs
(ARG002)
🔇 Additional comments (4)
docs/source/workflows/mcp/mcp-service-account-auth.md (1)
1-306: Excellent documentation for the service account authentication feature.The documentation is comprehensive, well-structured, and addresses all past review feedback. It clearly explains when to use service account authentication, covers both single and dual authentication patterns, includes security best practices, and provides practical troubleshooting guidance. All code blocks have proper language specifiers, and the troubleshooting section uses proper markdown headings.
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py (1)
51-67: LGTM! Clean initialization of the token client.The initialization correctly sets up the
ServiceAccountTokenClientwith all required OAuth2 parameters. The unusedbuilderparameter is part of the provider interface contract and can be safely ignored.packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py (2)
24-72: Well-designed configuration schema.The configuration class provides a clean, flexible interface for OAuth2 client credentials authentication. All fields are properly typed with appropriate use of
SerializableSecretStrandOptionalSecretStrfor sensitive data. The defaults are sensible, and the documentation clearly explains each field's purpose.
74-84: Excellent validator for flexible scope configuration.The validator elegantly handles both list and string formats for scopes, making the configuration user-friendly whether scopes are defined inline as a YAML list or passed as a space-delimited environment variable. The implementation correctly strips whitespace and filters empty strings.
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py (1)
93-147: Access token expiration through a public property.Line 145 accesses the private attribute
_token_expires_atdirectly from the token client, breaking encapsulation. A past review comment already identified this issue and recommended adding a public property toServiceAccountTokenClient.
🧹 Nitpick comments (2)
docs/source/workflows/mcp/mcp-service-account-auth.md (1)
218-257: Use proper Markdown headings for options.The authentication pattern options use bold text instead of proper headings. Use level 4 headings (
####) for better document structure.Apply this diff:
-**Option 1: Static Service Token** +#### Option 1: Static Service Token ```yaml authentication: dual_auth: _type: mcp_service_account client_id: ${CLIENT_ID} client_secret: ${CLIENT_SECRET} token_url: ${TOKEN_URL} scopes: - service.scope service_token: token: ${SERVICE_TOKEN} # Static token from environment header: X-Service-Account-Token-Option 2: Dynamic Service Token (Advanced)
+#### Option 2: Dynamic Service Token (Advanced)</blockquote></details> <details> <summary>packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py (1)</summary><blockquote> `28-81`: **Well-designed dual-mode token configuration.** The mutual exclusivity validation between static token and dynamic function is correctly implemented, and the documentation clearly explains both usage patterns. The design supports flexible authentication scenarios effectively. *Optional nitpick:* Consider extracting the validation error messages to custom exception classes to address the TRY003 linting hints: ```python class ServiceTokenConfigError(ValueError): """Base exception for ServiceTokenConfig validation errors.""" pass class MissingTokenOrFunctionError(ServiceTokenConfigError): """Raised when neither token nor function is provided.""" def __init__(self): super().__init__("Either 'token' or 'function' must be provided in service_token config") class ConflictingTokenAndFunctionError(ServiceTokenConfigError): """Raised when both token and function are provided.""" def __init__(self): super().__init__("Cannot specify both 'token' and 'function' in service_token config. Choose one.")Then use in the validator:
@model_validator(mode="after") def validate_token_or_function(self): """Ensure either token or function is provided, but not both.""" has_token = self.token is not None has_function = self.function is not None if not has_token and not has_function: - raise ValueError("Either 'token' or 'function' must be provided in service_token config") + raise MissingTokenOrFunctionError() if has_token and has_function: - raise ValueError("Cannot specify both 'token' and 'function' in service_token config. Choose one.") + raise ConflictingTokenAndFunctionError() return self
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
docs/source/workflows/mcp/mcp-service-account-auth.md(1 hunks)examples/MCP/service_account_auth_mcp/README.md(1 hunks)examples/MCP/service_account_auth_mcp/configs(1 hunks)examples/MCP/service_account_auth_mcp/pyproject.toml(1 hunks)examples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/__init__.py(1 hunks)examples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/configs/config-mcp-service-account-jama.yml(1 hunks)examples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/configs/config-mcp-service-account-jira-function.yml(1 hunks)examples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/configs/config-mcp-service-account-jira.yml(1 hunks)examples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/scripts/__init__.py(1 hunks)examples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/scripts/service_tokens.py(1 hunks)examples/MCP/simple_auth_mcp/configs/config-mcp-auth-jira.yml(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py(1 hunks)pyproject.toml(2 hunks)tests/nat/mcp/test_mcp_service_account.py(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- examples/MCP/simple_auth_mcp/configs/config-mcp-auth-jira.yml
- examples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/scripts/init.py
- pyproject.toml
- examples/MCP/service_account_auth_mcp/configs
- examples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/init.py
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/MCP/service_account_auth_mcp/pyproject.toml
- examples/MCP/service_account_auth_mcp/README.md
🧰 Additional context used
📓 Path-based instructions (5)
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
examples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/configs/config-mcp-service-account-jira-function.ymlexamples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/configs/config-mcp-service-account-jira.ymlexamples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/configs/config-mcp-service-account-jama.ymltests/nat/mcp/test_mcp_service_account.pyexamples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/scripts/service_tokens.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.pydocs/source/workflows/mcp/mcp-service-account-auth.md
examples/**/*
⚙️ CodeRabbit configuration file
examples/**/*: - This directory contains example code and usage scenarios for the toolkit, at a minimum an example should
contain a README.md or file README.ipynb.
- If an example contains Python code, it should be placed in a subdirectory named
src/and should
contain apyproject.tomlfile. Optionally, it might also contain scripts in ascripts/directory.- If an example contains YAML files, they should be placed in a subdirectory named
configs/. - If an example contains sample data files, they should be placed in a subdirectory nameddata/, and should
be checked into git-lfs.
Files:
examples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/configs/config-mcp-service-account-jira-function.ymlexamples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/configs/config-mcp-service-account-jira.ymlexamples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/configs/config-mcp-service-account-jama.ymlexamples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/scripts/service_tokens.py
tests/**/*.py
⚙️ CodeRabbit configuration file
tests/**/*.py: - Ensure that tests are comprehensive, cover edge cases, and validate the functionality of the code. - Test functions should be named using thetest_prefix, using snake_case. - Any frequently repeated code should be extracted into pytest fixtures. - Pytest fixtures should define the name argument when applying the pytest.fixture decorator. The fixture
function being decorated should be named using thefixture_prefix, using snake_case. Example:
@pytest.fixture(name="my_fixture")
def fixture_my_fixture():
pass
Files:
tests/nat/mcp/test_mcp_service_account.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py
docs/source/**/*
⚙️ CodeRabbit configuration file
This directory contains the source code for the documentation. All documentation should be written in Markdown format. Any image files should be placed in the
docs/source/_staticdirectory.
Files:
docs/source/workflows/mcp/mcp-service-account-auth.md
🧠 Learnings (2)
📚 Learning: 2025-09-17T05:20:03.228Z
Learnt from: AnuradhaKaruppiah
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 752
File: packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py:99-121
Timestamp: 2025-09-17T05:20:03.228Z
Learning: The AuthAdapter class in packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py has a known concurrency race condition where `self.auth_provider.config.auth_request = auth_request` can be racy under concurrent requests. This should be addressed by either refactoring AuthProviderBase.authenticate() to accept AuthRequest as a parameter or using an asyncio.Lock to serialize access.
Applied to files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py
📚 Learning: 2025-09-17T05:34:04.696Z
Learnt from: AnuradhaKaruppiah
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 752
File: packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py:0-0
Timestamp: 2025-09-17T05:34:04.696Z
Learning: The user AnuradhaKaruppiah confirmed that request copying is not needed in the AuthAdapter.async_auth_flow method in packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py, suggesting that httpx handles request mutation safely or there are no concurrent usage concerns for this implementation.
Applied to files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py
🧬 Code graph analysis (4)
tests/nat/mcp/test_mcp_service_account.py (4)
src/nat/data_models/authentication.py (2)
AuthResult(194-245)HeaderCred(122-128)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py (1)
MCPServiceAccountProvider(31-147)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py (2)
MCPServiceAccountProviderConfig(84-135)ServiceTokenConfig(28-81)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py (2)
ServiceAccountTokenClient(27-153)get_access_token(78-104)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py (4)
src/nat/authentication/interfaces.py (1)
AuthProviderBase(29-72)src/nat/data_models/authentication.py (2)
AuthResult(194-245)HeaderCred(122-128)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py (1)
MCPServiceAccountProviderConfig(84-135)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py (2)
ServiceAccountTokenClient(27-153)get_access_token(78-104)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py (2)
tests/nat/front_ends/auth_flow_handlers/mock_oauth2_server.py (1)
token_url(143-144)src/nat/data_models/common.py (1)
get_secret_value(177-193)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py (1)
src/nat/data_models/authentication.py (1)
AuthProviderBaseConfig(31-37)
🪛 markdownlint-cli2 (0.18.1)
docs/source/workflows/mcp/mcp-service-account-auth.md
218-218: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
234-234: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Ruff (0.14.4)
tests/nat/mcp/test_mcp_service_account.py
39-39: Possible hardcoded password assigned to argument: "client_secret"
(S106)
40-40: Possible hardcoded password assigned to argument: "token_url"
(S106)
43-43: Possible hardcoded password assigned to argument: "token"
(S106)
54-54: Possible hardcoded password assigned to argument: "client_secret"
(S106)
55-55: Possible hardcoded password assigned to argument: "token_url"
(S106)
79-79: Possible hardcoded password assigned to argument: "token"
(S106)
105-105: Possible hardcoded password assigned to argument: "token"
(S106)
111-111: Possible hardcoded password assigned to argument: "token"
(S106)
279-279: Possible hardcoded password assigned to argument: "client_secret"
(S106)
280-280: Possible hardcoded password assigned to argument: "token_url"
(S106)
308-308: Unused function argument: vault_path
(ARG001)
308-308: Unused function argument: kwargs
(ARG001)
313-313: Possible hardcoded password assigned to argument: "client_secret"
(S106)
314-314: Possible hardcoded password assigned to argument: "token_url"
(S106)
343-343: Unused function argument: kwargs
(ARG001)
344-344: Avoid specifying long messages outside the exception class
(TRY003)
348-348: Possible hardcoded password assigned to argument: "client_secret"
(S106)
349-349: Possible hardcoded password assigned to argument: "token_url"
(S106)
428-428: Possible hardcoded password assigned to: "access_token"
(S105)
examples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/scripts/service_tokens.py
28-28: Unused function argument: kwargs
(ARG001)
74-75: Avoid specifying long messages outside the exception class
(TRY003)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py
59-59: Unused method argument: builder
(ARG002)
89-89: Consider moving this statement to an else block
(TRY300)
91-91: Avoid specifying long messages outside the exception class
(TRY003)
93-93: Unused method argument: user_id
(ARG002)
93-93: Unused method argument: kwargs
(ARG002)
137-137: Avoid specifying long messages outside the exception class
(TRY003)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py
143-143: Avoid specifying long messages outside the exception class
(TRY003)
145-145: Avoid specifying long messages outside the exception class
(TRY003)
147-148: Avoid specifying long messages outside the exception class
(TRY003)
151-151: Avoid specifying long messages outside the exception class
(TRY003)
153-153: Avoid specifying long messages outside the exception class
(TRY003)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py
76-76: Avoid specifying long messages outside the exception class
(TRY003)
79-79: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (15)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py (1)
27-61: LGTM with token caching and concurrency control.The token client correctly implements OAuth2 client credentials flow with intelligent caching and proper concurrency control via lazy-initialized lock. The double-checked locking pattern in
get_access_tokenprevents duplicate token requests under concurrent access.examples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/configs/config-mcp-service-account-jama.yml (1)
16-64: LGTM! Clear example of single authentication pattern.The configuration correctly demonstrates the simplified authentication pattern for MCP servers that only require OAuth2 service account tokens. The inline comments effectively explain the key differences from the dual authentication pattern.
examples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/configs/config-mcp-service-account-jira.yml (1)
16-60: LGTM! Demonstrates dual authentication pattern with static token.The configuration correctly shows the dual authentication pattern with a static service token from environment variables, which is appropriate for simpler deployment scenarios.
examples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/configs/config-mcp-service-account-jira-function.yml (1)
16-73: LGTM! Demonstrates dynamic token retrieval pattern.The configuration effectively shows the dynamic function approach for service token retrieval, which is useful for enterprise environments with secure vaults. The comments clearly explain the difference from the static approach and the expected function signature.
docs/source/workflows/mcp/mcp-service-account-auth.md (1)
1-350: Excellent comprehensive documentation.The documentation provides clear explanations of when to use service account authentication, supported capabilities, configuration options, security considerations, and troubleshooting guidance. The examples and diagrams effectively illustrate both single and dual authentication patterns.
examples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/scripts/service_tokens.py (1)
28-77: LGTM! Clear example of dynamic service token function.The function demonstrates the expected signature and return types for dynamic service token retrieval. The comprehensive docstring includes both a simple example and a production-oriented example showing how to integrate with secure vaults and access runtime context.
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py (2)
59-80: LGTM! Proper initialization and configuration handling.The constructor correctly initializes the token client with normalized scopes and handles optional dynamic service token function loading. The logging provides useful diagnostic information.
107-141: Well-implemented dual authentication pattern.The method correctly builds credentials for both single and dual authentication patterns, handling static tokens and dynamic function-based token retrieval. The error handling for dynamic functions is appropriate, and the support for both string and tuple return values from functions provides good flexibility.
tests/nat/mcp/test_mcp_service_account.py (4)
73-113: Excellent validation test coverage.The tests comprehensively validate
ServiceTokenConfigbehavior, including mutual exclusivity between static token and dynamic function, default header values, and error cases. This ensures the configuration validation works correctly.
120-223: Thorough token client testing.The token client tests cover the critical behaviors: successful token fetch with prefix, caching to prevent duplicate requests, 401 error handling, and network error handling. The verification of Basic Auth encoding and OAuth2 request format ensures correct implementation.
230-366: Comprehensive provider authentication tests.The provider tests validate both single and dual authentication patterns, static and dynamic token retrieval, error propagation, and dynamic function return type handling. The test coverage ensures the provider correctly builds credentials for all supported scenarios.
373-470: Excellent integration test coverage.The integration tests validate end-to-end flows including dual-header authentication, token refresh behavior, and complete OAuth2 client credentials flow with proper request format verification. This ensures the components work correctly together.
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py (3)
1-26: LGTM! License header and imports are correct.The Apache License 2.0 header is properly formatted with the correct 2025 copyright year, and imports are well-organized.
84-124: Excellent OAuth2 configuration with dual authentication support.The implementation correctly uses
list[str]for scopes, which addresses the previous review feedback about programmatic scope management. The design cleanly separates OAuth2 client credentials from optional service-specific token configuration, supporting both single and dual authentication patterns.
125-135: Robust scopes normalization.The validator elegantly handles both
list[str]and space-delimited string formats, providing flexibility in configuration while ensuring consistent internal representation. The whitespace handling is appropriate for OAuth2 scope lists.
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py
Outdated
Show resolved
Hide resolved
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/source/workflows/mcp/mcp-service-account-auth.md(1 hunks)examples/MCP/service_account_auth_mcp/README.md(1 hunks)examples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/configs/config-mcp-service-account-jira-function.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- examples/MCP/service_account_auth_mcp/README.md
- examples/MCP/service_account_auth_mcp/src/nat_service_account_auth_mcp/configs/config-mcp-service-account-jira-function.yml
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
docs/source/workflows/mcp/mcp-service-account-auth.md
docs/source/**/*
⚙️ CodeRabbit configuration file
This directory contains the source code for the documentation. All documentation should be written in Markdown format. Any image files should be placed in the
docs/source/_staticdirectory.
Files:
docs/source/workflows/mcp/mcp-service-account-auth.md
🪛 markdownlint-cli2 (0.18.1)
docs/source/workflows/mcp/mcp-service-account-auth.md
218-218: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
234-234: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (1)
docs/source/workflows/mcp/mcp-service-account-auth.md (1)
1-350: Documentation is comprehensive and well-structured.The file successfully documents the new MCP service account authentication provider with clear explanations of use cases, capabilities, configuration options, authentication patterns, security considerations, and troubleshooting guidance. The Apache 2.0 license header is present with the correct 2025 copyright year. All code blocks have appropriate language specifiers, security best practices are emphasized, and examples reference the accompanying implementation files. The troubleshooting section correctly uses proper headings instead of emphasis.
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py (1)
83-92: Add return type hint to method signature.As per coding guidelines, Python methods should use type hints for all parameters and return values.
Apply this diff:
+from typing import Callable, Any + - def _load_function(self, function_path: str) -> typing.Callable: + def _load_function(self, function_path: str) -> Callable[..., Any]: """Load a Python function from a module path string (e.g., 'my_module.get_token')."""Based on coding guidelines
🧹 Nitpick comments (1)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py (1)
122-138: Verify dynamic function async requirement.Line 127 uses
awaiton the service token function, which requires it to be an async function. If a user configures a synchronous function, this will raise aTypeErrorat runtime.Consider one of the following approaches:
- Document the requirement: Add a note in the docstring and configuration documentation that the dynamic function must be async.
- Support both sync and async: Use
asyncio.iscoroutinefunction()to detect function type and handle accordingly.Example for approach 2:
try: + if asyncio.iscoroutinefunction(self._service_token_function): + result = await self._service_token_function(**self.config.service_token.kwargs) + else: + result = self._service_token_function(**self.config.service_token.kwargs) - result = await self._service_token_function(**self.config.service_token.kwargs) # Handle function return type: str or tuple[str, str]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/source/workflows/mcp/mcp-service-account-auth.md(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/source/workflows/mcp/mcp-service-account-auth.md
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py
🧠 Learnings (2)
📚 Learning: 2025-09-17T05:20:03.228Z
Learnt from: AnuradhaKaruppiah
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 752
File: packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py:99-121
Timestamp: 2025-09-17T05:20:03.228Z
Learning: The AuthAdapter class in packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py has a known concurrency race condition where `self.auth_provider.config.auth_request = auth_request` can be racy under concurrent requests. This should be addressed by either refactoring AuthProviderBase.authenticate() to accept AuthRequest as a parameter or using an asyncio.Lock to serialize access.
Applied to files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py
📚 Learning: 2025-09-17T05:34:04.696Z
Learnt from: AnuradhaKaruppiah
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 752
File: packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py:0-0
Timestamp: 2025-09-17T05:34:04.696Z
Learning: The user AnuradhaKaruppiah confirmed that request copying is not needed in the AuthAdapter.async_auth_flow method in packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py, suggesting that httpx handles request mutation safely or there are no concurrent usage concerns for this implementation.
Applied to files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py
🧬 Code graph analysis (2)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py (4)
src/nat/authentication/interfaces.py (1)
AuthProviderBase(29-72)src/nat/data_models/authentication.py (2)
AuthResult(194-245)HeaderCred(122-128)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py (1)
MCPServiceAccountProviderConfig(84-135)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py (3)
ServiceAccountTokenClient(28-156)get_access_token(81-107)token_expires_at(65-66)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py (2)
tests/nat/front_ends/auth_flow_handlers/mock_oauth2_server.py (1)
token_url(143-144)src/nat/data_models/common.py (1)
get_secret_value(177-193)
🪛 Ruff (0.14.4)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py
60-60: Unused method argument: builder
(ARG002)
90-90: Consider moving this statement to an else block
(TRY300)
92-92: Avoid specifying long messages outside the exception class
(TRY003)
94-94: Unused method argument: user_id
(ARG002)
94-94: Unused method argument: kwargs
(ARG002)
138-138: Avoid specifying long messages outside the exception class
(TRY003)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py
146-146: Avoid specifying long messages outside the exception class
(TRY003)
148-148: Avoid specifying long messages outside the exception class
(TRY003)
150-151: Avoid specifying long messages outside the exception class
(TRY003)
154-154: Avoid specifying long messages outside the exception class
(TRY003)
156-156: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (2)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py (2)
64-66: LGTM! Encapsulation issue resolved.The public
token_expires_atproperty correctly addresses the past review concern about accessing private attributes from outside the class.
145-156: LGTM! Comprehensive error handling.The error handling correctly addresses different failure scenarios:
- Authentication failures (401)
- Rate limiting (429)
- Network errors (timeouts, request failures)
Exception chaining with
from eproperly preserves stack traces. The static analysis warnings about long error messages (TRY003) can be safely ignored here as the descriptive messages aid in debugging.
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py
Show resolved
Hide resolved
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py (1)
83-92: Add return type hint to method signature.The method lacks a return type hint. Per coding guidelines, Python methods should use type hints for all parameters and return values.
Apply this diff:
- def _load_function(self, function_path: str) -> typing.Callable: + def _load_function(self, function_path: str) -> typing.Callable[..., typing.Any]: """Load a Python function from a module path string (e.g., 'my_module.get_token')."""Based on coding guidelines
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py (2)
74-79: Use timezone-aware datetime for token expiration checks.Line 79 uses naive
datetime.now()without timezone information, butAuthResult.is_expired()usesdatetime.now(UTC). This inconsistency can cause incorrect token expiration checks, potentially leading to expired tokens being used or valid tokens being unnecessarily refreshed.Apply this diff:
+from datetime import UTC + import asyncio import base64 import logging from datetime import datetime from datetime import timedelta # ... rest of file ... def _is_token_valid(self) -> bool: """Check if cached token is still valid (with buffer time).""" if not self._cached_token or not self._token_expires_at: return False buffer = timedelta(seconds=self.token_cache_buffer_seconds) - return datetime.now() < (self._token_expires_at - buffer) + return datetime.now(UTC) < (self._token_expires_at - buffer)
131-143: Use timezone-aware datetime when caching token expiration.Line 140 uses naive
datetime.now()to calculate token expiration, creating the same timezone inconsistency issue. This should usedatetime.now(UTC)to ensure consistent timezone handling throughout the authentication flow.Apply this diff (assuming UTC is imported at the top):
access_token = token_data.get("access_token") if not access_token: raise RuntimeError("Access token not found in token response") self._cached_token = SecretStr(access_token) expires_in = token_data.get("expires_in", 3600) - self._token_expires_at = datetime.now() + timedelta(seconds=expires_in) + self._token_expires_at = datetime.now(UTC) + timedelta(seconds=expires_in) logger.info("Service account token acquired (expires in %ss)", expires_in)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py
🧠 Learnings (2)
📚 Learning: 2025-09-17T05:20:03.228Z
Learnt from: AnuradhaKaruppiah
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 752
File: packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py:99-121
Timestamp: 2025-09-17T05:20:03.228Z
Learning: The AuthAdapter class in packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py has a known concurrency race condition where `self.auth_provider.config.auth_request = auth_request` can be racy under concurrent requests. This should be addressed by either refactoring AuthProviderBase.authenticate() to accept AuthRequest as a parameter or using an asyncio.Lock to serialize access.
Applied to files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py
📚 Learning: 2025-09-17T05:34:04.696Z
Learnt from: AnuradhaKaruppiah
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 752
File: packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py:0-0
Timestamp: 2025-09-17T05:34:04.696Z
Learning: The user AnuradhaKaruppiah confirmed that request copying is not needed in the AuthAdapter.async_auth_flow method in packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py, suggesting that httpx handles request mutation safely or there are no concurrent usage concerns for this implementation.
Applied to files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py
🧬 Code graph analysis (2)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py (2)
tests/nat/front_ends/auth_flow_handlers/mock_oauth2_server.py (1)
token_url(143-144)src/nat/data_models/common.py (1)
get_secret_value(177-193)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py (4)
src/nat/authentication/interfaces.py (1)
AuthProviderBase(29-72)src/nat/data_models/authentication.py (2)
AuthResult(194-245)HeaderCred(122-128)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py (1)
MCPServiceAccountProviderConfig(84-135)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py (3)
ServiceAccountTokenClient(28-156)get_access_token(81-107)token_expires_at(65-66)
🪛 Ruff (0.14.4)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py
137-137: Avoid specifying long messages outside the exception class
(TRY003)
146-146: Avoid specifying long messages outside the exception class
(TRY003)
148-148: Avoid specifying long messages outside the exception class
(TRY003)
150-151: Avoid specifying long messages outside the exception class
(TRY003)
154-154: Avoid specifying long messages outside the exception class
(TRY003)
156-156: Avoid specifying long messages outside the exception class
(TRY003)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py
60-60: Unused method argument: builder
(ARG002)
90-90: Consider moving this statement to an else block
(TRY300)
92-92: Avoid specifying long messages outside the exception class
(TRY003)
94-94: Unused method argument: user_id
(ARG002)
94-94: Unused method argument: kwargs
(ARG002)
137-137: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Pipeline / Check
🔇 Additional comments (3)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py (1)
60-82: LGTM!The constructor correctly initializes the token client with OAuth2 credentials and conditionally loads a dynamic service token function. The unused
builderparameter is part of the provider factory interface pattern and can be safely ignored.packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py (2)
35-62: LGTM!The constructor properly initializes all OAuth2 client credential parameters with appropriate type hints and sets up token caching infrastructure. Lazy initialization of the asyncio lock is a good pattern.
64-72: LGTM!The
token_expires_atproperty correctly exposes the private expiration timestamp, addressing the previous encapsulation concern. The lazy initialization of the asyncio lock is properly typed and thread-safe.
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py
Show resolved
Hide resolved
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py (2)
68-77: Tighten_load_functiontyping and validate that the loaded object is callable.The loader works, but you can fail earlier and more clearly if the target attribute isn’t callable, and make the return type more precise:
- def _load_function(self, function_path: str) -> typing.Callable: + def _load_function(self, function_path: str) -> typing.Callable[..., typing.Any]: """Load a Python function from a module path string (e.g., 'my_module.get_token').""" try: module_name, func_name = function_path.rsplit(".", 1) module = importlib.import_module(module_name) - func = getattr(module, func_name) + func = getattr(module, func_name) + if not callable(func): + raise TypeError(f"Attribute '{func_name}' in module '{module_name}' is not callable") logger.info("Loaded service token function: %s", function_path) return func except Exception as e: raise ValueError(f"Failed to load service token function '{function_path}': {e}") from eThis keeps misconfiguration errors local to startup rather than failing later on the first request.
96-126: Dynamic service-token function handling is solid; consider small robustness tweaks.The logic cleanly prefers a static token, then falls back to a loaded function and supports both sync and async callables via
asyncio.iscoroutinefunction, with safe tuple/str handling and no secret logging. You might optionally harden this further by:
- Guarding against unexpected tuple shapes or non-str elements, and
- (Optionally) treating an awaitable return value from a sync-looking function (wrappers/partials) via
inspect.isawaitable.Not required for correctness, but could make the extension surface more forgiving to user-provided functions.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py(1 hunks)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py
packages/**/*
⚙️ CodeRabbit configuration file
packages/**/*: - This directory contains optional plugin packages for the toolkit, each should contain apyproject.tomlfile. - Thepyproject.tomlfile should declare a dependency onnvidia-nator another package with a name starting
withnvidia-nat-. This dependency should be declared using~=<version>, and the version should be a two
digit version (ex:~=1.0).
- Not all packages contain Python code, if they do they should also contain their own set of tests, in a
tests/directory at the same level as thepyproject.tomlfile.
Files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py
🧠 Learnings (1)
📚 Learning: 2025-09-17T05:20:03.228Z
Learnt from: AnuradhaKaruppiah
Repo: NVIDIA/NeMo-Agent-Toolkit PR: 752
File: packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py:99-121
Timestamp: 2025-09-17T05:20:03.228Z
Learning: The AuthAdapter class in packages/nvidia_nat_mcp/src/nat/plugins/mcp/client_base.py has a known concurrency race condition where `self.auth_provider.config.auth_request = auth_request` can be racy under concurrent requests. This should be addressed by either refactoring AuthProviderBase.authenticate() to accept AuthRequest as a parameter or using an asyncio.Lock to serialize access.
Applied to files:
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.pypackages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py
🧬 Code graph analysis (2)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py (1)
src/nat/data_models/authentication.py (1)
AuthProviderBaseConfig(31-37)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py (4)
src/nat/authentication/interfaces.py (1)
AuthProviderBase(29-72)src/nat/data_models/authentication.py (2)
AuthResult(194-245)HeaderCred(122-128)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py (1)
MCPServiceAccountProviderConfig(86-137)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py (3)
ServiceAccountTokenClient(28-156)get_access_token(81-107)token_expires_at(65-66)
🪛 Ruff (0.14.4)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py
78-78: Avoid specifying long messages outside the exception class
(TRY003)
81-81: Avoid specifying long messages outside the exception class
(TRY003)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py
45-45: Unused method argument: builder
(ARG002)
75-75: Consider moving this statement to an else block
(TRY300)
77-77: Avoid specifying long messages outside the exception class
(TRY003)
79-79: Unused method argument: user_id
(ARG002)
79-79: Unused method argument: kwargs
(ARG002)
126-126: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (2)
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py (1)
1-31: Header and imports look good.Apache-2.0 header is present and imports are minimal and appropriate for the module’s responsibilities.
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py (1)
1-26: Config module header and imports look correct.Apache-2.0 header is present, and imports are minimal and aligned with Pydantic v2 usage.
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py
Show resolved
Hide resolved
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py
Show resolved
Hide resolved
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py
Show resolved
Hide resolved
packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py
Show resolved
Hide resolved
Signed-off-by: Anuradha Karuppiah <anuradhak@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/nat/mcp/test_mcp_service_account.py (3)
34-65: Consider following fixture naming conventions.The coding guidelines specify that pytest fixtures should use the
nameparameter in the decorator and prefix the function name withfixture_.As per coding guidelines.
Example refactor for consistency:
-@pytest.fixture -def service_account_config() -> MCPServiceAccountProviderConfig: +@pytest.fixture(name="service_account_config") +def fixture_service_account_config() -> MCPServiceAccountProviderConfig:Apply similar changes to
minimal_configandmock_token_responsefixtures.
123-158: Add assertion for credential encoding verification.The comment on line 157 states "The Base64 encoded value should contain the actual secret, not **********" but doesn't follow up with an actual assertion to verify this claim.
Consider adding an assertion to verify the credential encoding:
# The Base64 encoded value should contain the actual secret, not ********** + # Verify credentials are properly encoded + import base64 + auth_header = headers["Authorization"].split("Basic ")[1] + decoded = base64.b64decode(auth_header).decode() + assert "test_client_id:test_client_secret" == decoded
37-57: Consider explicit SecretStr construction to avoid type: ignore.Multiple
type: ignorecomments are used throughout the test file to suppress type checking when passing strings to fields expectingSecretStrorSerializableSecretStr. While Pydantic handles the coercion, explicitly constructingSecretStrobjects would be cleaner.Example refactor:
return MCPServiceAccountProviderConfig( client_id="test_client_id", - client_secret="test_client_secret", # type: ignore + client_secret=SecretStr("test_client_secret"), token_url="https://auth.example.com/token", scopes="read write", service_token=ServiceTokenConfig( - token="test_service_token", # type: ignore + token=SecretStr("test_service_token"), header="X-Service-Account-Token", ), )Apply similar changes to other fixtures and test cases (lines 54, 79, 105, 111, 278, 312, 347).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/nat/mcp/test_mcp_service_account.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*
⚙️ CodeRabbit configuration file
**/*: # Code Review Instructions
- Ensure the code follows best practices and coding standards. - For Python code, follow
PEP 20 and
PEP 8 for style guidelines.- Check for security vulnerabilities and potential issues. - Python methods should use type hints for all parameters and return values.
Example:def my_function(param1: int, param2: str) -> bool: pass- For Python exception handling, ensure proper stack trace preservation:
- When re-raising exceptions: use bare
raisestatements to maintain the original stack trace,
and uselogger.error()(notlogger.exception()) to avoid duplicate stack trace output.- When catching and logging exceptions without re-raising: always use
logger.exception()
to capture the full stack trace information.Documentation Review Instructions - Verify that documentation and comments are clear and comprehensive. - Verify that the documentation doesn't contain any TODOs, FIXMEs or placeholder text like "lorem ipsum". - Verify that the documentation doesn't contain any offensive or outdated terms. - Verify that documentation and comments are free of spelling mistakes, ensure the documentation doesn't contain any
words listed in the
ci/vale/styles/config/vocabularies/nat/reject.txtfile, words that might appear to be
spelling mistakes but are listed in theci/vale/styles/config/vocabularies/nat/accept.txtfile are OK.Misc. - All code (except .mdc files that contain Cursor rules) should be licensed under the Apache License 2.0,
and should contain an Apache License 2.0 header comment at the top of each file.
- Confirm that copyright years are up-to date whenever a file is changed.
Files:
tests/nat/mcp/test_mcp_service_account.py
tests/**/*.py
⚙️ CodeRabbit configuration file
tests/**/*.py: - Ensure that tests are comprehensive, cover edge cases, and validate the functionality of the code. - Test functions should be named using thetest_prefix, using snake_case. - Any frequently repeated code should be extracted into pytest fixtures. - Pytest fixtures should define the name argument when applying the pytest.fixture decorator. The fixture
function being decorated should be named using thefixture_prefix, using snake_case. Example:
@pytest.fixture(name="my_fixture")
def fixture_my_fixture():
pass
Files:
tests/nat/mcp/test_mcp_service_account.py
🧬 Code graph analysis (1)
tests/nat/mcp/test_mcp_service_account.py (4)
src/nat/data_models/authentication.py (2)
AuthResult(194-245)HeaderCred(122-128)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider.py (1)
MCPServiceAccountProvider(33-136)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/provider_config.py (2)
MCPServiceAccountProviderConfig(86-137)ServiceTokenConfig(28-83)packages/nvidia_nat_mcp/src/nat/plugins/mcp/auth/service_account/token_client.py (3)
ServiceAccountTokenClient(28-156)get_access_token(81-107)token_expires_at(65-66)
🪛 Ruff (0.14.4)
tests/nat/mcp/test_mcp_service_account.py
39-39: Possible hardcoded password assigned to argument: "client_secret"
(S106)
40-40: Possible hardcoded password assigned to argument: "token_url"
(S106)
43-43: Possible hardcoded password assigned to argument: "token"
(S106)
54-54: Possible hardcoded password assigned to argument: "client_secret"
(S106)
55-55: Possible hardcoded password assigned to argument: "token_url"
(S106)
79-79: Possible hardcoded password assigned to argument: "token"
(S106)
105-105: Possible hardcoded password assigned to argument: "token"
(S106)
111-111: Possible hardcoded password assigned to argument: "token"
(S106)
278-278: Possible hardcoded password assigned to argument: "client_secret"
(S106)
279-279: Possible hardcoded password assigned to argument: "token_url"
(S106)
307-307: Unused function argument: vault_path
(ARG001)
307-307: Unused function argument: kwargs
(ARG001)
312-312: Possible hardcoded password assigned to argument: "client_secret"
(S106)
313-313: Possible hardcoded password assigned to argument: "token_url"
(S106)
342-342: Unused function argument: kwargs
(ARG001)
343-343: Avoid specifying long messages outside the exception class
(TRY003)
347-347: Possible hardcoded password assigned to argument: "client_secret"
(S106)
348-348: Possible hardcoded password assigned to argument: "token_url"
(S106)
427-427: Possible hardcoded password assigned to: "access_token"
(S105)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CI Pipeline / Check
🔇 Additional comments (6)
tests/nat/mcp/test_mcp_service_account.py (6)
1-27: LGTM!License header and imports are well-organized. Copyright year is current and all imports are relevant to the test suite.
73-113: Excellent test coverage for configuration validation!The configuration tests comprehensively cover both valid configurations (static token and dynamic function) and validation edge cases (missing both, providing both). The use of
pytest.raiseswithmatchparameters ensures error messages are validated.
159-223: LGTM!Token caching and error handling tests are comprehensive and properly structured. The local import of
httpxon line 207 appropriately avoids namespace conflicts with the mockedhttpx.AsyncClient.
233-301: Comprehensive provider authentication tests!The test coverage for the provider is excellent, validating both single and dual authentication patterns, with and without service tokens, and proper error propagation.
303-365: Excellent coverage of dynamic function scenarios!These tests thoroughly validate the dynamic token function feature, including both the tuple return pattern (custom header + token) and error handling. The mocking strategy correctly prevents actual module imports while testing the integration logic.
Note: Static analysis warnings about unused
kwargsparameters in mock functions (lines 307, 342) are false positives—these parameters intentionally match the expected function signatures.
372-469: Comprehensive integration test suite!The integration tests provide excellent end-to-end coverage, including:
- Dual-header authentication pattern validation
- Token refresh behavior when expiry is imminent
- Complete OAuth2 client credentials flow verification with proper request structure
The test at lines 407-435 cleverly uses
expires_in: 0to force immediate cache invalidation and verify refresh logic.Note: Static analysis warning about "hardcoded password" on line 427 is a false positive—this is a dictionary key name, not a credential.
|
/merge |
…DIA#1176) This PR: 1. Adds support for a service account auth provider that can be adapted to your enterprise requirements 2. Adds an example to verify functionality 3. Adds detailed docs for customizing and using the `mcp_service_account` auth provider. - I am familiar with the [Contributing Guidelines](https://github.com/NVIDIA/NeMo-Agent-Toolkit/blob/develop/docs/source/resources/contributing.md). - We require that all contributors "sign-off" on their commits. This certifies that the contribution is your original work, or you have rights to submit it under the same license, or a compatible license. - Any contribution which contains commits that are not Signed-Off will not be accepted. - When the PR is ready for review, new or existing tests cover these changes. - When the PR is ready for review, the documentation is up to date with these changes. * **New Features** * MCP Service Account authentication (OAuth2 client‑credentials) with token caching/refresh, optional secondary service‑token header, and improved default‑user handling for tool calls. * **Documentation** * New Service Account Authentication guide and clarified MCP OAuth2 docs distinguishing interactive vs headless flows. * **Examples** * New example project demonstrating Jira/Jama service‑account setups, dynamic token retrieval, sample configs, and README. * **Bug Fixes** * Auth header handling now aggregates multiple credential headers. * **Tests** * Comprehensive tests for service‑account flows, caching, refresh, and error cases. * **Chores** * Example packaging and vocabulary updates. Authors: - Anuradha Karuppiah (https://github.com/AnuradhaKaruppiah) Approvers: - Will Killian (https://github.com/willkill07) - David Gardner (https://github.com/dagardner-nv) URL: NVIDIA#1176
4 participants
Description
This PR:
mcp_service_accountauth provider.By Submitting this PR I confirm:
Summary by CodeRabbit
New Features
Documentation
Examples
Bug Fixes
Tests
Chores