Notification: fix build path, root-route through gateway, add JWT auth#53
Notification: fix build path, root-route through gateway, add JWT auth#53devin-ai-integration[bot] wants to merge 1 commit into
Conversation
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| [HttpPost("/events/order-placed")] | ||
| public async Task<IActionResult> ReceiveOrderPlacedEvent([FromBody] OrderPlacedEventDto dto) |
There was a problem hiding this comment.
🚩 Event ingestion endpoint is exposed through the gateway without authentication
The ReceiveOrderPlacedEvent POST endpoint at NotificationController.cs:93 does not have [Authorize], while all GET endpoints do. The comment explains this endpoint simulates a RabbitMQ consumer for local testing. However, the YARP gateway's catch-all route (/api/notifications/{**catch-all} at ApiGateway/appsettings.json:33) will forward POST /api/notifications/events/order-placed to this endpoint, making it publicly reachable without any token. Anyone who can reach the gateway can create notifications with arbitrary data. If this is acceptable for the current development phase, consider adding a TODO or documenting the intent to protect or remove this endpoint before production.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Intentional for this phase. The POST /events/order-placed endpoint simulates an internal message-broker (RabbitMQ) event — in production it'd be a broker consumer, not a gateway-exposed route. The task spec explicitly allows leaving intake unauthenticated; the security gate only requires the read/query endpoints (list/get/preview) to be [Authorize] (401 without a token), which they are. Locking the intake down (or removing the gateway-exposed shim once a real broker is wired) is a follow-up, kept out of scope here to avoid regressing the local-testing flow.
| return CreatedAtAction( | ||
| nameof(GetPreview), | ||
| new { id = notification.Id }, | ||
| new { notification.Id, PreviewUrl = $"/api/notification/{notification.Id}/preview" }); | ||
| new { notification.Id, PreviewUrl = $"/{notification.Id}/preview" }); | ||
| } |
There was a problem hiding this comment.
🚩 CreatedAtAction Location header will not be routable through the gateway
The CreatedAtAction at NotificationController.cs:104-107 generates a Location header based on the GetPreview action's route /{id:guid}/preview. When accessed through the YARP gateway, the backend generates a URL like http://notification-service:5005/{guid}/preview. Without UseForwardedHeaders() middleware and corresponding YARP X-Forwarded-* header configuration, this Location header points to an internal host the client can't reach. The hardcoded PreviewUrl (/{id}/preview) has the same issue — it lacks the /api/notifications gateway prefix. Note: the pre-PR code had a similar issue (it used /api/notification/ singular, not matching the gateway's /api/notifications/ plural). This is an infrastructure concern rather than a code-level bug, but worth addressing when the service is deployed behind the gateway.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Acknowledged — this is a pre-existing gateway-prefix/forwarded-headers concern, not a regression from this PR. The old value was /api/notification/{id}/preview (singular, already not matching the gateway's plural /api/notifications); I only updated it to the new service-root path /{id}/preview to stay consistent with the root-routing change. Making the Location/PreviewUrl fully gateway-routable requires UseForwardedHeaders() + YARP X-Forwarded-* config (an infra change touching the gateway), which is outside this PR's scope of "scope changes to Notification only" and "keep response shapes unchanged." Leaving as a follow-up.
1 participant
Summary
Finishes the Notification microservice carve-out: fixes its broken build, makes it reachable & protected through the gateway, and adds an E2E smoke test. Scoped to
src/Services/Notification/**— no monolith, gateway, or compose changes. Existing order-confirmation rendering behavior and response shapes are unchanged.1. Build fix (Shared project path)
Notification.API.csprojreferenced the Shared projects two levels up (..\..\Shared\...), which resolves to the non-existentsrc/Services/Shared. The service lives one level deeper (src/Services/Notification/Notification.API), so the correct path is three levels up:The Dockerfile COPY lines were already correct (paths relative to the
srcbuild context:Shared/Shared.Contracts/...), so no Dockerfile change was needed.Verified:
cd src && dotnet build Services/Notification/Notification.API/Notification.API.csproj -c Release→ Build succeeded.2. Root-routing through the gateway
The gateway strips
/api/notificationsbefore forwarding tonotification-service:5005. The controller used[Route("api/[controller]")](=/api/notification), so external/api/notifications/*never reached the actions. Removed the controller-level route and pinned each action to an absolute service-root route.External (via gateway) → service path:
GET /api/notificationsGET /[Authorize]GET /api/notifications/{id}GET /{id:guid}[Authorize]GET /api/notifications/{id}/previewGET /{id:guid}/preview[Authorize]POST /api/notifications/events/order-placedPOST /events/order-placedGET /api/notifications/healthzGET /healthzThe intake response
previewUrlwas updated from/api/notification/{id}/previewto the service-root/{id}/previewto match.3. JWT auth (canonical contract from Identity PR #48)
Microsoft.AspNetCore.Authentication.JwtBearer(10.*).Jwtsection toappsettings.json(Key/Issuer=quickapp-identity/Audience=quickapp; dev-only signing key).AddAuthentication().AddJwtBearerwithTokenValidationParametersvalidating issuer/audience/lifetime/signing-key (HS256SymmetricSecurityKeyover UTF8 ofJwt:Key),MapInboundClaims=false,NameClaimType="name",RoleClaimType="role".app.UseAuthentication(); app.UseAuthorization();beforeMapControllers().[Authorize]; thePOST /events/order-placedintake (simulated internal broker event) is left unauthenticated.4. E2E smoke test
Added
src/Services/Notification/e2e-smoke.sh(exits non-zero on failure) asserting through the gateway:GET /api/notifications/healthz→ 200GET /api/notificationswithout a token → 401Validation
Brought up
postgres,notification-service,api-gatewayviadocker composeand ran the smoke test throughhttp://localhost:5000:GET /api/notifications/healthz→ 200GET /api/notifications(no token) → 401GET /api/notifications(valid HS256 token) → 200POST /api/notifications/events/order-placed→ 201GET /api/notifications/{id}/preview(no token) → 401; (valid token) → 200 with the unchanged rendered HTML emailLink to Devin session: https://partner-workshops.devinenterprise.com/sessions/f32b8fe62be54776b075130b3644b409
Requested by: @mbatchelor81