Skip to content

Notification: fix build path, root-route through gateway, add JWT auth #53

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
devin-ai-integration wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Notification: fix build path, root-route through gateway, add JWT auth #53

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions src/Services/Notification/Notification.API/Controllers/NotificationController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Mvc;
using Notification.API.Services;
using Notification.Domain.Interfaces;
Expand All @@ -6,7 +7,6 @@
namespace Notification.API.Controllers;

[ApiController]
[Route("api/[controller]")]
public class NotificationController : ControllerBase
{
private readonly INotificationRepository _repository;
Expand All @@ -23,7 +23,8 @@ public NotificationController(
_logger = logger;
}

[HttpGet]
[Authorize]
[HttpGet("/")]
public async Task<IActionResult> GetAll([FromQuery] int page = 1, [FromQuery] int pageSize = 20)
{
var notifications = await _repository.GetAllAsync(page, pageSize);
Expand All @@ -41,7 +42,8 @@ public async Task<IActionResult> GetAll([FromQuery] int page = 1, [FromQuery] in
}));
}

[HttpGet("{id:guid}")]
[Authorize]
[HttpGet("/{id:guid}")]
public async Task<IActionResult> GetById(Guid id)
{
var notification = await _repository.GetByIdAsync(id);
Expand All @@ -68,7 +70,8 @@ public async Task<IActionResult> GetById(Guid id)
/// Returns the rendered HTML email preview for a notification.
/// Use this endpoint to visually inspect notification output.
/// </summary>
[HttpGet("{id:guid}/preview")]
[Authorize]
[HttpGet("/{id:guid}/preview")]
[Produces("text/html")]
public async Task<IActionResult> GetPreview(Guid id)
{
Expand All @@ -87,7 +90,7 @@ public async Task<IActionResult> GetPreview(Guid id)
/// In production this would be a RabbitMQ consumer; this endpoint
/// enables local testing without a message broker.
/// </summary>
[HttpPost("events/order-placed")]
[HttpPost("/events/order-placed")]
public async Task<IActionResult> ReceiveOrderPlacedEvent([FromBody] OrderPlacedEventDto dto)
Comment on lines +93 to 94

@devin-ai-integration devin-ai-integration Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@devin-ai-integration devin-ai-integration Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
var orderEvent = new OrderPlacedEvent(
Expand All @@ -101,7 +104,7 @@ public async Task<IActionResult> ReceiveOrderPlacedEvent([FromBody] OrderPlacedE
return CreatedAtAction(
nameof(GetPreview),
new { id = notification.Id },
new { notification.Id, PreviewUrl = $"/api/notification/{notification.Id}/preview" });
new { notification.Id, PreviewUrl = $"/{notification.Id}/preview" });
}
Comment on lines 104 to 108

@devin-ai-integration devin-ai-integration Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 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.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@devin-ai-integration devin-ai-integration Bot Jun 26, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

Expand Down
5 changes: 3 additions & 2 deletions src/Services/Notification/Notification.API/Notification.API.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,11 @@
<ItemGroup>
<ProjectReference Include="..\Notification.Domain\Notification.Domain.csproj" />
<ProjectReference Include="..\Notification.Infrastructure\Notification.Infrastructure.csproj" />
<ProjectReference Include="..\..\Shared\Shared.Contracts\Shared.Contracts.csproj" />
<ProjectReference Include="..\..\Shared\Shared.Infrastructure\Shared.Infrastructure.csproj" />
<ProjectReference Include="..\..\..\Shared\Shared.Contracts\Shared.Contracts.csproj" />
<ProjectReference Include="..\..\..\Shared\Shared.Infrastructure\Shared.Infrastructure.csproj" />
</ItemGroup>
<ItemGroup>
<PackageReference Include="Microsoft.AspNetCore.Authentication.JwtBearer" Version="10.*" />
<PackageReference Include="Microsoft.EntityFrameworkCore.Design" Version="10.*" />
<PackageReference Include="Swashbuckle.AspNetCore" Version="7.*" />
</ItemGroup>
Expand Down
28 changes: 28 additions & 0 deletions src/Services/Notification/Notification.API/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@
using Notification.Domain.Interfaces;
using Notification.Infrastructure.Data;
using Notification.Infrastructure.Repositories;
using Microsoft.AspNetCore.Authentication.JwtBearer;
using Microsoft.EntityFrameworkCore;
using Microsoft.IdentityModel.Tokens;
using System.Text;

var builder = WebApplication.CreateBuilder(args);

Expand All @@ -11,6 +14,28 @@
builder.Services.AddSwaggerGen();
builder.Services.AddHealthChecks();

var jwtSection = builder.Configuration.GetSection("Jwt");
builder.Services
.AddAuthentication(JwtBearerDefaults.AuthenticationScheme)
.AddJwtBearer(options =>
{
options.MapInboundClaims = false;
options.TokenValidationParameters = new TokenValidationParameters
{
ValidateIssuer = true,
ValidateAudience = true,
ValidateLifetime = true,
ValidateIssuerSigningKey = true,
ValidIssuer = jwtSection["Issuer"],
ValidAudience = jwtSection["Audience"],
IssuerSigningKey = new SymmetricSecurityKey(
Encoding.UTF8.GetBytes(jwtSection["Key"]!)),
NameClaimType = "name",
RoleClaimType = "role"
};
});
builder.Services.AddAuthorization();

builder.Services.AddDbContext<NotificationDbContext>(options =>
options.UseNpgsql(builder.Configuration.GetConnectionString("DefaultConnection")));

Expand All @@ -32,6 +57,9 @@
app.UseSwaggerUI();
}

app.UseAuthentication();
app.UseAuthorization();

app.MapControllers();
app.MapHealthChecks("/healthz");

Expand Down
5 changes: 5 additions & 0 deletions src/Services/Notification/Notification.API/appsettings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,10 @@
},
"ConnectionStrings": {
"DefaultConnection": "Host=localhost;Database=notificationdb;Username=postgres;Password=postgres"
},
"Jwt": {
"Key": "quickapp-dev-only-signing-key-change-me-please-32+chars",
"Issuer": "quickapp-identity",
"Audience": "quickapp"
}
}
42 changes: 42 additions & 0 deletions src/Services/Notification/e2e-smoke.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
#!/usr/bin/env bash
#
# E2E smoke test for the Notification service through the API gateway.
#
# Verifies (through http://localhost:5000, the gateway, which strips the
# /api/notifications prefix and forwards to notification-service:5005):
# 1. GET /api/notifications/healthz -> 200 (service reachable & healthy)
# 2. GET /api/notifications (no JWT) -> 401 (read endpoints are protected)
#
# Exits non-zero on any failure.
#
# Usage:
# src/Services/Notification/e2e-smoke.sh # uses default GATEWAY_URL
# GATEWAY_URL=http://localhost:5000 ./e2e-smoke.sh
#
set -euo pipefail

GATEWAY_URL="${GATEWAY_URL:-http://localhost:5000}"
fail=0

check() {
local desc="$1" expected="$2" url="$3"
local actual
actual="$(curl -s -o /dev/null -w '%{http_code}' "$url" || echo "000")"
if [[ "$actual" == "$expected" ]]; then
echo "PASS: $desc ($url -> $actual)"
else
echo "FAIL: $desc ($url -> expected $expected, got $actual)"
fail=1
fi
}

echo "== Notification E2E smoke (gateway: $GATEWAY_URL) =="
check "health endpoint returns 200" 200 "$GATEWAY_URL/api/notifications/healthz"
check "list endpoint without token returns 401" 401 "$GATEWAY_URL/api/notifications"

if [[ "$fail" -ne 0 ]]; then
echo "SMOKE TEST FAILED"
exit 1
fi

echo "SMOKE TEST PASSED"