Add Health Check #167
Add Health Check #167
Conversation
WalkthroughRefactors authentication to use ASP.NET Core authorization with a fallback policy requiring authenticated users, removes custom inline 401 middleware, adds an anonymous GET /health endpoint returning Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Pipeline as ASP.NET Core Pipeline
participant AuthZ as AuthorizationMiddleware
participant Policy as FallbackPolicy (RequireAuthenticated)
participant Health as /health endpoint
participant App as Application Endpoint
Client->>Pipeline: HTTP request /health
Pipeline->>AuthZ: Authenticate & Authorize
Note right of AuthZ: FallbackPolicy allows anonymous for /health
AuthZ-->>Health: Forward to endpoint
Health-->>Client: 200 { "status":"Healthy" }
Client->>Pipeline: HTTP request /breefs
Pipeline->>AuthZ: Authenticate & Authorize
Note right of Policy: FallbackPolicy requires authenticated user
alt Authenticated
AuthZ-->>App: Forward to breefs handler
App-->>Client: 200/201 ...
else Unauthenticated
AuthZ-->>Client: 401 with WWW-Authenticate: ApiKey param="BREEF-API-KEY"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/Elzik.Breef.Api/Program.cs (1)
138-139: Consider using PascalCase for consistency.The anonymous object uses lowercase
status, while the test'sHealthResponsemodel uses PascalCaseStatus. Although ASP.NET Core's JSON deserialization is case-insensitive by default and the test works, using PascalCase would align better with C# naming conventions.Apply this diff for consistency:
- app.MapGet("/health", () => Results.Ok(new { status = "Healthy" })) + app.MapGet("/health", () => Results.Ok(new { Status = "Healthy" }))src/Elzik.Breef.Api/Elzik.Breef.Api.http (1)
12-15: Optional: Remove unnecessary Content-Type header.The
Content-Type: application/jsonheader is unnecessary for a GET request without a request body. While harmless, removing it would make the request cleaner.Apply this diff:
### Health Get {{Elzik.Breef.Api_HostAddress}}/health -Content-Type: application/jsontests/Elzik.Breef.Api.Tests.Functional/HealthEndpointTests.cs (1)
8-8: Consider adding a using directive for improved readability.The fully qualified name
Microsoft.AspNetCore.Mvc.Testing.WebApplicationFactory<Elzik.Breef.Api.Program>makes the class declaration lengthy. Adding a using directive at the top would improve readability.Add this using directive at the top of the file:
using Shouldly; using System.Net; using System.Net.Http.Json; using Xunit; +using Microsoft.AspNetCore.Mvc.Testing;Then update the class declaration:
- public class HealthEndpointTests : IClassFixture<Microsoft.AspNetCore.Mvc.Testing.WebApplicationFactory<Elzik.Breef.Api.Program>> + public class HealthEndpointTests : IClassFixture<WebApplicationFactory<Elzik.Breef.Api.Program>>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Elzik.Breef.Api/Auth/AuthExtensions.cs(2 hunks)src/Elzik.Breef.Api/Elzik.Breef.Api.http(1 hunks)src/Elzik.Breef.Api/Program.cs(1 hunks)tests/Elzik.Breef.Api.Tests.Functional/BreefTestsBase.cs(1 hunks)tests/Elzik.Breef.Api.Tests.Functional/BreefTestsDocker.cs(1 hunks)tests/Elzik.Breef.Api.Tests.Functional/HealthEndpointTests.cs(1 hunks)tests/Elzik.Breef.Infrastructure.Tests.Unit/ContentExtractors/Reddit/SubRedditExtractorTests.cs(0 hunks)
💤 Files with no reviewable changes (1)
- tests/Elzik.Breef.Infrastructure.Tests.Unit/ContentExtractors/Reddit/SubRedditExtractorTests.cs
🧰 Additional context used
🧬 Code graph analysis (1)
tests/Elzik.Breef.Api.Tests.Functional/HealthEndpointTests.cs (1)
src/Elzik.Breef.Api/Program.cs (2)
Program(19-159)Program(21-24)
⏰ 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). (3)
- GitHub Check: build-ubuntu
- GitHub Check: build-ubuntu
- GitHub Check: Analyze (csharp)
🔇 Additional comments (4)
tests/Elzik.Breef.Api.Tests.Functional/BreefTestsDocker.cs (1)
83-86: LGTM! Improved test reliability.Switching to an HTTP health check wait strategy ensures the container is fully ready before tests execute, reducing flakiness.
tests/Elzik.Breef.Api.Tests.Functional/BreefTestsBase.cs (1)
76-82: LGTM! Test correctly validates framework authentication.The test now properly validates the WWW-Authenticate challenge header and empty response body, aligning with the refactored authorization flow.
src/Elzik.Breef.Api/Auth/AuthExtensions.cs (1)
16-22: LGTM! Solid authorization pattern.The fallback policy with
RequireAuthenticatedUser()ensures secure-by-default behavior, where all endpoints require authentication unless explicitly marked with.AllowAnonymous(). This is a cleaner approach than custom middleware.tests/Elzik.Breef.Api.Tests.Functional/HealthEndpointTests.cs (1)
17-28: LGTM! Well-structured health endpoint test.The test properly validates the health endpoint's response status, deserialization, and content. Using
WebApplicationFactoryfor in-memory testing is the right approach.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Elzik.Breef.Api/Auth/AuthExtensions.cs (1)
17-23: Simplify authorization configuration by inlining the policy.The "RequireAuthenticated" policy is not referenced anywhere else in the codebase, so it can be inlined directly into the fallback configuration rather than created as a named policy. This eliminates unnecessary indirection and simplifies the code.
- var authBuilder = services.AddAuthorizationBuilder(); - authBuilder.AddPolicy("RequireAuthenticated", p => p.RequireAuthenticatedUser()); - - services.Configure<AuthorizationOptions>(options => - { - options.FallbackPolicy = options.GetPolicy("RequireAuthenticated"); - }); + services.AddAuthorization(options => + { + options.FallbackPolicy = new AuthorizationPolicyBuilder() + .RequireAuthenticatedUser() + .Build(); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Elzik.Breef.Api/Auth/AuthExtensions.cs(2 hunks)
🔇 Additional comments (1)
src/Elzik.Breef.Api/Auth/AuthExtensions.cs (1)
2-2: LGTM!The namespace import is necessary for the authorization configuration below.
|
This pull request introduces several improvements to authentication, authorization, health checking, and test coverage in the API. The most significant changes include enforcing a fallback authorization policy, adding a
/healthendpoint, updating tests to reflect new authentication behavior, and improving Docker test reliability. These updates help standardize authentication flows, provide a health check for deployments, and ensure tests accurately validate the API's behavior.Authentication & Authorization Enhancements:
RequireAuthenticatedUser, replacing the previous default policy and custom middleware for handling unauthorized requests. This ensures all endpoints require authentication unless explicitly marked as anonymous, and responses to unauthorized requests now follow standard API key authentication challenge behavior. (AuthExtensions.cs)AuthExtensions.cs)Health Check Endpoint:
/healthendpoint was added to the API, returning a simple JSON object indicating the service status. This endpoint is accessible without authentication and is useful for deployment and monitoring purposes. (Program.cs,Elzik.Breef.Api.http) [1] [2]Test Improvements:
BreefTestsBase.cs)/healthendpoint returns the expected status and body. (HealthEndpointTests.cs)/healthendpoint to succeed before proceeding, improving reliability of integration tests. (BreefTestsDocker.cs)Test Suite Maintenance:
SubRedditExtractorTests.cs)Summary by CodeRabbit
New Features
Improvements
Tests
Bug Fixes