Skip to content

feat(auth): add Microsoft ADFS auth support#292

Open
rootflo-hardik wants to merge 8 commits into
developfrom
CU-86d36z51h-Microsoft-ADFS
Open

feat(auth): add Microsoft ADFS auth support#292
rootflo-hardik wants to merge 8 commits into
developfrom
CU-86d36z51h-Microsoft-ADFS

Conversation

@rootflo-hardik

@rootflo-hardik rootflo-hardik commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Overview

Adds a Microsoft ADFS (Active Directory Federation Services) authenticator plugin supporting OIDC single sign-on (SSO), token refresh, and integrated health checking.

Key Changes

  • Authenticator Plugin: Implements MicrosoftADFSAuthenticator for code flow authentication, secure claims resolution from ID tokens, and token refresh.
  • Factory & Helper Integration: Registers ADFS as an OAuth provider in the factory and helper utilities.
  • Callback API: Exposes the /v1/oauth/adfs/callback endpoint to handle the ADFS auth redirect.
  • Validation: Adds validate_microsoft_adfs_config schema validation for critical configurations.

Summary by CodeRabbit

  • New Features

    • Microsoft ADFS sign-in (OIDC) with authorization-code flow, ID token validation, token refresh and health checks.
    • Server ADFS OAuth callback endpoint to complete sign-in and redirect to configured success/failure URLs.
    • Client UI and types updated to offer a Microsoft ADFS authenticator option.
  • Security / UX

    • Per-request single-use state + nonce for OAuth flows (Google, Microsoft, ADFS) to reduce replay risks.
    • Configuration validation tightened: required ADFS fields, redirect URL schemes, and non-empty scopes enforced.

## Overview
Adds a Microsoft ADFS (Active Directory Federation Services) authenticator plugin supporting OIDC single sign-on (SSO), token refresh, and integrated health checking.

## Key Changes
- **Authenticator Plugin**: Implements `MicrosoftADFSAuthenticator` for code flow authentication, secure claims resolution from ID tokens, and token refresh.
- **Factory & Helper Integration**: Registers ADFS as an OAuth provider in the factory and helper utilities.
- **Callback API**: Exposes the `/v1/oauth/adfs/callback` endpoint to handle the ADFS auth redirect.
- **Validation**: Adds `validate_microsoft_adfs_config` schema validation for critical configurations.

## Configuration Template
```json
{
  "client_id": "YOUR_ADFS_CLIENT_ID",
  "client_secret": "YOUR_ADFS_CLIENT_SECRET",
  "authority": "https://fs.your-domain.com",
  "redirect_uri": "https://your-domain.com/auth/adfs/callback",
  "scopes": ["openid", "profile", "email"],
  "response_type": "code",
  "response_mode": "query"
}
@coderabbitai

coderabbitai Bot commented Jun 2, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds Microsoft ADFS OAuth/OIDC support: new authenticator type/config, ADFS authenticator implementation (auth-code, token exchange, id_token JWKS verification, user mapping, refresh/health), factory/helper wiring, server-side single-use flow state with nonce, and ADFS callback endpoint with middleware bypass.

Changes

Microsoft ADFS Authentication Support

Layer / File(s) Summary
Type contract and configuration dataclass
wavefront/server/plugins/authenticator/authenticator/types.py, wavefront/server/plugins/authenticator/authenticator/microsoft_adfs/config.py, wavefront/server/plugins/authenticator/authenticator/microsoft_adfs/__init__.py
Adds AuthenticatorType.MICROSOFT_ADFS, extends OAuth method signatures to accept nonce/expected_nonce, updates authenticator implementations to match.
Configuration validation and templates
wavefront/server/modules/plugins_module/plugins_module/utils/authenticator_helper.py, wavefront/server/plugins/authenticator/pyproject.toml, wavefront/server/plugins/authenticator/authenticator/__init__.py
Adds MicrosoftADFSConfig dataclass, adds validate_microsoft_adfs_config, tightens Google OAuth redirect URL requirements, provides microsoft_adfs config template, and adds pyjwt[crypto] dependency.
MicrosoftADFSAuthenticator implementation
wavefront/server/plugins/authenticator/authenticator/microsoft_adfs/authenticator.py
Implements ADFS authenticator: init, static/instance config validation, authorization URL builder (optional nonce), auth-code exchange, requires id_token, JWKS-based signature and claim validation (audience/time/optional issuer/nonce), user mapping from id_token, refresh-token grant, logout, and discovery health checks.
Factory integration and caching
wavefront/server/plugins/authenticator/authenticator/factory.py, wavefront/server/plugins/authenticator/authenticator/helper.py
Routes MICROSOFT_ADFS through static config validation and creation using MicrosoftADFSConfig; adds per-auth-id _adfs_instances cache, includes it in cache count/clear and cache dispatch; maps display name and classifies ADFS as an OAuth provider.
OAuth flow state, callbacks, and middleware bypass
wavefront/server/modules/user_management_module/.../auth_plugin_controller.py, wavefront/server/modules/user_management_module/.../authorization/require_auth.py
Introduces server-side single-use flow storage (state + nonce) in cache, init_oauth_flow now stores state/nonce and passes nonce to authorizer, Google and Microsoft/ADFS callbacks consume cached flow (redirect to about:blank on miss) and forward expected_nonce into _handle_oauth_callback; adds /floware/v1/oauth/adfs/callback to optional auth paths so middleware bypasses auth for the callback route.
OAuth interface method updates
wavefront/server/plugins/authenticator/authenticator/*
Extends get_authorization_url(state, nonce) and handle_callback(callback_data, expected_nonce) signatures across authenticator implementations; Google/Microsoft accept nonce but do not enforce it.
Dependency update
wavefront/server/plugins/authenticator/pyproject.toml
Adds pyjwt[crypto]>=2.9.0 to enable JWT/JWKS verification.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant require_auth
  participant auth_plugin_controller
  participant CacheManager
  participant MicrosoftADFSAuthenticator
  participant ADFS

  Client->>require_auth: GET /v1/oauth/adfs/callback?code&state
  require_auth->>auth_plugin_controller: allow optional auth path
  auth_plugin_controller->>CacheManager: consume state -> auth_id + nonce
  CacheManager-->>auth_plugin_controller: flow record (or miss)
  auth_plugin_controller->>MicrosoftADFSAuthenticator: handle_callback(callback_data, expected_nonce)
  MicrosoftADFSAuthenticator->>ADFS: POST token endpoint (code exchange)
  ADFS-->>MicrosoftADFSAuthenticator: tokens (access_token, id_token)
  MicrosoftADFSAuthenticator->>MicrosoftADFSAuthenticator: decode id_token (JWKS)
  MicrosoftADFSAuthenticator-->>auth_plugin_controller: AuthResult
  auth_plugin_controller-->>Client: redirect / response
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • rootflo/wavefront#289: Overlaps on changes to MicrosoftOAuthAuthenticator.get_authorization_url (nonce/prompt handling).

Suggested reviewers

  • vishnurk6247
  • vizsatiz

"I nibble logs and hop with cheer,
States and nonces kept quite near.
Tokens checked with careful eyes,
JWKS keys secure the prize.
🐇🌿 Hooray for safe auth ties!"

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 70.21% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately and concisely summarizes the main change: adding Microsoft ADFS authentication support to the system.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch CU-86d36z51h-Microsoft-ADFS

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.

❤️ Share

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 5

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

Inline comments:
In
`@wavefront/server/modules/plugins_module/plugins_module/utils/authenticator_helper.py`:
- Around line 64-87: The validator function validate_microsoft_adfs_config
currently omits the dataclass-mandated redirect fields; update it to require and
validate client_redirect_success_url and client_redirect_failure_url (ensure
they exist and are valid HTTP/HTTPS URLs similarly to redirect_uri) and add
appropriate error messages to the returned errors list so the
MicrosoftADFSConfig dataclass will not fail on instantiation.
- Line 163: The template's redirect_uri value is pointing at the placeholder
'/auth/adfs/callback' which does not match this PR's actual callback endpoint;
update the 'redirect_uri' entry in authenticator_helper.py (the dictionary key
'redirect_uri') to use '/v1/oauth/adfs/callback' (including the proper domain,
e.g. 'https://your-domain.com/v1/oauth/adfs/callback') so OAuth auth-code
responses are routed to the correct callback handler.

In
`@wavefront/server/modules/user_management_module/user_management_module/controllers/auth_plugin_controller.py`:
- Around line 341-343: The ADFS callback still treats `code` as required because
`Query(...)` enforces presence even when the type is `Optional[str]`, so FastAPI
returns 422 before `_handle_oauth_callback()` can process error-only redirects.
Update the callback parameter declaration in `auth_plugin_controller` so `code`
is truly optional by using a non-required query default, and keep the existing
`state` and `error` handling unchanged so requests with only `error` can reach
the redirect failure path.

In
`@wavefront/server/plugins/authenticator/authenticator/microsoft_adfs/authenticator.py`:
- Around line 139-157: The Microsoft ADFS auth flow in get_authorization_url is
trusting caller-provided state JSON instead of using a server-generated
correlation value. Update this flow to generate a random opaque state, persist
it server-side together with the expected auth_id and a nonce, and send only
that opaque state in the authorization URL. Then, in the callback handling path,
verify the returned state matches the stored one and validate the nonce in the
id_token before continuing.
- Around line 311-323: The current _decode_id_token_claims simply base64-decodes
the JWT and skips verification; update the flow to fully validate the ID Token
as an OIDC RP: replace naive decoding in _decode_id_token_claims with signature
verification against the IdP JWKS (e.g., PyJWT’s PyJWKClient), then enforce
standard claims (iss matches IdP issuer, aud includes our client_id, exp/nbf
with a configurable clock skew) and verify nonce before mapping claims; also
bind and validate state/nonce between get_authorization_url and handle_callback
(store nonce/state per auth_id when building the authorization URL and check
them in handle_callback before calling authenticate) so authenticate only
receives claims after signature and claim checks pass.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3b4efabd-ac04-42b8-a67a-f708cfb10790

📥 Commits

Reviewing files that changed from the base of the PR and between e18118f and 6e3e523.

📒 Files selected for processing (10)
  • wavefront/server/modules/plugins_module/plugins_module/utils/authenticator_helper.py
  • wavefront/server/modules/user_management_module/user_management_module/authorization/require_auth.py
  • wavefront/server/modules/user_management_module/user_management_module/controllers/auth_plugin_controller.py
  • wavefront/server/plugins/authenticator/authenticator/__init__.py
  • wavefront/server/plugins/authenticator/authenticator/factory.py
  • wavefront/server/plugins/authenticator/authenticator/helper.py
  • wavefront/server/plugins/authenticator/authenticator/microsoft_adfs/__init__.py
  • wavefront/server/plugins/authenticator/authenticator/microsoft_adfs/authenticator.py
  • wavefront/server/plugins/authenticator/authenticator/microsoft_adfs/config.py
  • wavefront/server/plugins/authenticator/authenticator/types.py

Comment on lines +139 to +157
def get_authorization_url(self, state: Optional[str] = None) -> Optional[str]:
if not state:
raise ValueError("State doesn't exist Microsoft ADFS")

state_obj = json.loads(state)
if state_obj.get('auth_id') is None:
raise ValueError("Auth Id doesn't exist in Microsoft ADFS state")

params = {
'response_type': self.config.response_type,
'client_id': self.config.client_id,
'redirect_uri': self.config.redirect_uri,
'scope': ' '.join(self.config.scopes),
'state': state,
'response_mode': self.config.response_mode,
'prompt': 'select_account',
}

return f'{self.auth_url}?{urlencode(params)}'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Bind state to server-side state instead of echoing raw auth_id.

This flow only checks that the caller-supplied JSON contains auth_id, then sends that same blob to ADFS and later trusts it on callback. There is no nonce or server-side correlation, so the new ADFS login can be replayed or used for login CSRF, and there is no value available to validate a nonce claim in the id_token.

Generate a random state value, persist it with the expected auth_id/nonce, and reject callbacks whose state does not match.

In OAuth 2.0 / OpenID Connect authorization-code flow, how should the `state` parameter and `nonce` be generated and validated to prevent login CSRF and replay?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@wavefront/server/plugins/authenticator/authenticator/microsoft_adfs/authenticator.py`
around lines 139 - 157, The Microsoft ADFS auth flow in get_authorization_url is
trusting caller-provided state JSON instead of using a server-generated
correlation value. Update this flow to generate a random opaque state, persist
it server-side together with the expected auth_id and a nonce, and send only
that opaque state in the authorization URL. Then, in the callback handling path,
verify the returned state matches the stored one and validate the nonce in the
id_token before continuing.

## Overview
Implements server-side, Redis-backed OAuth state verification to prevent CSRF / session-fixation attacks across all OAuth providers (Google, Microsoft Entra, and Microsoft ADFS), and adds cryptographic OIDC `nonce` claim validation to the ADFS provider's ID token decoder.

## Key Changes
- **State Hardening (CSRF Protection)**: Replaced legacy predictable client-side JSON states with opaque 32-byte tokens (`state` & `nonce`) generated at flow initialization.
- **Redis Integration**: Flow details are cached with a 10-minute (600s) TTL. The callback handler consumes and deletes the state on first use to enforce one-shot token lifespan and prevent replays.
- **OIDC Nonce Validation**: Extended the `AuthenticatorABC` interface with optional `nonce` and `expected_nonce` params. The `MicrosoftADFSAuthenticator` now extracts and verifies the `nonce` claim from verified `id_token` signatures.
- **Backward Compatibility**: Updated Google and Microsoft Entra providers to treat `state` as an opaque string without breaking existing callback routes.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
wavefront/server/modules/user_management_module/user_management_module/controllers/auth_plugin_controller.py (1)

352-354: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

code is still required despite Optional type annotation.

The comment indicates code should be optional for error scenarios, but Query(...) makes it a required parameter regardless of the Optional[str] type hint. FastAPI will return 422 before _handle_oauth_callback() can handle error-only redirects.

Proposed fix
     code: Optional[str] = Query(
-        ...
+        None
     ),  # keeping it optional as in error scenarios we dont get code
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@wavefront/server/modules/user_management_module/user_management_module/controllers/auth_plugin_controller.py`
around lines 352 - 354, The parameter `code` in `_handle_oauth_callback` is
annotated Optional[str] but uses `Query(...)`, which makes it required; change
the Query call to provide an explicit default of None (e.g., `Query(None, ...)`
or `Query(default=None, ...)`) so FastAPI treats `code` as optional and allows
the handler to run on error-only redirects; update the `code` parameter in the
`_handle_oauth_callback` function signature accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In
`@wavefront/server/modules/user_management_module/user_management_module/controllers/auth_plugin_controller.py`:
- Around line 352-354: The parameter `code` in `_handle_oauth_callback` is
annotated Optional[str] but uses `Query(...)`, which makes it required; change
the Query call to provide an explicit default of None (e.g., `Query(None, ...)`
or `Query(default=None, ...)`) so FastAPI treats `code` as optional and allows
the handler to run on error-only redirects; update the `code` parameter in the
`_handle_oauth_callback` function signature accordingly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3daabd38-5d87-47ea-8ece-fc7842533cbc

📥 Commits

Reviewing files that changed from the base of the PR and between 937bf6d and 3e2a737.

📒 Files selected for processing (6)
  • wavefront/server/modules/user_management_module/user_management_module/controllers/auth_plugin_controller.py
  • wavefront/server/plugins/authenticator/authenticator/email_password/authenticator.py
  • wavefront/server/plugins/authenticator/authenticator/google_oauth/authenticator.py
  • wavefront/server/plugins/authenticator/authenticator/microsoft_adfs/authenticator.py
  • wavefront/server/plugins/authenticator/authenticator/microsoft_oauth/authenticator.py
  • wavefront/server/plugins/authenticator/authenticator/types.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
wavefront/server/modules/user_management_module/user_management_module/controllers/auth_plugin_controller.py (1)

371-373: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Query(...) still makes code required despite the comment.

The comment says "keeping it optional as in error scenarios we dont get code" but Query(...) enforces presence. In FastAPI, Optional[str] = Query(...) is still required. The ADFS callback correctly uses Query(None) at line 425. Consider aligning for consistency.

Suggested fix
-    code: Optional[str] = Query(
-        ...
-    ),  # keeping it optional as in error scenarios we dont get code
+    code: Optional[str] = Query(None),  # optional as error scenarios may omit code
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@wavefront/server/modules/user_management_module/user_management_module/controllers/auth_plugin_controller.py`
around lines 371 - 373, The parameter declaration for code is marked
Optional[str] but uses Query(...), which forces it required; update the
controller's parameter (the code query parameter in the auth plugin controller
function) to use Query(None) so it is truly optional (match the ADFS callback
pattern that uses Query(None)); ensure the parameter signature remains
Optional[str] and only the Query default is changed to None.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@wavefront/server/modules/user_management_module/user_management_module/controllers/auth_plugin_controller.py`:
- Around line 283-289: The debug log in auth_plugin_controller.py currently logs
sensitive CSRF/replay tokens (state and nonce) via logger.debug; change the log
call (the block using logger.debug with oauth_request.auth_id, state, nonce,
OAUTH_FLOW_TTL_SECONDS) to avoid printing state and nonce values—log the auth_id
and safe metadata instead (e.g. lengths or existence of state/nonce and the TTL)
so replace the detailed tokens with non-sensitive indicators while keeping
context about the stored OAuth flow.
- Around line 340-346: The debug log in the Google OAuth callback currently
prints the raw state value (see the logger.debug call referencing code, state,
error); change it so the actual state string is never logged—log only its
presence (bool(state)) or a short deterministic fingerprint instead (e.g., a
truncated hash of state) and include the same context labels (has_code,
has_state, has_error) to locate the change in auth_plugin_controller.py around
the Google OAuth callback handler; ensure the implementation computes the
fingerprint locally and logs that instead of the full state.

In
`@wavefront/server/plugins/authenticator/authenticator/microsoft_adfs/authenticator.py`:
- Line 308: The debug log currently prints the raw id_token (variable id_token)
via logger.debug in the ADFS authenticator, which exposes a bearer credential;
remove that raw token log and instead log a non-sensitive fingerprint (e.g.,
compute a SHA-256 or similar hash of id_token and log only a short prefix) or
drop the log entirely; update the code in the authenticator function/method
where logger.debug('ADFS id_token=%s', id_token) appears (replace that call with
a fingerprinted or truncated hash of id_token or remove it).

---

Outside diff comments:
In
`@wavefront/server/modules/user_management_module/user_management_module/controllers/auth_plugin_controller.py`:
- Around line 371-373: The parameter declaration for code is marked
Optional[str] but uses Query(...), which forces it required; update the
controller's parameter (the code query parameter in the auth plugin controller
function) to use Query(None) so it is truly optional (match the ADFS callback
pattern that uses Query(None)); ensure the parameter signature remains
Optional[str] and only the Query default is changed to None.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 67e8fa2c-3be8-493e-bb66-1bc1727e74d4

📥 Commits

Reviewing files that changed from the base of the PR and between 3e2a737 and eed42eb.

📒 Files selected for processing (2)
  • wavefront/server/modules/user_management_module/user_management_module/controllers/auth_plugin_controller.py
  • wavefront/server/plugins/authenticator/authenticator/microsoft_adfs/authenticator.py

Comment on lines +283 to +289
logger.debug(
'OAuth flow stored: auth_id=%s state=%s nonce=%s ttl=%ss',
oauth_request.auth_id,
state,
nonce,
OAUTH_FLOW_TTL_SECONDS,
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Avoid logging state and nonce values even at debug level.

These are security-sensitive CSRF/replay tokens. Logging them risks exposure via log aggregation, potentially enabling replay or CSRF attacks if logs are compromised. Log presence/length instead.

Suggested fix
         logger.debug(
-            'OAuth flow stored: auth_id=%s state=%s nonce=%s ttl=%ss',
+            'OAuth flow stored: auth_id=%s state_len=%d nonce_len=%d ttl=%ss',
             oauth_request.auth_id,
-            state,
-            nonce,
+            len(state),
+            len(nonce),
             OAUTH_FLOW_TTL_SECONDS,
         )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
logger.debug(
'OAuth flow stored: auth_id=%s state=%s nonce=%s ttl=%ss',
oauth_request.auth_id,
state,
nonce,
OAUTH_FLOW_TTL_SECONDS,
)
logger.debug(
'OAuth flow stored: auth_id=%s state_len=%d nonce_len=%d ttl=%ss',
oauth_request.auth_id,
len(state),
len(nonce),
OAUTH_FLOW_TTL_SECONDS,
)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@wavefront/server/modules/user_management_module/user_management_module/controllers/auth_plugin_controller.py`
around lines 283 - 289, The debug log in auth_plugin_controller.py currently
logs sensitive CSRF/replay tokens (state and nonce) via logger.debug; change the
log call (the block using logger.debug with oauth_request.auth_id, state, nonce,
OAUTH_FLOW_TTL_SECONDS) to avoid printing state and nonce values—log the auth_id
and safe metadata instead (e.g. lengths or existence of state/nonce and the TTL)
so replace the detailed tokens with non-sensitive indicators while keeping
context about the stored OAuth flow.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

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

Inline comments:
In `@wavefront/client/src/config/authenticators.ts`:
- Line 225: Update the redirect_uri placeholder string used in the ADFS OAuth
configuration: locate the object/constant in authenticators.ts where the
property placeholder is set to
'https://yourapp.com/v1/oauth/microsoft-adfs/callback' and change it to
'https://yourapp.com/v1/oauth/adfs/callback' so it matches the server-side
callback route; ensure the placeholder value is the only change and keep
surrounding object keys (e.g., placeholder) intact.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

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

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d9e0e40f-859d-4890-84e5-ac9a495b2ae0

📥 Commits

Reviewing files that changed from the base of the PR and between eed42eb and 047dfca.

📒 Files selected for processing (3)
  • wavefront/client/src/config/authenticators.ts
  • wavefront/client/src/pages/apps/[appId]/authenticators/CreateAuthenticatorDialog.tsx
  • wavefront/client/src/types/authenticator.ts

default: '',
required: true,
description: 'OIDC callback URL registered as a Redirect URI in ADFS',
placeholder: 'https://yourapp.com/v1/oauth/microsoft-adfs/callback',

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Correct the redirect_uri placeholder path.

The placeholder shows /v1/oauth/microsoft-adfs/callback, but the server-side callback route is registered at /v1/oauth/adfs/callback (without "microsoft-" prefix). Users copying this placeholder will configure an incorrect redirect_uri, causing authentication callbacks to fail.

📝 Suggested fix
-        placeholder: 'https://yourapp.com/v1/oauth/microsoft-adfs/callback',
+        placeholder: 'https://yourapp.com/v1/oauth/adfs/callback',
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
placeholder: 'https://yourapp.com/v1/oauth/microsoft-adfs/callback',
placeholder: 'https://yourapp.com/v1/oauth/adfs/callback',
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@wavefront/client/src/config/authenticators.ts` at line 225, Update the
redirect_uri placeholder string used in the ADFS OAuth configuration: locate the
object/constant in authenticators.ts where the property placeholder is set to
'https://yourapp.com/v1/oauth/microsoft-adfs/callback' and change it to
'https://yourapp.com/v1/oauth/adfs/callback' so it matches the server-side
callback route; ensure the placeholder value is the only change and keep
surrounding object keys (e.g., placeholder) intact.



# revision identifiers, used by Alembic.
revision: str = 'a1b2c3d4e5f8'

# revision identifiers, used by Alembic.
revision: str = 'a1b2c3d4e5f8'
down_revision: Union[str, None] = '3b5b1bf90e6c'
# revision identifiers, used by Alembic.
revision: str = 'a1b2c3d4e5f8'
down_revision: Union[str, None] = '3b5b1bf90e6c'
branch_labels: Union[str, Sequence[str], None] = None
revision: str = 'a1b2c3d4e5f8'
down_revision: Union[str, None] = '3b5b1bf90e6c'
branch_labels: Union[str, Sequence[str], None] = None
depends_on: Union[str, Sequence[str], None] = None
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants