Skip to content

fix(OPENFRAM-007-3): CU-86akn96pf NotificationDataFetcher re-implements principal-resolution logic that CurrentPrincipalSupport already centralizes #2425

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

Draft
flamingo wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix(OPENFRAM-007-3): CU-86akn96pf NotificationDataFetcher re-implements principal-resolution logic that CurrentPrincipalSupport already centralizes #2425

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
58 changes: 24 additions & 34 deletions ...api-service-core/src/main/java/com/openframe/api/datafetcher/NotificationDataFetcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,7 @@
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.security.access.prepost.PreAuthorize;
import org.springframework.security.core.Authentication;
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.security.oauth2.server.resource.authentication.JwtAuthenticationToken;
import org.springframework.security.core.annotation.AuthenticationPrincipal;

import java.util.List;
import java.util.Map;
Expand All @@ -43,6 +41,7 @@ public class NotificationDataFetcher {
private final NotificationService notificationService;
private final NotificationReadStateService readStateService;
private final GraphQLNotificationMapper notificationMapper;
private final CurrentPrincipalSupport currentPrincipalSupport;

@DgsData(parentType = "Notification", field = "id")
public String notificationNodeId(DgsDataFetchingEnvironment dfe) {
Expand All @@ -59,9 +58,10 @@ public GenericConnection<GenericEdge<NotificationView>> notifications(
@InputArgument String after,
@InputArgument Integer last,
@InputArgument String before,
@InputArgument SortInput sort) {
@InputArgument SortInput sort,
@AuthenticationPrincipal AuthPrincipal principal) {

Recipient r = currentRecipient();
Recipient r = currentRecipient(principal);
log.debug("Listing notifications for {} {} (filter={}, search={}, sort={})",
r.type(), r.id(), filter, search, sort);

Expand All @@ -79,15 +79,15 @@ public GenericConnection<GenericEdge<NotificationView>> notifications(

@PreAuthorize("hasAnyAuthority('ADMIN', 'AGENT')")
@DgsQuery
public boolean hasUnreadNotifications() {
Recipient r = currentRecipient();
public boolean hasUnreadNotifications(@AuthenticationPrincipal AuthPrincipal principal) {
Recipient r = currentRecipient(principal);
return readStateService.hasUnread(r.id(), r.type());
}

@PreAuthorize("hasAnyAuthority('ADMIN', 'AGENT')")
@DgsQuery
public List<UnreadCategoryCount> unreadCountsByCategory() {
Recipient r = currentRecipient();
public List<UnreadCategoryCount> unreadCountsByCategory(@AuthenticationPrincipal AuthPrincipal principal) {
Recipient r = currentRecipient(principal);
String recipientId = r.id();
RecipientType recipientType = r.type();
Map<NotificationCategory, Long> counts = readStateService.unreadCountsByCategory(recipientId, recipientType);
Expand All @@ -97,54 +97,53 @@ public List<UnreadCategoryCount> unreadCountsByCategory() {
@PreAuthorize("hasAnyAuthority('ADMIN', 'AGENT')")
@DgsMutation
public long markNotificationsReadForEntity(@InputArgument NotificationEntityType entityType,
@InputArgument String entityId) {
Recipient r = currentRecipient();
@InputArgument String entityId,
@AuthenticationPrincipal AuthPrincipal principal) {
Recipient r = currentRecipient(principal);
return readStateService.markEntityAsRead(r.id(), r.type(), entityType, entityId);
}

@PreAuthorize("hasAnyAuthority('ADMIN', 'AGENT')")
@DgsMutation
public boolean markNotificationAsRead(@InputArgument String notificationId) {
Recipient r = currentRecipient();
public boolean markNotificationAsRead(@InputArgument String notificationId,
@AuthenticationPrincipal AuthPrincipal principal) {
Recipient r = currentRecipient(principal);
return readStateService.markRead(r.id(), r.type(), decodeNotificationId(notificationId));
}

@PreAuthorize("hasAnyAuthority('ADMIN', 'AGENT')")
@DgsMutation
public long markAllNotificationsAsRead() {
Recipient r = currentRecipient();
public long markAllNotificationsAsRead(@AuthenticationPrincipal AuthPrincipal principal) {
Recipient r = currentRecipient(principal);
return readStateService.markAllAsRead(r.id(), r.type());
}

@PreAuthorize("hasAnyAuthority('ADMIN', 'AGENT')")
@DgsMutation
public boolean deleteNotification(@InputArgument String notificationId) {
Recipient r = currentRecipient();
public boolean deleteNotification(@InputArgument String notificationId,
@AuthenticationPrincipal AuthPrincipal principal) {
Recipient r = currentRecipient(principal);
return readStateService.deleteNotification(r.id(), r.type(), decodeNotificationId(notificationId));
}

@PreAuthorize("hasAnyAuthority('ADMIN', 'AGENT')")
@DgsMutation
public long deleteAllReadNotifications() {
Recipient r = currentRecipient();
public long deleteAllReadNotifications(@AuthenticationPrincipal AuthPrincipal principal) {
Recipient r = currentRecipient(principal);
return readStateService.deleteAllRead(r.id(), r.type());
}

private record Recipient(String id, RecipientType type) {}

private Recipient currentRecipient() {

@flamingo flamingo Bot Sep 28, 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.

🦩 🟠 NotificationDataFetcher re-implements principal-resolution logic that CurrentPrincipalSupport already centralizes

In NotificationDataFetcher, removed the private currentPrincipal()/currentRecipient() JWT-extraction methods and replaced principal acquisition with @AuthenticationPrincipal AuthPrincipal parameters on every query/mutation (matching the pattern used by PresenceDataFetcher/NotificationSettingsDataFetcher), and delegated human-user resolution to an injected CurrentPrincipalSupport.requireHumanUserId(principal) call in the new currentRecipient(AuthPrincipal) helper, keeping only the AGENT/machine_id branch local since that path is specific to this fetcher. This is UNVERIFIED against the actual CurrentPrincipalSupport class signature (I could not see its source, so the method name/parameter shape requireHumanUserId(AuthPrincipal) is inferred from the finding text referencing "CurrentPrincipalSupport.requireHumanUserId"); if its actual signature differs (e.g., it takes no argument and resolves from SecurityContextHolder itself, or throws a different exception, or the AGENT branch should also be delegated to it), this will fail to compile and needs adjustment to match the real API. A complete fix requires confirming CurrentPrincipalSupport's exact method signatures from PresenceDataFetcher/NotificationSettingsDataFetcher and aligning this file's calls and constructor injection accordingly.

🤖 Prompt for AI agents 
In openframe-api-service-core/src/main/java/com/openframe/api/datafetcher/NotificationDataFetcher.java around line 135, review and complete this code-review fix: NotificationDataFetcher re-implements principal-resolution logic that CurrentPrincipalSupport already centralizes.
What the draft fix changed: In `NotificationDataFetcher`, removed the private `currentPrincipal()`/`currentRecipient()` JWT-extraction methods and replaced principal acquisition with `@AuthenticationPrincipal AuthPrincipal` parameters on every query/mutation (matching the pattern used by `PresenceDataFetcher`/`NotificationSettingsDataFetcher`), and delegated human-user resolution to an injected `CurrentPrincipalSupport.requireHumanUserId(principal)` call in the new `currentRecipient(AuthPrincipal)` helper, keeping only the AGENT/machine_id branch local since that path is specific to this fetcher. This is UNVERIFIED against the actual `CurrentPrincipalSupport` class signature (I could not see its source, so the method name/parameter shape `requireHumanUserId(AuthPrincipal)` is inferred from the finding text referencing "CurrentPrincipalSupport.requireHumanUserId"); if its actual signature differs (e.g., it takes no argument and resolves from `SecurityContextHolder` itself, or throws a different exception, or the AGENT branch should also be delegated to it), this will fail to compile and needs adjustment to match the real API. A complete fix requires confirming `CurrentPrincipalSupport`'s exact method signatures from `PresenceDataFetcher`/`NotificationSettingsDataFetcher` and aligning this file's calls and constructor injection accordingly.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 35 low — review closely — react 👍/👎 to teach the reviewer

AuthPrincipal principal = currentPrincipal();
private Recipient currentRecipient(AuthPrincipal principal) {
if (principal.getActorType() == ActorType.AGENT) {
String machineId = principal.getMachineId();
if (isBlank(machineId)) {
throw new UnauthorizedException("AGENT principal missing machine_id claim");
}
return new Recipient(machineId, RecipientType.MACHINE);
}
String userId = principal.getId();
if (isBlank(userId)) {
throw new UnauthorizedException("Authenticated user is required to access notifications");
}
String userId = currentPrincipalSupport.requireHumanUserId(principal);
return new Recipient(userId, RecipientType.USER);
}

Expand All @@ -164,13 +163,4 @@ private String decodeNotificationId(String input) {
}
return resolved.getId();
}

private AuthPrincipal currentPrincipal() {
Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
if (authentication instanceof JwtAuthenticationToken jwtAuth) {
return AuthPrincipal.fromJwt(jwtAuth.getToken());
}
throw new UnauthorizedException("Notifications require a JWT-authenticated principal; got " +
(authentication == null ? "no authentication" : authentication.getClass().getSimpleName()));
}
}