fix(directory): drive VMACS endpoint from config, fail closed if unset#262
Merged
Conversation
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
📝 WalkthroughWalkthroughVMACS directory access now uses configured base URLs, safe XML deserialization, and a dedicated health check. Registration adds adaptive polling and environment-specific token handling, while tests cover parsing, security, configuration, HTTP outcomes, cancellation, and request construction. ChangesVMACS directory integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HealthCheck as VmacsDirectoryHealthCheck
participant HttpClient
participant VMACS as VMACS endpoint
participant Parser as VMACSService.Deserialize
HealthCheck->>HttpClient: Send directory probe with login and AUTH token
HttpClient->>VMACS: HTTP GET request
VMACS-->>HttpClient: HTTP response and XML body
HttpClient-->>HealthCheck: Response status and body
HealthCheck->>Parser: Deserialize XML body
Parser-->>HealthCheck: VMACSQuery or InvalidOperationException
HealthCheck-->>HealthCheck: Return mapped health status
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@web/Areas/Directory/Services/VMACSService.cs`:
- Around line 31-42: Require VmacsBaseUrl to parse as an absolute HTTP or HTTPS
URI before VMACSService calls sharedClient.GetAsync; return null and preserve
the existing missing/invalid configuration logging behavior otherwise. Add or
reuse a shared helper for this validation, and update VmacsDirectoryHealthCheck
to use it and report Unhealthy for invalid HTTP/S configuration. Apply the
service change in web/Areas/Directory/Services/VMACSService.cs lines 31-42 and
the health-check change in web/Classes/HealthChecks/VmacsDirectoryHealthCheck.cs
lines 49-67.
In `@web/Classes/HealthChecks/HealthCheckExtensions.cs`:
- Around line 197-198: Update the healthyCacheDuration and
unhealthyCacheDuration arguments in the health-check registration to use the
existing bounded policy of one hour for successful probes and five minutes for
failed probes, or equivalent shorter intervals; leave the surrounding
health-check configuration unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5e684d1a-13f8-4343-a550-7f45c8afff2c
📒 Files selected for processing (8)
test/Areas/Directory/VMACSServiceTest.cstest/HealthChecks/VmacsDirectoryHealthCheckTests.csweb/Areas/Directory/Services/VMACSService.csweb/Classes/HealthChecks/HealthCheckExtensions.csweb/Classes/HealthChecks/VmacsDirectoryHealthCheck.csweb/appsettings.Development.jsonweb/appsettings.Production.jsonweb/appsettings.Test.json
rlorenzo
force-pushed
the
fix/vmacs-qa-endpoint
branch
from
July 17, 2026 19:46
8493473 to
30ebb6e
Compare
Bundle ReportBundle size has no change ✅ |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #262 +/- ##
==========================================
+ Coverage 45.31% 45.39% +0.07%
==========================================
Files 915 916 +1
Lines 52690 52784 +94
Branches 4992 5005 +13
==========================================
+ Hits 23877 23960 +83
- Misses 28186 28196 +10
- Partials 627 628 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
rlorenzo
force-pushed
the
fix/vmacs-qa-endpoint
branch
2 times, most recently
from
July 17, 2026 21:25
9635358 to
e6f59ae
Compare
There was a problem hiding this comment.
Pull request overview
This PR removes the hardcoded VMACS directory lookup host and makes the directory query endpoint environment-configurable via Vmacs:BaseUrl, adding a targeted health check for the /trust/query.xml endpoint and hardening failure behavior so misconfiguration can’t silently hit production.
Changes:
- Make
VMACSService.Search()drive the VMACS base URL from configuration and fail closed when unset/invalid; move XML parsing into a reusableDeserialize()helper with safer degradation on malformed responses. - Add
campus-vmacs-directoryhealth check that probes the real authenticated directory query endpoint with adaptive polling and severity mapping. - Declare
Vmacs:BaseUrlper environment inappsettings.{Development,Test,Production}.json.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| web/Classes/HealthChecks/VmacsDirectoryHealthCheck.cs | Adds an endpoint-specific health check that authenticates and validates response shape for /trust/query.xml. |
| web/Classes/HealthChecks/HealthCheckExtensions.cs | Registers the new adaptive-polling campus-vmacs-directory health check and wires config values into it. |
| web/Areas/Directory/Services/VMACSService.cs | Removes hardcoded prod host, validates config-driven base URL, and hardens XML deserialization behavior. |
| web/appsettings.Development.json | Adds Vmacs:BaseUrl pointing to QA for Development. |
| web/appsettings.Test.json | Adds Vmacs:BaseUrl pointing to QA for Test. |
| web/appsettings.Production.json | Adds Vmacs:BaseUrl pointing to prod for Production. |
| test/HealthChecks/VmacsDirectoryHealthCheckTests.cs | Adds coverage for the new health check’s status mapping and request construction. |
| test/Areas/Directory/VMACSServiceTest.cs | Adds tests for base URL validation and XML deserialization behavior. |
rlorenzo
force-pushed
the
fix/vmacs-qa-endpoint
branch
from
July 17, 2026 21:58
e6f59ae to
bd82e3b
Compare
rlorenzo
force-pushed
the
fix/vmacs-qa-endpoint
branch
from
July 17, 2026 22:12
bd82e3b to
442b383
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
web/Areas/Directory/Services/VMACSService.cs:135
- BuildSearchPath interpolates the VMACS auth token directly into the query string without URL-encoding. If the token contains characters like '+', '/', or '=' (common in tokens), the request URI can be malformed or interpreted incorrectly. URL-encode the token the same way loginID is encoded.
string encodedLoginId = Uri.EscapeDataString(loginID ?? string.Empty);
return $"/trust/query.xml?dbfile=3&index=CampusLoginId&find={encodedLoginId}&format=CHRIS4&AUTH={authToken}";
}
- Read base URL from Vmacs:BaseUrl (vmacs-qa in dev/test, vmacs-vmth in prod); remove the hardcoded prod fallback - Skip the lookup and log once when unset, so a misconfigured box returns no results instead of silently hitting prod - Degrade to null (log a warning) on a malformed VMACS response instead of failing the directory search - Add campus-vmacs-directory health check: an authenticated /trust/query.xml probe reporting an outage as Degraded and a missing URL or rejected token as Unhealthy, checked ~daily while healthy and hourly while down
rlorenzo
force-pushed
the
fix/vmacs-qa-endpoint
branch
from
July 17, 2026 23:28
442b383 to
4a71b4d
Compare
Contributor
Author
|
@bsedwards Alex complained that hitting the production URL for the VMACS Trust URL was causing too many emails to be sent to them. So this PR changes our code to use the QA URL for Development and TEST environments. |
bsedwards
approved these changes
Jul 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
The VMACS directory lookup endpoint was hardcoded to the production host (
vmacs-vmth), so Development and TEST were querying prod. This makes the endpoint configurable per environment and hardens the failure behavior.Changes
Endpoint is config-driven, fails closed
VMACSServicereads the base URL fromVmacs:BaseUrl(no hardcoded fallback).Search()only calls the endpoint when the URL parses as an absolutehttp(s)URI andCredentials:VmacsAuthTokenis configured; otherwise it skips the request, returns no results, and logs once per process. A misconfigured box can never silently hit prod, a malformed/unsupported-scheme URL fails closed instead of throwing out of the lookup, and an environment with no token (e.g. Development) won't fan out one doomed unauthorized request per directory result.HttpRequestException), timeouts (TaskCanceledException— the sharedHttpClientis bounded to a 10s timeout so a hung endpoint can't stall a lookup for the default 100s), and malformed responses all degrade to null + a warning instead of throwing out of the lookup. This matters because the endpoint goes down often andDirectoryControllerblocks onSearch().Resultper result — an unhandled throw would 500 the whole search./health/detailpayload (the probe URI carries the auth token), reporting a generic "unreachable" instead.Config (each environment declares its own endpoint)
Vmacs:BaseUrlhttps://vmacs-qa.vetmed.ucdavis.eduhttps://vmacs-qa.vetmed.ucdavis.eduhttps://vmacs-vmth.vetmed.ucdavis.eduThe base
appsettings.jsonintentionally has noVmacssection, so "unset" is a real, detectable state rather than a silent prod default.Endpoint-specific health check (
campus-vmacs-directory)/trust/query.xmldirectory endpoint (the oneSearchuses) with an authenticated query, so a failure pinpoints that endpoint being down rather than a bug in VIPER — the VMACS team reports this endpoint goes down often._testdvm. It need not resolve to a record — a valid token returns HTTP 200 with an empty<query>for an unknown id — so the check stays green across environments with different datasets (QA is sparse) and never depends on a real person's data./healthliveness — VMACS being down never pulls VIPER out of rotation.How often the endpoint is called
healthyCacheDuration: 1 day) and hourly while down (unhealthyCacheDuration: 1 hour) viaAdaptivePollingHealthCheck. These windows are intentionally wider than the shared 1h/5min helper because this endpoint is known to be flaky and directory enrichment is optional — probing sparingly avoids alert noise on a non-critical dependency.Search()calls the endpoint once per matched user in a result set (DirectoryControllerinvokes it per person as it builds the response). When the base URL or auth token is unset/invalid it skips entirely — zero calls — logging once per process.Testing
VMACSServiceTest(24, incl. new absolute-http(s)-URL validation cases) andVmacsDirectoryHealthCheckTests(21) pass; full backend suite green._testdvmprobe.Targets
Developmentper the branch/deploy flow (deploys to TEST first).