Open
Conversation
Enables authenticated users to generate, view, and revoke API keys for programmatic access. Provides a dedicated settings page in the client for users to manage their keys securely, including displaying a key hint and creation date, and handling one-time display of newly generated raw keys. Implements secure server-side handling of API keys, utilizing PBKDF2 for hashing and HMAC for efficient, constant-time lookup. A new authentication scheme is configured to accept API keys via the `Authorization` header, seamlessly integrating with existing session-based authentication.
Contributor
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end API key support: client React Query hooks and a Settings UI; server domain model, EF migration, service, controller, API-key authentication handler, DI registration, and development HMAC secret configuration. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Controller as ApiKeyController
participant Service as ApiKeyService
participant DB as Database
Client->>Controller: POST /api/apikey (generate)
Controller->>Service: GenerateApiKeyAsync(userId, ct)
Service->>Service: generate rawKey, salt, PBKDF2 hash, HMAC lookup
Service->>DB: delete existing ApiKey where UserId
Service->>DB: insert new ApiKey (Lookup, Hash, Salt, KeyHint, CreatedAt)
DB-->>Service: persisted
Service-->>Controller: (RawKey, KeyHint, CreatedAt)
Controller-->>Client: 200 OK with generated key
sequenceDiagram
autonumber
actor Client
participant Handler as ApiKeyAuthenticationHandler
participant Service as ApiKeyService
participant DB as Database
Client->>Handler: Request with Authorization: ApiKey <rawKey>
Handler->>Service: ValidateApiKeyAsync(rawKey)
Service->>Service: decode key, compute HMAC lookup
Service->>DB: query ApiKey by Lookup
DB-->>Service: ApiKey record (Hash, Salt, UserId)
Service->>Service: recompute PBKDF2 hash, constant-time compare
alt valid
Service-->>Handler: UserId
Handler-->>Client: Authenticated (ClaimsPrincipal)
else invalid
Service-->>Handler: null
Handler-->>Client: 401 Unauthorized
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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: 4
🧹 Nitpick comments (2)
client/src/queries/apikey.ts (1)
2-2: Use@/alias instead of a relative client import path.Switching this import keeps client code aligned with the project’s import-resolution convention.
♻️ Suggested import update
-import { fetchJson } from '../lib/api.ts'; +import { fetchJson } from '@/lib/api.ts';As per coding guidelines "Use path alias
@/to resolve to./src/in client imports".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/queries/apikey.ts` at line 2, Replace the relative import of fetchJson in the apikey query with the project path-alias form: update the import statement that currently references "../lib/api.ts" to use "@/lib/api" so the file imports fetchJson via the alias (look for the fetchJson import in client/src/queries/apikey.ts and update that import to use "@/lib/api"). Ensure the .ts extension usage follows repository conventions (remove or keep extension consistently with other client imports).client/src/routes/(authenticated)/settings/index.tsx (1)
2-7: Prefer the shareduseApiKeyQueryhook to avoid duplicate query wiring.This keeps query consumption consistent across route components and centralizes query behavior in one place.
♻️ Optional cleanup
import { createFileRoute } from '@tanstack/react-router'; import { apiKeyQueryOptions, + useApiKeyQuery, useGenerateApiKeyMutation, useRevokeApiKeyMutation, } from '@/queries/apikey.ts'; -import { useQuery } from '@tanstack/react-query'; import { useState, useRef } from 'react'; @@ function SettingsPage() { - const { data: apiKeyInfo, isLoading } = useQuery(apiKeyQueryOptions()); + const { data: apiKeyInfo, isLoading } = useApiKeyQuery();Also applies to: 17-17
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/src/routes/`(authenticated)/settings/index.tsx around lines 2 - 7, Replace the direct useQuery wiring with the shared hook: remove the useQuery import and apiKeyQueryOptions usage and instead call the central useApiKeyQuery hook (e.g., useApiKeyQuery()) in this component to get the API key state; keep the existing useGenerateApiKeyMutation and useRevokeApiKeyMutation usage but consume the data and status from useApiKeyQuery (symbols: useApiKeyQuery, useGenerateApiKeyMutation, useRevokeApiKeyMutation) so query behavior is centralized and duplicate query options are eliminated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server.core/Data/AppDbContext.cs`:
- Line 18: The DbSet ApiKeys in AppDbContext is added but no EF migration
exists, causing runtime table-not-found errors when ApiKeyService accesses
_db.ApiKeys; generate and apply a migration by running dotnet ef migrations add
AddApiKeys (or equivalent migration name), inspect the generated migration to
ensure it creates the ApiKeys table with the correct schema for the ApiKey
model, then run dotnet ef database update to apply it to the target database so
the AppDbContext/ApiKeyService can operate without errors.
In `@server/Controllers/ApiKeyController.cs`:
- Around line 32-41: The GenerateApiKey action returns a sensitive RawKey
(GenerateApiKeyResponse) and must prevent caching; update
ApiKeyController.GenerateApiKey to add no-store cache directives before
returning — either decorate the method with [ResponseCache(NoStore = true,
Location = ResponseCacheLocation.None)] or explicitly set response headers
(e.g., Response.Headers["Cache-Control"]="no-store",
Response.Headers["Pragma"]="no-cache") so browsers/intermediaries do not persist
RawKey; keep the rest of the method logic unchanged.
In `@server/Services/ApiKeyService.cs`:
- Around line 99-113: In ValidateApiKeyAsync, add input size guards to avoid
expensive/invalid decodes: first check rawKey for reasonable Base64Url length
(e.g. reject if rawKey.Length is unreasonably small or large; for a 32-byte
secret expect ~44 chars so enforce a small range around that) before calling
Base64UrlEncoder.DecodeBytes, then after decoding verify secretBytes.Length
equals the expected SecretBytes constant (32) and return null if it does not;
only then proceed to compute HMACSHA256.HashData(_hmacKey, secretBytes) and the
DB lookup. Ensure you reference ValidateApiKeyAsync, rawKey, secretBytes,
SecretBytes (32), and HMACSHA256.HashData in the change.
- Around line 78-95: The delete-then-insert for ApiKey rotation in ApiKeyService
is not atomic; wrap the removal of existing keys and insertion of the new ApiKey
(the ExecuteDeleteAsync call and the subsequent _db.ApiKeys.Add +
SaveChangesAsync) in a single database transaction (e.g., begin a transaction on
the DbContext, perform the delete and add inside it, then commit) so any failure
rolls back and avoids leaving the user without a key; alternatively use a
database upsert/merge if supported, but ensure the code around
ExecuteDeleteAsync, _db.ApiKeys.Add(apiKey), and await _db.SaveChangesAsync(ct)
executes inside a single transaction scope.
---
Nitpick comments:
In `@client/src/queries/apikey.ts`:
- Line 2: Replace the relative import of fetchJson in the apikey query with the
project path-alias form: update the import statement that currently references
"../lib/api.ts" to use "@/lib/api" so the file imports fetchJson via the alias
(look for the fetchJson import in client/src/queries/apikey.ts and update that
import to use "@/lib/api"). Ensure the .ts extension usage follows repository
conventions (remove or keep extension consistently with other client imports).
In `@client/src/routes/`(authenticated)/settings/index.tsx:
- Around line 2-7: Replace the direct useQuery wiring with the shared hook:
remove the useQuery import and apiKeyQueryOptions usage and instead call the
central useApiKeyQuery hook (e.g., useApiKeyQuery()) in this component to get
the API key state; keep the existing useGenerateApiKeyMutation and
useRevokeApiKeyMutation usage but consume the data and status from
useApiKeyQuery (symbols: useApiKeyQuery, useGenerateApiKeyMutation,
useRevokeApiKeyMutation) so query behavior is centralized and duplicate query
options are eliminated.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
client/src/queries/apikey.tsclient/src/routeTree.gen.tsclient/src/routes/(authenticated)/settings/index.tsxclient/src/routes/__root.tsxserver.core/Data/AppDbContext.csserver.core/Domain/ApiKey.csserver.core/Domain/User.csserver/Controllers/ApiKeyController.csserver/Helpers/ApiKeyAuthenticationHandler.csserver/Helpers/AuthenticationHelper.csserver/Program.csserver/Services/ApiKeyService.csserver/appsettings.Development.json
Explicitly sets the text color of the API key input to black. This improves visibility and ensures the key remains readable across different browser or theme settings.
Introduces the `ApiKeys` table to securely store API key credentials. Includes fields for `Hash`, `Salt`, `Lookup` (for efficient retrieval), and `KeyHint`. Establishes a one-to-one relationship with the `User` entity and adds indexes for optimal performance and uniqueness.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server.core/Migrations/20260303172131_ApiKey.cs`:
- Around line 38-41: Change the non-unique index created with
migrationBuilder.CreateIndex (name: IX_ApiKeys_Lookup, table: ApiKeys, column:
Lookup) to be a unique index so Lookup values cannot collide, and add logic in
the API-key creation flow to detect unique-constraint violations and retry
generating a new Lookup (catch the DB unique conflict/DbUpdateException or
specific SQL unique-constraint error) until insert succeeds; update the
migration and the key-generation code path that inserts into ApiKeys to handle
and retry on unique key conflicts.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server.core/Migrations/20260303172131_ApiKey.Designer.csserver.core/Migrations/20260303172131_ApiKey.csserver.core/Migrations/AppDbContextModelSnapshot.cs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/Controllers/WeatherForecastController.cs`:
- Around line 21-23: The Get method in WeatherForecastController currently
returns Task<IEnumerable<WeatherForecast>>; change its signature to
Task<IActionResult> and update the method to return an explicit ActionResult
(e.g., return Ok(forecasts)) so the controller follows API conventions; ensure
the method remains async and that you return appropriate IActionResult variants
on success or error from within Get.
Enables API keys to be provided via the `X-Api-Key` header, offering a more convenient authentication method, especially for direct API calls and tools like Swagger. Enhances the API key authentication handler to populate the user's principal with full claims, including display name, email, and roles. This ensures API key-authenticated users have a consistent set of identity claims as cookie-authenticated users. Integrates `X-Api-Key` into Swagger UI for improved API documentation and testing. Client-side documentation is also updated to reflect this new option.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/Program.cs (1)
80-93: Scope Swagger security requirements to only secured operations.This global
AddSecurityRequirementmakes every endpoint appear to requireX-Api-Key, which can misrepresent cookie-authenticated or anonymous endpoints in OpenAPI clients/UI. Prefer applying it conditionally (e.g., based on[Authorize]) or documenting both schemes per operation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/Program.cs` around lines 80 - 93, The global AddSecurityRequirement call is making every operation require the ApiKey scheme; remove or stop using the global OpenApiSecurityRequirement and instead register an IOperationFilter (e.g., implement and register an AuthorizeCheckOperationFilter) that inspects action/controller attributes like [Authorize] and only adds the OpenApiSecurityRequirement referencing the ApiKey OpenApiSecurityScheme for secured operations; ensure the filter uses the same scheme Id ("ApiKey") and adds Array.Empty<string>() as the scopes when adding the requirement so cookie-authenticated or anonymous endpoints are not marked as requiring X-Api-Key.server/Helpers/ApiKeyAuthenticationHandler.cs (1)
17-23: Please confirm nullable context is enabled project-wide.This new handler uses nullable annotations and suppression patterns; ensure they’re enforced by compiler nullability (
<Nullable>enable</Nullable>or file-level enable) so the safety checks are effective.As per coding guidelines "Enable nullable reference types in C# backend code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/Helpers/ApiKeyAuthenticationHandler.cs` around lines 17 - 23, The project must enable nullable reference types so the nullable annotations/suppressions in ApiKeyAuthenticationHandler are enforced; either add <Nullable>enable</Nullable> to the project file(s) (project-wide) or put " `#nullable` enable" at the top of ApiKeyAuthenticationHandler.cs; ensure the compiler setting is applied to the project that contains the ApiKeyAuthenticationHandler class and rerun the build to verify no nullable warnings are suppressed silently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/Helpers/AuthenticationHelper.cs`:
- Around line 38-49: The current header check uses
ctx.Request.Headers.ContainsKey(ApiKeyAuthenticationHandler.CustomHeaderName)
which treats present-but-empty headers as API-key attempts; change the logic in
the authentication selection to use
ctx.Request.Headers.TryGetValue(ApiKeyAuthenticationHandler.CustomHeaderName,
out var apiKeyValues) and validate that apiKeyValues is not empty and the first
value is non-null/non-whitespace (e.g., Trim and check Length > 0) before
returning ApiKeyAuthenticationHandler.SchemeName; keep the existing
auth.StartsWith(ApiKeyAuthenticationHandler.HeaderPrefix,
StringComparison.OrdinalIgnoreCase) check for the Authorization header and
otherwise fall back to CookieAuthenticationDefaults.AuthenticationScheme.
---
Nitpick comments:
In `@server/Helpers/ApiKeyAuthenticationHandler.cs`:
- Around line 17-23: The project must enable nullable reference types so the
nullable annotations/suppressions in ApiKeyAuthenticationHandler are enforced;
either add <Nullable>enable</Nullable> to the project file(s) (project-wide) or
put " `#nullable` enable" at the top of ApiKeyAuthenticationHandler.cs; ensure the
compiler setting is applied to the project that contains the
ApiKeyAuthenticationHandler class and rerun the build to verify no nullable
warnings are suppressed silently.
In `@server/Program.cs`:
- Around line 80-93: The global AddSecurityRequirement call is making every
operation require the ApiKey scheme; remove or stop using the global
OpenApiSecurityRequirement and instead register an IOperationFilter (e.g.,
implement and register an AuthorizeCheckOperationFilter) that inspects
action/controller attributes like [Authorize] and only adds the
OpenApiSecurityRequirement referencing the ApiKey OpenApiSecurityScheme for
secured operations; ensure the filter uses the same scheme Id ("ApiKey") and
adds Array.Empty<string>() as the scopes when adding the requirement so
cookie-authenticated or anonymous endpoints are not marked as requiring
X-Api-Key.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
client/src/routes/(authenticated)/settings/index.tsxserver/Helpers/ApiKeyAuthenticationHandler.csserver/Helpers/AuthenticationHelper.csserver/Program.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- client/src/routes/(authenticated)/settings/index.tsx
Adds a new `/Upload/DirectUpload` endpoint to simplify API-key based file uploads. Clients can now POST PDF files directly via `multipart/form-data`. The server handles streaming the file to Azure Blob Storage, abstracting away the need for clients to manage SAS URLs. Introduces `IIncomingBlobUploadService` to encapsulate the blob storage upload logic, alongside validation, database record creation, and error handling for robust uploads.
Updates the authentication scheme selector to verify that the API key header contains a non-empty value before routing the request to the API key handler. This prevents unnecessary authentication attempts when the header is present but contains only whitespace or no value.
Adds checks to ensure the raw API key and its decoded byte representation meet expected length requirements. This prevents the processing of malformed or invalid keys before performing cryptographic operations.
Implements direct server-side file upload
Ensures the generation and persistence of new API keys are performed as an atomic operation.
Adds the `ResponseCache` attribute to the API key generation endpoint to ensure that the sensitive raw key is not stored in browser or intermediate caches.
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
1 participant
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.
Enables authenticated users to generate, view, and revoke API keys for programmatic access.
Provides a dedicated settings page in the client for users to manage their keys securely, including displaying a key hint and creation date, and handling one-time display of newly generated raw keys.
Implements secure server-side handling of API keys, utilizing PBKDF2 for hashing and HMAC for efficient, constant-time lookup. A new authentication scheme is configured to accept API keys via the
Authorizationheader, seamlessly integrating with existing session-based authentication.Summary by CodeRabbit
New Features
Chores