Add 6 month default and max MCP token expiry#1043
Conversation
Agent-Logs-Url: https://github.com/IntelliTect/EssentialCSharp.Web/sessions/29b86c94-db59-4104-8285-197ae2428813 Co-authored-by: BenjaminMichaelis <22186029+BenjaminMichaelis@users.noreply.github.com>
Agent-Logs-Url: https://github.com/IntelliTect/EssentialCSharp.Web/sessions/29b86c94-db59-4104-8285-197ae2428813 Co-authored-by: BenjaminMichaelis <22186029+BenjaminMichaelis@users.noreply.github.com>
Agent-Logs-Url: https://github.com/IntelliTect/EssentialCSharp.Web/sessions/29b86c94-db59-4104-8285-197ae2428813 Co-authored-by: BenjaminMichaelis <22186029+BenjaminMichaelis@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Enforces a 6-month default and maximum expiration window for MCP API tokens across the service layer, API endpoint, and account management UI.
Changes:
- Added shared default/max expiry logic to
McpApiTokenServiceand applied it during token creation. - Updated MCP token creation validation/messaging in both the API controller and Identity management UI.
- Added integration tests covering default expiry, custom expiry within bounds, and rejection beyond the max.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| EssentialCSharp.Web/Views/McpSetup/Index.cshtml | Updates setup guide wording to reflect the new default expiry behavior. |
| EssentialCSharp.Web/Services/McpApiTokenService.cs | Centralizes 6-month default/max expiry calculation and enforces it in CreateTokenAsync. |
| EssentialCSharp.Web/Controllers/McpTokenController.cs | Adds max-expiry validation for the API token creation endpoint. |
| EssentialCSharp.Web/Areas/Identity/Pages/Account/Manage/McpAccess.cshtml.cs | Adds server-side expiry bounds/defaulting and validation in the account management flow. |
| EssentialCSharp.Web/Areas/Identity/Pages/Account/Manage/McpAccess.cshtml | Updates UI constraints (max attribute) and messaging for the new expiry rules. |
| EssentialCSharp.Web.Tests/McpApiTokenServiceTests.cs | Adds tests for default expiry, valid custom expiry, and max-expiry enforcement. |
| DateTime requestedExpiry = McpApiTokenService.GetDefaultExpirationUtc(DateTime.UtcNow).AddDays(1); | ||
|
|
||
| await Assert.That(() => tokenService.CreateTokenAsync(userId, "too-long", requestedExpiry)) | ||
| .Throws<ArgumentOutOfRangeException>() | ||
| .WithMessageContaining("6 months"); |
There was a problem hiding this comment.
This test can be flaky around a UTC midnight boundary: requestedExpiry is computed from DateTime.UtcNow, but CreateTokenAsync computes createdAt from a separate UtcNow. If the date rolls over between those calls, the service's computed max can advance by a day and the requestedExpiry may no longer be beyond the max, causing intermittent failures. Consider making requestedExpiry unambiguously beyond any possible max (e.g., more than 1 day past), or refactoring token creation to accept/inject a reference time so tests can be deterministic.
| public class McpApiTokenService(EssentialCSharpWebContext db) | ||
| { | ||
| public const int DefaultLifetimeMonths = 6; | ||
| public const string MaxExpiryValidationMessage = "MCP tokens can expire at most 6 months from today."; |
There was a problem hiding this comment.
MaxExpiryValidationMessage hard-codes "6 months" while DefaultLifetimeMonths is the actual source of truth. If the lifetime ever changes, the validation message (and any consumers that display it) will become inaccurate; consider formatting the message from DefaultLifetimeMonths (and optionally include the computed max date for clarity).
| public const string MaxExpiryValidationMessage = "MCP tokens can expire at most 6 months from today."; | |
| public static string MaxExpiryValidationMessage => $"MCP tokens can expire at most {DefaultLifetimeMonths} months from today."; |
| DateTime? expiresAt = null; | ||
| DateOnly maxExpiresOn = McpApiTokenService.GetDefaultExpiryDate(); | ||
| if (request?.ExpiresOn is DateOnly expiresOn) | ||
| { | ||
| if (expiresOn < DateOnly.FromDateTime(DateTime.UtcNow)) |
There was a problem hiding this comment.
DateTime.UtcNow is evaluated multiple times during validation (GetDefaultExpiryDate() and the expiresOn < DateOnly.FromDateTime(DateTime.UtcNow) check). If the request crosses a UTC midnight boundary between calls, the computed maxExpiresOn and "today" could disagree, causing inconsistent accept/reject behavior. Capture a single nowUtc at the top of the action and derive both bounds from it (you can pass it into GetDefaultExpiryDate(nowUtc)).
| DateTime? expiresAt = null; | |
| DateOnly maxExpiresOn = McpApiTokenService.GetDefaultExpiryDate(); | |
| if (request?.ExpiresOn is DateOnly expiresOn) | |
| { | |
| if (expiresOn < DateOnly.FromDateTime(DateTime.UtcNow)) | |
| DateTime nowUtc = DateTime.UtcNow; | |
| DateOnly todayUtc = DateOnly.FromDateTime(nowUtc); | |
| DateTime? expiresAt = null; | |
| DateOnly maxExpiresOn = McpApiTokenService.GetDefaultExpiryDate(nowUtc); | |
| if (request?.ExpiresOn is DateOnly expiresOn) | |
| { | |
| if (expiresOn < todayUtc) |
| DisableCaching(); | ||
| InitializeExpiryBounds(); | ||
| ApplyDefaultExpiryIfMissing(); | ||
| string? userId = userManager.GetUserId(User); | ||
| if (userId is null) return Challenge(); | ||
|
|
||
| if (string.IsNullOrWhiteSpace(TokenName)) | ||
| ModelState.AddModelError(nameof(TokenName), "Token name is required."); | ||
|
|
||
| if (ExpiresOn.HasValue && ExpiresOn.Value < DateOnly.FromDateTime(DateTime.UtcNow)) | ||
| ModelState.AddModelError(nameof(ExpiresOn), "Expiry date must be today or in the future."); | ||
| if (ExpiresOn.HasValue) | ||
| { | ||
| if (ExpiresOn.Value < DateOnly.FromDateTime(DateTime.UtcNow)) | ||
| ModelState.AddModelError(nameof(ExpiresOn), "Expiry date must be today or in the future."); | ||
| if (ExpiresOn.Value > MaxExpiresOn) | ||
| ModelState.AddModelError(nameof(ExpiresOn), McpApiTokenService.MaxExpiryValidationMessage); |
There was a problem hiding this comment.
InitializeExpiryBounds() (via GetDefaultExpiryDate()) and the "today" check each call DateTime.UtcNow separately. Around a UTC date rollover, this can yield inconsistent validation (e.g., MaxExpiresOn computed from one day but the comparison uses the next day). Capture a single nowUtc per request and use it for both MaxExpiresOn and the minimum-date validation (e.g., DateOnly.FromDateTime(nowUtc)).
| <input asp-for="ExpiresOn" type="date" class="form-control" max="@Model.MaxExpiresOn.ToString("yyyy-MM-dd")" /> | ||
| <span asp-validation-for="ExpiresOn" class="text-danger"></span> | ||
| <div class="form-text">Leave blank for a non-expiring token. The token expires at end of day (23:59:59) UTC on the selected date.</div> | ||
| <div class="form-text">MCP tokens default to 6 months and cannot exceed 6 months from today. The token expires at end of day (23:59:59) UTC on the selected date.</div> |
There was a problem hiding this comment.
The help text says the token expires at end of day "(23:59:59) UTC", but the code persists TimeOnly.MaxValue (23:59:59.9999999). To avoid inaccurate UX/docs, consider changing the text to just "end of day (UTC)" or align it with the actual persisted precision.
| <div class="form-text">MCP tokens default to 6 months and cannot exceed 6 months from today. The token expires at end of day (23:59:59) UTC on the selected date.</div> | |
| <div class="form-text">MCP tokens default to 6 months and cannot exceed 6 months from today. The token expires at end of day (UTC) on the selected date.</div> |
Uh oh!
There was an error while loading. Please reload this page.