Conversation
Add comprehensive avatar upload/download/delete REST APIs and GraphQL mutations for user preferences, including caching with Redis. Includes troubleshooting docs for common issues like upload failures and cache invalidation. Enhances user settings with operations for API keys, password changes, and account status, driven by demand for personalized user experiences.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. WalkthroughAdds a complete user settings feature: DB migrations and new entities for avatars, preferences, API keys and deactivation audit; backend REST/GraphQL endpoints and reactive services (avatar storage, API-key lifecycle, preferences caching, ETag caching); frontend settings routes, components and service; CORS/config updates and documentation expansion. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User (Browser)
participant UI as Settings UI
participant FSvc as Frontend SettingsService
participant Apollo as Apollo Client
participant Backend as Backend (GraphQL/REST)
participant DB as Database
participant Redis as Redis (preferences/ETag)
participant MinIO as MinIO
rect rgb(200,220,255)
U->>UI: Update preferences
UI->>FSvc: updateUserPreferences(prefs)
FSvc->>Apollo: GraphQL mutation
Apollo->>Backend: updateUserPreferences
Backend->>DB: upsert user_preferences
Backend->>Redis: evict user-preferences cache
DB-->>Backend: OK
Backend-->>Apollo: UserPreferencesDto
Apollo-->>FSvc: response
FSvc-->>UI: show success
end
rect rgb(220,240,220)
U->>UI: Generate API key
UI->>FSvc: generateApiKey(name, days)
FSvc->>Apollo: GraphQL mutation
Apollo->>Backend: generateApiKey
Backend->>Backend: create rawKey/hash/preview
Backend->>DB: insert api_keys
DB-->>Backend: id
Backend-->>Apollo: CreateApiKeyResponseDto (rawKey shown once)
Apollo-->>FSvc: response
FSvc-->>UI: display raw key modal
end
rect rgb(255,240,220)
U->>UI: Upload avatar
UI->>FSvc: uploadAvatar(file) (REST)
FSvc->>Backend: POST /api/user/avatar
Backend->>MinIO: upload object
Backend->>DB: update users.avatar_key
Backend->>Redis: invalidate ETag(s)
MinIO-->>Backend: OK
DB-->>Backend: OK
Backend-->>FSvc: AvatarUploadResponseDto
FSvc-->>UI: refresh avatar
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus during review:
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (12)
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: 12
🧹 Nitpick comments (26)
src/main/java/com/rcs/ssf/controller/CacheMetricsController.java (4)
13-27: Controller wiring and actuator-style base pathThe controller wiring and constructor injection look clean and consistent with the existing
ETagMetricsControllerpattern. One thing to double‑check is that your security config treats/actuator/cache-metrics/**the way you expect (these are regular MVC endpoints, not Spring Boot Actuator@Endpointbeans), especially if you rely on actuator‑specific exposure/authorization rules for other management URLs.
29-63: Caffeine stats collection is sound; minor micro‑refactor possibleLogic for iterating caches, guarding
null, and only handling Caffeine‑backed caches is solid. You currently callcaffeineCache.stats()twice; you can reuse the samestatsinstance both for the stringified"stats"field and the individual counters:- Map<String, Object> cacheStats = new HashMap<>(); - cacheStats.put("stats", caffeineCache.stats().toString()); - cacheStats.put("estimatedSize", caffeineCache.estimatedSize()); - - // Parse stats into individual metrics - var stats = caffeineCache.stats(); + var stats = caffeineCache.stats(); + + Map<String, Object> cacheStats = new HashMap<>(); + cacheStats.put("stats", stats.toString()); + cacheStats.put("estimatedSize", caffeineCache.estimatedSize()); + + // Parse stats into individual metricsThis keeps behavior identical while avoiding the duplicate call and slightly tightening the code.
68-124: Reduce duplication in Micrometer metric extraction and verify metric names/typesThe three blocks for
cache.hits,cache.misses, andcache.putsare structurally identical. Consider extracting a small helper (or local function) that, given a metric name, returns theList<Map<String, Object>>for that name. That will make it easier to add more cache metrics later and avoid copy‑paste divergence.Also, please double‑check against your Micrometer/Spring configuration that:
cache.hits,cache.misses, andcache.putsare indeed the metric names being emitted, and- they are exposed as gauges (not counters/timers), since you are specifically calling
.gauges()here.Some Micrometer cache instrumentation setups instead emit counters with names like
cache.getsand use tags (e.g.,result=hit|miss), so a quick verification against the running app’s/actuator/metricsoutput is worthwhile.
129-171: Summary aggregation is correct; consider reusing logic and clarifying hit‑rate semanticsThe aggregation over all Caffeine caches and the per‑cache summary map look correct. Two small improvement ideas:
Reuse iteration logic:
getSummary()andgetCaffeineStats()both walkcacheManager.getCacheNames()and filter on Caffeine caches. Pulling this into a shared private helper (e.g., to yield a typed view ofcacheName+CacheStats) would reduce duplication and future drift.Overall hit‑rate representation: Right now
overallHitRateis:
"N/A (no misses)"whentotalMisses == 0, even if there have been many hits, and- a
"%.2f%%"string otherwise.If consumers expect a numeric value, you might prefer always returning a percentage (and maybe a separate
"note"field), or distinguishing the “no data at all” case(totalHits + totalMisses == 0)from the “100% hit rate” case(totalMisses == 0 && totalHits > 0).These are mostly semantics/maintainability tweaks; current behavior is functionally fine.
db/migration/V501__api_keys_add_preview.sql (1)
1-5: Consider VARCHAR2(20) size constraint for future flexibility.The 20-character limit for
key_previewmay be restrictive if preview formats evolve (e.g., longer prefixes, different masking patterns). Consider whether a slightly larger size like VARCHAR2(50) would provide better future-proofing without significant storage overhead.src/main/java/com/rcs/ssf/dto/CreateApiKeyRequestDto.java (1)
19-29: Consider reviewing the 10-year maximum expiry for security.The
MAX_EXPIRY_DAYS = 3650(approximately 10 years) is quite long for API keys from a security perspective. Industry best practices often recommend shorter maximum lifetimes (e.g., 1-2 years) to limit exposure from compromised keys and encourage regular rotation.Consider reducing the maximum to a more security-conscious value:
private static final long DEFAULT_EXPIRY_DAYS = 90; private static final long MIN_EXPIRY_DAYS = 1; - private static final long MAX_EXPIRY_DAYS = 3650; // ~10 years + private static final long MAX_EXPIRY_DAYS = 730; // ~2 years @NotBlank(message = "Key name must be provided") @Size(min = 1, max = 100, message = "Key name must be between 1 and 100 characters") private String keyName; @Min(value = MIN_EXPIRY_DAYS, message = "Expiration days must be at least 1") - @Max(value = MAX_EXPIRY_DAYS, message = "Expiration days cannot exceed 3650 days (~10 years)") + @Max(value = MAX_EXPIRY_DAYS, message = "Expiration days cannot exceed 730 days (~2 years)") private Long expiresInDays; // Optional, defaults to 90 days if nullfrontend/src/app/features/settings/settings.component.ts (1)
93-98: Consider more robust URL matching for redirect logic.The exact string comparison
this.router.url === '/settings'won't handle trailing slashes (e.g.,/settings/) or query parameters (e.g.,/settings?tab=profile). Consider using a more flexible approach:ngOnInit(): void { // Navigate to profile by default - if (this.router.url === '/settings') { + if (this.router.url === '/settings' || this.router.url === '/settings/') { this.router.navigate(['/settings/profile']); } }Or use route matching:
ngOnInit(): void { // Navigate to profile by default if on root settings path const urlTree = this.router.parseUrl(this.router.url); if (urlTree.root.children['primary']?.segments.length === 1 && urlTree.root.children['primary']?.segments[0].path === 'settings') { this.router.navigate(['/settings/profile']); } }src/main/java/com/rcs/ssf/controller/ETagMetricsController.java (1)
23-36: Consider securing actuator endpoints and using a structured response DTO.Two suggestions for improvement:
Security: Actuator endpoints typically expose operational data and should be secured in production. Consider adding
@PreAuthorizeor configuring endpoint security in Spring Boot actuator properties.Type safety: Using
Map<String, Object>for the response loses type safety. Consider creating a dedicated response DTO (e.g.,ETagCacheStatusResponse) for better API documentation and client code generation.Example DTO approach:
public record ETagCacheStatusResponse( boolean operational, int ttlMinutes, String status, String description ) {} @GetMapping("/status") public ETagCacheStatusResponse getETagCacheStatus() { var stats = etagCacheService.getStats(); return new ETagCacheStatusResponse( stats.operational(), stats.ttlMinutes(), stats.status(), "ETags are cached in Redis with " + stats.ttlMinutes() + " minute TTL to avoid repeated SHA-256 computation" ); }src/main/java/com/rcs/ssf/repository/UserPreferencesRepository.java (1)
7-13: Repository signature looks good; consider tweaking the caching comment
UserPreferencesRepositorycorrectly extendsR2dbcRepository<UserPreferences, Long>andfindByUserIdis idiomatic for Spring Data R2DBC. If Redis caching is actually handled in the service layer, you may want to rephrase the Javadoc to avoid implying this interface itself “supports Redis caching” rather than being cacheable by higher layers.src/main/java/com/rcs/ssf/dto/AccountStatusDto.java (1)
14-21: Prefer using theAccountStatusenum instead of rawStringforstatusGiven there is an
AccountStatusenum (ACTIVE, DEACTIVATED, SUSPENDED), modelingstatusas aStringhere loses type safety and risks drift between allowed values and actual enum constants. Consider:
- Changing the field to
private AccountStatus status;- Replacing
@NotBlankwith@NotNull(and, if needed, adding an enum‑validation constraint)to keep the DTO aligned with the rest of the account status model and GraphQL/schema expectations.
src/main/java/com/rcs/ssf/dto/DeactivationReasonDto.java (1)
8-20: Structured reason input looks good; consider constrainingreasonCodeThe DTO is well-structured for deactivation input. To avoid arbitrary codes creeping in over time, you might later introduce an enum (or at least central constants) for
reasonCode(e.g.,USER_REQUESTED,INACTIVITY, etc.) and map them here, rather than taking a free‑formString.src/main/java/com/rcs/ssf/dto/AvatarUploadResponseDto.java (1)
3-51: Avatar upload response DTO is fine; optional Lombok alignmentThe DTO is straightforward and correct. For consistency with the rest of the
dtopackage (which mostly uses Lombok), you could replace the manual boilerplate with:@Data @NoArgsConstructor @AllArgsConstructor public class AvatarUploadResponseDto { private String avatarKey; private String avatarUrl; private String message; }But the current explicit form is also perfectly valid.
frontend/src/app/features/settings/pages/profile.component.ts (1)
168-176: Avatar state is never updated, so “Remove Avatar” is always disabled
hasAvataris initialized tofalseand only updated in the commented‑out block inside thegetCurrentUser$subscription. As a result:
[disabled]="!hasAvatar"keeps the “Remove Avatar” button permanently disabled.handleAvatarChangedoesn’t updateavatarUrl/hasAvatareither.Once the backend/avatar field is available on
User, re‑enable the logic that derivesavatarUrlandhasAvatarfromuser.avatarKey(or equivalent), and/or update them from the upload response so the remove button reflects actual avatar state.Also applies to: 219-238
src/main/java/com/rcs/ssf/http/ETagFilter.java (1)
45-52: Delegating ETag computation to ETagCacheService is clean and maintains behaviorConstructor injection of
ETagCacheServiceand usinggetOrComputeETag(responseBody)keeps the previous flow intact while enabling centralized caching. As long as the service guarantees a non-null ETag string for any byte array, this filter remains safe and side‑effect compatible with the old implementation.Also applies to: 68-70
frontend/src/app/app.routes.ts (1)
53-95: Settings child routes wiring is correct; consider trimming duplicate redirectsThe nested
childrenunderpath: 'settings'with a default'' -> 'profile'redirect and the lazySettingsComponentalign well with Angular’s standalone routing. GivenSettingsComponentalready redirects/settingsto/settings/profileon init, you could remove either that logic or this child redirect to avoid redundant behavior, but it’s not functionally harmful.src/main/java/com/rcs/ssf/service/UserPreferencesService.java (2)
30-58: Consider concurrency on first-time creation and align with cache/TTL configOn a cache miss, two concurrent calls to
getPreferencesByUserIdfor the sameuserIdcan both see an empty result and attemptsave(new UserPreferences(userId)). If there is a uniqueness constraint onuser_id, one insert will fail; if not, you may end up with duplicate rows.If that matters in your deployment, consider:
- Enforcing uniqueness on
user_idand handling duplicate-key errors by re-reading, or- Using an idempotent “get-or-create” pattern at the database level (e.g., upsert / MERGE).
Also, the Javadoc mentions a 60‑minute cache duration, but that’s governed by the cache configuration rather than this class. If the TTL may change, it might be safer to either reference configuration (
app.cache.user-preferences.ttl) or remove the hardcoded duration from the comment.
63-99: UpdateupdatedAtwhen mutating user preferences
updatePreferencesselectively mutates fields and persists them, but it never advancesupdatedAt, even though the entity exposes that column and the create‑time constructor initializes it. If any part of the system (UI or auditing) relies onupdatedAt, it will silently become stale.You could set it just before saving:
- if (dto.getNotificationSecurityUpdates() != null) { - existing.setNotificationSecurityUpdates(dto.getNotificationSecurityUpdates()); - } - UserPreferences updated = userPreferencesRepository + if (dto.getNotificationSecurityUpdates() != null) { + existing.setNotificationSecurityUpdates(dto.getNotificationSecurityUpdates()); + } + + existing.setUpdatedAt(System.currentTimeMillis()); + + UserPreferences updated = userPreferencesRepository .save(existing) .timeout(OPERATION_TIMEOUT) .block();This keeps the timestamp semantics consistent with the creation path.
src/main/java/com/rcs/ssf/service/AvatarUploadService.java (1)
82-101: Make "avatar not found" handling more robust than message string matchingCatch
ErrorResponseException(a subclass ofMinioException) specifically and check the structured error code rather than relying on string matching. Current approach catches the parentMinioExceptionand usese.getMessage().contains("The specified key does not exist"), which is brittle—error messages can change, and other failure modes may produce similar text.Recommended pattern for MinIO 8.6.0:
- Import and catch
ErrorResponseException, then calle.errorResponse()to access the error code- Check the error code for "NoSuchKey" or check the HTTP status code for 404
- This makes the code resilient to message text changes and explicitly distinguishes not-found from other failures
Suggested refactor in
AvatarUploadService.downloadAvatar()(lines 82–101):catch (ErrorResponseException e) { if ("NoSuchKey".equals(e.errorResponse().code()) || e.response().code() == 404) { log.warn("Avatar not found: {}", objectKey); return Optional.empty(); } throw e; } catch (MinioException e) { throw e; }You will also need to add the import:
import io.minio.errors.ErrorResponseException;Additionally, document that callers must close the returned
InputStreamto release network resources (already shown in MinIO'sgetObjectjavadoc as best practice).src/main/java/com/rcs/ssf/controller/UserProfileController.java (3)
82-85: Improve error response to include error details.The error handling swallows all exception details and returns an empty 500 response. Consider including an error message or using a structured error response DTO to help clients diagnose upload failures.
Apply this diff to improve error handling:
- .onErrorResume(e -> Mono.just( - ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).build() - )); + .onErrorResume(e -> { + String errorMsg = e.getMessage() != null ? e.getMessage() : "Avatar upload failed"; + return Mono.just( + ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) + .body(new AvatarUploadResponseDto(null, null, errorMsg)) + ); + });
109-112: Consider memory implications of loading entire avatar in memory.Using
readAllBytes()loads the entire avatar file into memory. For large images (up to 5MB as mentioned in comments), this could cause memory pressure under high concurrency.Consider streaming the response body instead:
return Mono.fromCallable(() -> avatarUploadService.downloadAvatar(avatarKey)) .subscribeOn(Schedulers.boundedElastic()) .map(avatarDataOpt -> { if (avatarDataOpt.isEmpty()) { return (ResponseEntity<?>) ResponseEntity.notFound().build(); } String contentType = determineContentType(avatarKey); return (ResponseEntity<?>) ResponseEntity.ok() .header("Content-Type", contentType) .header("Cache-Control", "public, max-age=86400") .body(new InputStreamResource(avatarDataOpt.get())); }) // ... rest of error handlingNote: You'll need to import
org.springframework.core.io.InputStreamResourceand adjust the return type.
178-184: Fragile exception handling relying on message content.Parsing exception messages with string matching (line 180) is brittle and can break if error messages change. Consider creating custom exceptions or checking exception types instead.
If
AvatarUploadServicecan throw custom exceptions, use type-based handling:.onErrorResume(e -> { - // Map exceptions to appropriate HTTP status codes - if (e.getMessage() != null && e.getMessage().contains("not found")) { + if (e instanceof AvatarNotFoundException) { return Mono.just((ResponseEntity<?>) ResponseEntity.status(HttpStatus.NOT_FOUND).build()); } return Mono.just((ResponseEntity<?>) ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).build()); })frontend/src/app/features/settings/pages/api-keys.component.ts (1)
235-260: Complete the GraphQL integration for key generation.The
submitGenerateKeymethod has the GraphQL mutation commented out, leaving a non-functional placeholder. The form and UI are complete but won't actually generate keys.Do you want me to help complete the GraphQL integration? I can provide the correct service calls based on the
SettingsServiceinterface.frontend/src/app/core/services/settings.service.ts (1)
290-309: Complete avatar upload implementation or document it as future work.The
uploadAvatarmethod is a placeholder that immediately throws an error. Since the PR introducesUserProfileControllerwith a REST endpoint for avatar upload (POST /api/user/avatar), this method should useHttpClientto call that endpoint.Do you want me to generate the implementation using Angular's
HttpClient?Example:
uploadAvatar(file: File): Observable<any> { const formData = new FormData(); formData.append('file', file); return this.http.post<any>('/api/user/avatar', formData).pipe( map(response => ({ success: true, avatarKey: response.avatarKey, avatarUrl: response.avatarUrl, message: response.message })) ); }Note: You'll need to inject
HttpClientfrom@angular/common/http.src/main/java/com/rcs/ssf/graphql/SettingsMutation.java (2)
143-149: Avoid swallowing all delete errors behind aBooleanflag
deleteApiKeymaps every error (not found,unauthorized, repository failures, timeouts, etc.) tofalsevia.onErrorReturn(false). This makes it hard for clients and operators to distinguish user errors from server-side issues and hides useful error messages thatApiKeyService.deleteApiKeyalready provides.Consider either:
- letting exceptions propagate so GraphQL returns structured errors, or
- mapping only specific expected exceptions (e.g.
IllegalArgumentException) tofalsewhile letting unexpected exceptions bubble up.
74-80: Update Javadoc to reflect fully reactivegetAccountStatusThe comment still says “blocks for GraphQL response”, but the method now returns
Mono<String>and delegates to a reactive service without explicit blocking. It would be clearer to update the Javadoc to describe it as a reactive resolver returning aMono.src/main/java/com/rcs/ssf/service/ApiKeyService.java (1)
184-256: Be mindful of O(n) validation cost as active key count grows
validateApiKeyReactive(and the deprecatedvalidateApiKey) iterate over all active keys viafindAllActive(currentTime)andpasswordEncoder.matches, which is O(n) in the number of active keys. The Javadoc already calls this out and suggests storing a hash prefix.As active-key volume increases, consider:
- adding an indexed “hash prefix” or similar lookup hint so validation can short-list candidates, and/or
- enforcing reasonable limits on the number of active keys per user to keep
findAllActivebounded.This is mostly an operational/scalability concern; the current implementation is correct for moderate volumes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
HELP.md(1 hunks)README.md(4 hunks)db/migration/V500__user_settings_profile.sql(1 hunks)db/migration/V501__api_keys_add_preview.sql(1 hunks)frontend/src/app/app.routes.ts(1 hunks)frontend/src/app/core/services/settings.service.ts(1 hunks)frontend/src/app/features/settings/pages/api-keys.component.ts(1 hunks)frontend/src/app/features/settings/pages/notifications.component.ts(1 hunks)frontend/src/app/features/settings/pages/preferences.component.ts(1 hunks)frontend/src/app/features/settings/pages/profile.component.ts(1 hunks)frontend/src/app/features/settings/settings.component.ts(1 hunks)src/main/java/com/rcs/ssf/config/CorsProperties.java(1 hunks)src/main/java/com/rcs/ssf/config/SecurityConfig.java(2 hunks)src/main/java/com/rcs/ssf/controller/AuthController.java(1 hunks)src/main/java/com/rcs/ssf/controller/CacheMetricsController.java(1 hunks)src/main/java/com/rcs/ssf/controller/ETagMetricsController.java(1 hunks)src/main/java/com/rcs/ssf/controller/UserProfileController.java(1 hunks)src/main/java/com/rcs/ssf/dto/AccountStatusDto.java(1 hunks)src/main/java/com/rcs/ssf/dto/ApiKeyDto.java(1 hunks)src/main/java/com/rcs/ssf/dto/AvatarUploadResponseDto.java(1 hunks)src/main/java/com/rcs/ssf/dto/CreateApiKeyRequestDto.java(1 hunks)src/main/java/com/rcs/ssf/dto/CreateApiKeyResponseDto.java(1 hunks)src/main/java/com/rcs/ssf/dto/DeactivationReasonDto.java(1 hunks)src/main/java/com/rcs/ssf/dto/UserPreferencesDto.java(1 hunks)src/main/java/com/rcs/ssf/entity/AccountDeactivationAudit.java(1 hunks)src/main/java/com/rcs/ssf/entity/AccountStatus.java(1 hunks)src/main/java/com/rcs/ssf/entity/ApiKey.java(1 hunks)src/main/java/com/rcs/ssf/entity/User.java(2 hunks)src/main/java/com/rcs/ssf/entity/UserPreferences.java(1 hunks)src/main/java/com/rcs/ssf/graphql/AuthMutation.java(1 hunks)src/main/java/com/rcs/ssf/graphql/SettingsMutation.java(1 hunks)src/main/java/com/rcs/ssf/http/ETagFilter.java(4 hunks)src/main/java/com/rcs/ssf/repository/AccountDeactivationAuditRepository.java(1 hunks)src/main/java/com/rcs/ssf/repository/ApiKeyRepository.java(1 hunks)src/main/java/com/rcs/ssf/repository/UserPreferencesRepository.java(1 hunks)src/main/java/com/rcs/ssf/service/AccountService.java(1 hunks)src/main/java/com/rcs/ssf/service/ApiKeyService.java(1 hunks)src/main/java/com/rcs/ssf/service/AuditService.java(1 hunks)src/main/java/com/rcs/ssf/service/AvatarUploadService.java(1 hunks)src/main/java/com/rcs/ssf/service/ETagCacheService.java(1 hunks)src/main/java/com/rcs/ssf/service/UserPreferencesService.java(1 hunks)src/main/java/com/rcs/ssf/service/UserService.java(2 hunks)src/main/resources/application-dev.yml(1 hunks)src/main/resources/application-production.yml(1 hunks)src/main/resources/application.yml(3 hunks)src/main/resources/graphql/schema.graphqls(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (26)
src/main/java/com/rcs/ssf/controller/ETagMetricsController.java (1)
src/main/java/com/rcs/ssf/controller/CacheMetricsController.java (1)
RestController(17-172)
frontend/src/app/app.routes.ts (1)
frontend/src/app/core/guards/auth.guard.ts (1)
authGuard(21-40)
frontend/src/app/features/settings/pages/preferences.component.ts (2)
frontend/src/app/features/settings/pages/notifications.component.ts (1)
Component(20-347)frontend/src/app/features/settings/settings.component.ts (1)
Component(16-99)
src/main/java/com/rcs/ssf/dto/CreateApiKeyResponseDto.java (2)
src/main/java/com/rcs/ssf/dto/ApiKeyDto.java (1)
Data(12-29)src/main/java/com/rcs/ssf/dto/CreateApiKeyRequestDto.java (1)
Data(15-41)
src/main/java/com/rcs/ssf/dto/AccountStatusDto.java (4)
src/main/java/com/rcs/ssf/dto/ApiKeyDto.java (1)
Data(12-29)src/main/java/com/rcs/ssf/dto/CreateApiKeyRequestDto.java (1)
Data(15-41)src/main/java/com/rcs/ssf/dto/DeactivationReasonDto.java (1)
Data(12-21)src/main/java/com/rcs/ssf/dto/UserPreferencesDto.java (1)
Data(12-32)
src/main/java/com/rcs/ssf/repository/ApiKeyRepository.java (1)
src/main/java/com/rcs/ssf/service/reactive/ReactiveDataSourceConfiguration.java (1)
org(343-460)
src/main/java/com/rcs/ssf/repository/AccountDeactivationAuditRepository.java (1)
src/main/java/com/rcs/ssf/service/reactive/ReactiveDataSourceConfiguration.java (1)
org(343-460)
frontend/src/app/features/settings/pages/profile.component.ts (3)
frontend/src/app/features/settings/pages/api-keys.component.ts (1)
Component(35-336)frontend/src/app/features/settings/settings.component.ts (1)
Component(16-99)frontend/src/app/core/services/auth.service.ts (1)
User(26-30)
src/main/java/com/rcs/ssf/dto/CreateApiKeyRequestDto.java (2)
src/main/java/com/rcs/ssf/dto/ApiKeyDto.java (1)
Data(12-29)src/main/java/com/rcs/ssf/dto/CreateApiKeyResponseDto.java (1)
Data(12-24)
frontend/src/app/features/settings/pages/notifications.component.ts (3)
frontend/src/app/features/settings/pages/api-keys.component.ts (1)
Component(35-336)frontend/src/app/features/settings/pages/preferences.component.ts (1)
Component(20-265)frontend/src/app/features/settings/settings.component.ts (1)
Component(16-99)
frontend/src/app/features/settings/pages/api-keys.component.ts (2)
frontend/src/app/features/settings/pages/notifications.component.ts (1)
Component(20-347)frontend/src/app/features/settings/settings.component.ts (1)
Component(16-99)
src/main/java/com/rcs/ssf/entity/UserPreferences.java (3)
src/main/java/com/rcs/ssf/dto/UserPreferencesDto.java (1)
Data(12-32)src/main/java/com/rcs/ssf/entity/ApiKey.java (1)
Table(14-87)src/main/java/com/rcs/ssf/entity/User.java (1)
Table(14-108)
frontend/src/app/features/settings/settings.component.ts (4)
frontend/src/app/features/settings/pages/api-keys.component.ts (1)
Component(35-336)frontend/src/app/features/settings/pages/notifications.component.ts (1)
Component(20-347)frontend/src/app/features/settings/pages/preferences.component.ts (1)
Component(20-265)frontend/src/app/features/settings/pages/profile.component.ts (1)
Component(22-272)
src/main/java/com/rcs/ssf/repository/UserPreferencesRepository.java (1)
src/main/java/com/rcs/ssf/service/reactive/ReactiveDataSourceConfiguration.java (1)
org(343-460)
src/main/java/com/rcs/ssf/service/AccountService.java (2)
src/main/java/com/rcs/ssf/graphql/SettingsMutation.java (1)
Slf4j(31-249)frontend/src/app/core/services/settings.service.ts (3)
deactivateAccount(253-259)reactivateAccount(264-270)getAccountStatus(242-248)
src/main/java/com/rcs/ssf/dto/UserPreferencesDto.java (5)
src/main/java/com/rcs/ssf/dto/AccountStatusDto.java (1)
Data(11-22)src/main/java/com/rcs/ssf/dto/ApiKeyDto.java (1)
Data(12-29)src/main/java/com/rcs/ssf/dto/CreateApiKeyRequestDto.java (1)
Data(15-41)src/main/java/com/rcs/ssf/dto/CreateApiKeyResponseDto.java (1)
Data(12-24)src/main/java/com/rcs/ssf/dto/DeactivationReasonDto.java (1)
Data(12-21)
src/main/java/com/rcs/ssf/controller/UserProfileController.java (1)
src/main/java/com/rcs/ssf/dto/AvatarUploadResponseDto.java (1)
AvatarUploadResponseDto(6-51)
src/main/java/com/rcs/ssf/service/AvatarUploadService.java (1)
frontend/src/app/core/services/settings.service.ts (1)
uploadAvatar(290-309)
src/main/java/com/rcs/ssf/service/UserService.java (1)
frontend/src/app/core/services/settings.service.ts (1)
updatePassword(277-284)
src/main/java/com/rcs/ssf/entity/AccountDeactivationAudit.java (4)
src/main/java/com/rcs/ssf/dto/DeactivationReasonDto.java (1)
Data(12-21)src/main/java/com/rcs/ssf/entity/ApiKey.java (1)
Table(14-87)src/main/java/com/rcs/ssf/entity/User.java (1)
Table(14-108)src/main/java/com/rcs/ssf/entity/UserPreferences.java (1)
Table(14-76)
src/main/java/com/rcs/ssf/controller/CacheMetricsController.java (1)
src/main/java/com/rcs/ssf/controller/ETagMetricsController.java (1)
RestController(13-37)
src/main/java/com/rcs/ssf/graphql/SettingsMutation.java (5)
src/main/java/com/rcs/ssf/security/AuthenticatedUser.java (1)
AuthenticatedUser(9-24)src/main/java/com/rcs/ssf/service/AccountService.java (1)
Slf4j(20-154)src/main/java/com/rcs/ssf/service/ApiKeyService.java (1)
Slf4j(24-327)src/main/java/com/rcs/ssf/service/UserPreferencesService.java (1)
Slf4j(18-126)src/main/java/com/rcs/ssf/service/UserService.java (1)
Slf4j(36-445)
src/main/java/com/rcs/ssf/entity/ApiKey.java (5)
src/main/java/com/rcs/ssf/dto/ApiKeyDto.java (1)
Data(12-29)src/main/java/com/rcs/ssf/dto/CreateApiKeyRequestDto.java (1)
Data(15-41)src/main/java/com/rcs/ssf/dto/CreateApiKeyResponseDto.java (1)
Data(12-24)src/main/java/com/rcs/ssf/entity/User.java (1)
Table(14-108)src/main/java/com/rcs/ssf/entity/UserPreferences.java (1)
Table(14-76)
src/main/java/com/rcs/ssf/dto/DeactivationReasonDto.java (1)
src/main/java/com/rcs/ssf/dto/AccountStatusDto.java (1)
Data(11-22)
src/main/java/com/rcs/ssf/dto/ApiKeyDto.java (2)
src/main/java/com/rcs/ssf/dto/CreateApiKeyRequestDto.java (1)
Data(15-41)src/main/java/com/rcs/ssf/dto/CreateApiKeyResponseDto.java (1)
Data(12-24)
src/main/java/com/rcs/ssf/service/ApiKeyService.java (2)
src/main/java/com/rcs/ssf/graphql/SettingsMutation.java (1)
Slf4j(31-249)frontend/src/app/core/services/settings.service.ts (3)
generateApiKey(199-209)revokeApiKey(215-223)deleteApiKey(229-237)
🪛 ast-grep (0.40.0)
src/main/java/com/rcs/ssf/service/ETagCacheService.java
[warning] 108-108: Detected SHA1 hash algorithm which is considered insecure. SHA1 is not collision resistant and is therefore not suitable as a cryptographic signature. Instead, use PBKDF2 for password hashing or SHA256 or SHA512 for other hash function applications.
Context: MessageDigest.getInstance("SHA-1")
Note: [CWE-328] Use of Weak Hash. [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(use-of-sha1-java)
src/main/java/com/rcs/ssf/service/AvatarUploadService.java
[warning] 157-157: Detected MD5 hash algorithm which is considered insecure. MD5 is not collision resistant and is therefore not suitable as a cryptographic signature. Use HMAC instead.
Context: "MD5"
Note: [CWE-328] Use of Weak Hash. [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(use-of-md5-java)
⏰ 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). (1)
- GitHub Check: build
🔇 Additional comments (45)
src/main/resources/application.yml (3)
124-125: LGTM! CORS configuration is well-structured.The CORS configuration appropriately defaults to
http://localhost:4200for development while allowing production override via environment variable. This aligns with the configuration inapplication-dev.ymlandapplication-production.yml.
161-166: LGTM! Metrics configuration enhances observability.Enabling cache, JVM, process, and system metrics provides essential operational visibility without performance concerns.
178-187: LGTM! Logging adjustments reduce verbosity appropriately.Setting these components to INFO level provides a good balance between observability and log volume for production environments.
db/migration/V500__user_settings_profile.sql (3)
4-38: LGTM! Users table alterations are well-structured.The columns are appropriately typed and the idempotent exception handling ensures safe re-runs. The default value for
account_statusaligns with theAccountStatus.ACTIVEenum shown in theUserentity.
40-76: LGTM! User preferences table is well-designed.The schema appropriately uses
NUMBER(1)for boolean flags,NUMBER(19)for timestamps, and includes proper indexing and cascading deletes. TheUNIQUEconstraint onuser_idensures one-to-one relationship with users.
103-126: LGTM! API key indexes support efficient queries.The composite index on
(user_id, revoked_at, expires_at)efficiently supports queries for active keys per user, and the single-column index onuser_idprovides broader query support.src/main/java/com/rcs/ssf/entity/AccountDeactivationAudit.java (1)
1-48: LGTM! Entity is well-structured with auto-timestamp initialization.The entity cleanly maps to the audit table schema (once the table is created per the critical issue flagged in the repository review). The special constructor that auto-sets
timestampto the current time is a good defensive pattern.README.md (4)
191-205: LGTM! CORS documentation is clear and security-conscious.The documentation appropriately emphasizes the production requirement for CORS configuration and provides practical examples for setting trusted origins.
337-339: LGTM! Avatar and cache configuration is well-documented.The default values are reasonable (5 MB max size, common image formats, 60-minute cache TTL) and clearly documented.
434-546: LGTM! GraphQL examples are comprehensive and syntactically correct.The examples demonstrate proper GraphQL query and mutation syntax, covering the full range of user settings operations introduced in this PR.
548-583: LGTM! REST avatar endpoints are well-documented.The documentation clearly describes the upload, download, and delete operations with appropriate constraints and response examples that align with the configured limits.
src/main/java/com/rcs/ssf/controller/AuthController.java (1)
76-77: LGTM! Defensive truncation prevents audit log failures.Using
AuditService.truncateFailureReason()ensures that exception messages don't exceed database column limits when logging authentication failures, preventing secondary errors during audit logging.src/main/resources/application-dev.yml (1)
4-6: LGTM! Dev CORS configuration covers common development scenarios.The allowed origins appropriately include multiple localhost and 127.0.0.1 variants on common frontend development ports (4200 for Angular, 3000 for React, 8080 for general dev).
src/main/resources/application-production.yml (1)
43-45: LGTM! Production CORS requires explicit configuration.Correctly sources allowed origins from the
CORS_ALLOWED_ORIGINSenvironment variable without a fallback, enforcing explicit configuration in production. The comment provides helpful examples.src/main/java/com/rcs/ssf/graphql/AuthMutation.java (1)
135-136: LGTM! Defensive improvement for database storage.The truncation of failure reasons prevents potential database column overflow issues while preserving audit trail information.
src/main/java/com/rcs/ssf/entity/AccountStatus.java (1)
1-22: LGTM! Clean type-safe enum implementation.The enum provides clear, well-documented account status values with type safety. The Javadoc comments effectively describe each status state.
src/main/java/com/rcs/ssf/config/CorsProperties.java (1)
1-41: LGTM! Well-documented configuration properties class.The implementation follows Spring Boot best practices with clear documentation and sensible defaults. The Javadoc examples effectively demonstrate configuration usage across different environments.
src/main/java/com/rcs/ssf/service/AuditService.java (1)
18-42: Well-implemented utility method with verified database compatibility.The truncation logic correctly handles null inputs and preserves meaningful failure information within database constraints. Verification confirms the
failure_reasoncolumn is defined asVARCHAR2(500)in the database schema, and the method safely truncates to 499 characters max (496 chars + "..." suffix), well within the column limit.db/migration/V501__api_keys_add_preview.sql (1)
4-13: The error code is correct as-is.ORA-01430 (numeric code 1430) refers to "column being added already exists in table." In Oracle PL/SQL,
SQLCODEreturns error codes as negative numbers, so the code correctly checks forSQLCODE = -1430to catch this exception. No changes required.src/main/java/com/rcs/ssf/config/SecurityConfig.java (1)
84-94: CORS origin parsing viaCorsPropertieslooks correct; be mindful of empty config and credentialsUsing
CorsProperties.getAllowedOrigins()andparseAllowedOriginsto drivesetAllowedOriginPatternsis a good improvement over hardcoding. A couple of behavioral points to keep in mind:
- If
allowedOriginsis ever set to an empty string, no origin patterns will be configured, effectively blocking all cross‑origin requests.configuration.setAllowCredentials(false);is fine for Authorization‑header–based JWT auth, but will prevent cookie‑based auth from working cross‑origin; if you later introduce cookie sessions, this flag (and matching frontendcredentialssettings) will need revisiting.Functionally the change looks sound as long as the new
app.cors.allowed-origins(or equivalent) property is wired and documented.Also applies to: 212-237
src/main/java/com/rcs/ssf/entity/UserPreferences.java (1)
14-75: UserPreferences defaults and constructors look solidField-level defaults plus the
@PersistenceCreatorconstructor defensively handling nulls give predictable behavior both for DB hydration and programmatic construction; the convenience(Long userId)ctor matches this pattern. No issues from a persistence or caching standpoint.src/main/java/com/rcs/ssf/service/UserService.java (1)
224-243: Stronger post‑conditions in updateUser look goodThe JDBC branch now explicitly fails when
affectedRows == 0, and the R2DBC branch verifies the save by refetching the user and throwing if the record disappeared. This makes update semantics clearer and protects callers from silent no‑ops.src/main/java/com/rcs/ssf/dto/CreateApiKeyResponseDto.java (1)
12-24: CreateApiKeyResponseDto shape looks consistent and safeFields and Lombok annotations line up with the rest of the API key DTOs, and excluding
rawKeyfromtoStringavoids accidental key leakage in logs. No changes needed here.src/main/java/com/rcs/ssf/repository/ApiKeyRepository.java (1)
12-24: Repository methods and active/expired semantics are coherent
findActiveByUserId,findAllActive, anddeleteExpiredKeysall use consistent conditions onrevoked_atandexpires_at, matching theApiKey.isActive()/isExpired()logic in the entity. Signatures look appropriate for R2DBC usage.src/main/java/com/rcs/ssf/entity/ApiKey.java (1)
14-86: ApiKey entity design aligns with repository and DTOsField mappings, constructors, and
isRevoked/isExpired/isActivehelpers are consistent with the repository queries and DTO surface. Using epoch millis keeps things simple and coherent with the rest of the codebase. No changes needed here.src/main/java/com/rcs/ssf/service/AvatarUploadService.java (1)
40-77: Upload pipeline and MinIO usage are correctFor known-size uploads, the recommended pattern is
stream(inputStream, objectSize, -1)to let the client auto-choose multipart part size. The code implements this correctly withstream(inputStream, file.getSize(), -1).Validation (non-empty, ≤5MB, allowed MIME types) and key generation are straightforward with no functional issues.
src/main/java/com/rcs/ssf/controller/UserProfileController.java (2)
193-210: LGTM!The authentication extraction logic appropriately handles different authentication scenarios with a reasonable fallback strategy.
218-227: LGTM!The content type determination is straightforward and covers the supported avatar formats (JPEG, PNG, WebP) with an appropriate fallback.
src/main/java/com/rcs/ssf/entity/User.java (2)
47-64: LGTM! Defensive enum parsing with appropriate fallback.The constructor correctly handles potential database inconsistencies by catching
IllegalArgumentExceptionwhen parsing theaccountStatusstring. Falling back toACTIVEwith a warning is a reasonable strategy for data resilience.
38-45: LGTM! New fields align with PR objectives.The new fields support avatar management and account lifecycle features as described in the PR objectives.
src/main/resources/graphql/schema.graphqls (3)
12-17: LGTM! Account status enum aligns with entity definition.The
AccountStatusenum matches the Java enum and supports the account lifecycle features.
36-45: LGTM! Settings mutations provide comprehensive user management.The mutations cover all the key settings operations: avatar, preferences, API keys, account status, and password updates.
64-110: LGTM! Well-structured DTOs for settings features.The types provide clear contracts for the settings API surface, with appropriate fields for each concern (preferences, API keys, profile, account status).
src/main/java/com/rcs/ssf/dto/UserPreferencesDto.java (2)
19-23: LGTM! Validation on required preference fields.The
@NotBlankconstraints appropriately enforce that theme and language must be provided.
25-28: The notification preferences design is intentional and correct as-is.The DTO's nullable
Booleanfields without defaults implement a standard partial-update pattern:nullmeans "don't change this field," whiletrueorfalseexplicitly sets the value. The service layer correctly respects this by checkingif (dto.getNotificationEmails() != null)before updating (lines 76–86 ofUserPreferencesService). The entity has appropriate defaults (true) applied only when creating new records. No changes needed.Likely an incorrect or invalid review comment.
frontend/src/app/features/settings/pages/api-keys.component.ts (2)
312-316: Verify clipboard API browser support and permissions.The
navigator.clipboard.writeText()API requires HTTPS (or localhost) and may require user permission in some browsers.Consider adding fallback handling:
copyKeyToClipboard(): void { if (!navigator.clipboard) { this.msg.warning('Clipboard access not available. Please copy manually.'); return; } navigator.clipboard.writeText(this.newGeneratedKey).then( () => this.msg.success('API Key copied to clipboard'), (err) => { console.error('Failed to copy:', err); this.msg.error('Failed to copy to clipboard'); } ); }
220-228: LGTM! Form initialization with appropriate validation.The form setup includes proper validators for key name (required, min length) and expiration days (required, range 1-365).
src/main/java/com/rcs/ssf/service/ETagCacheService.java (3)
107-117: SHA-1 usage is acceptable here (not a security concern).The static analysis tool flagged SHA-1 as weak, but in this context it's used only for generating cache keys (content-addressed storage), not for cryptographic signatures or authentication. SHA-1 collisions are not a security risk for this use case since:
- The cache key is internal to Redis (not exposed to users)
- A collision would only result in a cache hit for different content (minor efficiency issue, not a security breach)
- SHA-1 is faster than SHA-256 for this non-cryptographic purpose
The actual ETag computation uses SHA-256 (line 87), which is the security-relevant hash.
50-77: LGTM! Robust cache-aside pattern with graceful degradation.The implementation correctly implements cache-aside with appropriate error handling:
- Cache misses fall back to computation
- Redis failures don't break the request
- Proper logging for observability
140-153: LGTM! Safe Redis connection management.Using
RedisTemplate.execute()withRedisCallbackproperly manages connection lifecycle, avoiding potential connection leaks.frontend/src/app/features/settings/pages/notifications.component.ts (2)
220-245: LGTM! Form initialization with sensible defaults.The forms are initialized with reasonable default values for notification preferences.
247-250: LGTM! Proper subscription cleanup.Using
destroy$withtakeUntilin the submit handlers prevents memory leaks from uncompleted subscriptions.frontend/src/app/core/services/settings.service.ts (1)
199-209: LGTM! API key generation with proper cache refresh.The
generateApiKeymethod correctly usesrefetchQueriesto update the cached API keys list after generation.src/main/java/com/rcs/ssf/service/AccountService.java (1)
117-142: Account status read helpers look correct and defensive
getAccountStatusandisAccountActivehandle nulluserId, empty repository results, and repository errors gracefully (IllegalArgumentExceptionorfalsefor booleans). The use ofdefaultIfEmpty(false)plusonErrorReturn(false)inisAccountActivemakes the read path robust against both missing users and backend failures.src/main/java/com/rcs/ssf/service/ApiKeyService.java (1)
49-81: API key generation flow is secure and one‑time‑view friendly
generateApiKeycorrectly:
- generates a high-entropy raw key with a fixed prefix,
- computes a preview from the raw key,
- hashes the raw key for storage, and
- only returns the raw key in the response without persisting it.
Logging avoids exposing the raw key. From a lifecycle and security perspective this aligns well with common API-key designs.
- Add key_preview column to api_keys table for UI-visible masked previews (e.g., sk_••••••••), avoiding hash-of-hash issues - Modify deactivateAccount GraphQL mutation to use reasonCode and justification fields - Add account_deactivation_audit table with indexes for tracking deactivations, including user_id, timestamp, reason_code, and justification - Refactor CacheMetricsController to extract Caffeine cache entries, reducing code duplication
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (6)
db/migration/V501__api_keys_add_preview.sql (1)
18-27: LGTM: No explicit COMMIT in migration block.The past review concern about explicit COMMIT interfering with Flyway transaction management has been addressed. The UPDATE block (lines 18-27) correctly relies on Flyway's transaction control without manual commits.
db/migration/V500__user_settings_profile.sql (1)
78-102: LGTM:key_previewcolumn now included in api_keys table.Line 85 adds the
key_preview VARCHAR2(255)column that was missing in the previous review. This resolves the entity-schema mapping issue and aligns with theApiKeyentity's@Column("key_preview")annotation.frontend/src/app/features/settings/pages/profile.component.ts (2)
16-16: LGTM:NzGridModulenow imported for grid directives.
NzGridModuleis correctly imported at line 16 and included in the component's imports array at line 40, resolving the past review issue wherenz-rowandnz-coldirectives would not have been available.Also applies to: 40-40
244-266: LGTM:isUpdatingPasswordflag now properly managed.The password change submission now includes the complete
updatePasswordcall with both success and error handlers that reset theisUpdatingPasswordflag (lines 259, 263), resolving the past review issue where the button would remain stuck in the "Updating..." state.frontend/src/app/core/services/settings.service.ts (1)
38-51: Fix multiple GraphQL contract mismatches inSettingsServiceSeveral operations here don’t match the server schema / resolvers and will fail at validation time:
UPDATE_USER_PREFERENCESmutation (lines 38–51)
- Schema:
updateUserPreferences(input: UserPreferencesInput!): UserPreferences!- Java resolver:
updateUserPreferences(@Argument UserPreferencesInput input)- Current query uses
updateUserPreferences(preferences: $preferences)and selects
emailNotifications,pushNotifications,apiKeyNotifications,accountActivityNotifications
which are not fields onUserPreferences(schema usesnotificationEmails,notificationPush,
notificationLoginAlerts,notificationSecurityUpdates).Fix:
- Change the argument name to
input:mutation UpdateUserPreferences($preferences: UserPreferencesInput!) { updateUserPreferences(input: $preferences) { userId theme language notificationEmails notificationPush notificationLoginAlerts notificationSecurityUpdates updatedAt } }- Keep
variables: { preferences }in TS; the variable name doesn’t need to equal the schema arg name.
GET_API_KEYSandREVOKE_API_KEY(lines 56–69, 91–99)
- Schema
type ApiKeyInfoexposesisActive: Boolean!(plus id, keyName, keyPreview, timestamps).- These queries request a
statusfield, which is not in the schema.Fix:
- Either add
statustoApiKeyInfoin the schema and map it fromApiKeyDto, or- Change the queries to use
isActiveand compute a status string client‑side if needed.
GET_ACCOUNT_STATUS(lines 113–121)
- Schema:
getAccountStatus: AccountStatus!(enum).- Resolver: returns
Mono<String>/enum value, not an object.- Query currently treats it as an object:
getAccountStatus { userId status deactivatedAt }.Fix:
- Update the query to:
query GetAccountStatus { getAccountStatus }- And adjust
getAccountStatus()in TS to treat the result as the enum name.If you decide instead to return an object, change the schema + resolver to
AccountStatusResponse!and keep the object shape here.
UPDATE_PASSWORD(lines 152–159)
- Schema:
updatePassword(currentPassword: String!, newPassword: String!): Boolean!- Resolver: returns
Boolean.- Query expects an object
{ success, message }.Fix:
- Either:
- Keep schema/resolver as
Boolean!and change the mutation to:and have TS treat the result as a simplemutation UpdatePassword($currentPassword: String!, $newPassword: String!) { updatePassword(currentPassword: $currentPassword, newPassword: $newPassword) }boolean; or- Change schema + resolver to return a dedicated payload type with
success/message
and keep the current shape.Until these are corrected, the affected operations will not work end‑to‑end.
Check that the GraphQL server’s introspected schema for UserPreferences, ApiKeyInfo, AccountStatus, and updatePassword matches the field/argument names used above (especially `notification*` vs `*Notifications`, `isActive` vs `status`, and the return type of `updatePassword`).Also applies to: 56-69, 91-99, 113-121, 126-147, 152-159
frontend/src/app/features/settings/pages/preferences.component.ts (1)
250-269: HardenloadPreferencesagainst bad/partial localStorage data
loadPreferencesblindlyJSON.parses and patches all fields, so:
- Corrupted JSON in
localStoragewill throw and break the component.- Missing fields (e.g., from older versions) get patched as
undefined, overwriting your in-code defaults.Guard both by catching parse errors and only patching controls when the stored value is not
undefined:private loadPreferences(): void { - const stored = localStorage.getItem('user-preferences'); - if (stored) { - const preferences = JSON.parse(stored); - this.appearanceForm.patchValue({ - theme: preferences.theme, - language: preferences.language - }); - this.notificationForm.patchValue({ - notificationEmails: preferences.notificationEmails, - notificationPush: preferences.notificationPush, - notificationLoginAlerts: preferences.notificationLoginAlerts, - notificationSecurityUpdates: preferences.notificationSecurityUpdates - }); - - // Apply theme when preferences are loaded - if (preferences.theme !== undefined) { - this.applyTheme(preferences.theme); - } - } + const stored = localStorage.getItem('user-preferences'); + if (!stored) { + return; + } + let preferences: any; + try { + preferences = JSON.parse(stored); + } catch { + // Ignore invalid JSON and keep defaults + return; + } + + const appearancePatch: any = {}; + if (preferences.theme !== undefined) { + appearancePatch.theme = preferences.theme; + } + if (preferences.language !== undefined) { + appearancePatch.language = preferences.language; + } + this.appearanceForm.patchValue(appearancePatch); + + const notificationPatch: any = {}; + if (preferences.notificationEmails !== undefined) { + notificationPatch.notificationEmails = preferences.notificationEmails; + } + if (preferences.notificationPush !== undefined) { + notificationPatch.notificationPush = preferences.notificationPush; + } + if (preferences.notificationLoginAlerts !== undefined) { + notificationPatch.notificationLoginAlerts = preferences.notificationLoginAlerts; + } + if (preferences.notificationSecurityUpdates !== undefined) { + notificationPatch.notificationSecurityUpdates = preferences.notificationSecurityUpdates; + } + this.notificationForm.patchValue(notificationPatch); + + if (preferences.theme !== undefined) { + this.applyTheme(preferences.theme); + } }
🧹 Nitpick comments (4)
src/main/java/com/rcs/ssf/controller/CacheMetricsController.java (2)
35-69: Reduce duplication by reusinggetCaffeineCacheEntries()ingetCaffeineStats
getCaffeineStatsandgetCaffeineCacheEntriesboth iterate overcacheManager.getCacheNames(), unwrap the native cache, and filter by Caffeine type. This is correct but duplicated and slightly increases maintenance cost.You can reuse
getCaffeineCacheEntries()insidegetCaffeineStats()to centralize the unwrapping logic:- @GetMapping("/caffeine") - public Map<String, Object> getCaffeineStats() { - Map<String, Object> allStats = new HashMap<>(); - - for (String cacheName : cacheManager.getCacheNames()) { - org.springframework.cache.Cache cache = cacheManager.getCache(cacheName); - if (cache != null) { - Object nativeCache = cache.getNativeCache(); - if (nativeCache instanceof Cache) { - @SuppressWarnings("unchecked") - Cache<Object, Object> caffeineCache = (Cache<Object, Object>) nativeCache; - - var stats = caffeineCache.stats(); - - Map<String, Object> cacheStats = new HashMap<>(); - cacheStats.put("stats", stats.toString()); - cacheStats.put("estimatedSize", caffeineCache.estimatedSize()); - cacheStats.put("hitCount", stats.hitCount()); - cacheStats.put("missCount", stats.missCount()); - cacheStats.put("loadSuccessCount", stats.loadSuccessCount()); - cacheStats.put("loadFailureCount", stats.loadFailureCount()); - cacheStats.put("evictionCount", stats.evictionCount()); - cacheStats.put("hitRate", stats.hitRate()); - - allStats.put(cacheName, cacheStats); - } - } - } - - return allStats; - } + @GetMapping("/caffeine") + public Map<String, Object> getCaffeineStats() { + Map<String, Object> allStats = new HashMap<>(); + + for (CacheEntry entry : getCaffeineCacheEntries()) { + var stats = entry.cache().stats(); + + Map<String, Object> cacheStats = new HashMap<>(); + cacheStats.put("stats", stats.toString()); + cacheStats.put("estimatedSize", entry.cache().estimatedSize()); + cacheStats.put("hitCount", stats.hitCount()); + cacheStats.put("missCount", stats.missCount()); + cacheStats.put("loadSuccessCount", stats.loadSuccessCount()); + cacheStats.put("loadFailureCount", stats.loadFailureCount()); + cacheStats.put("evictionCount", stats.evictionCount()); + cacheStats.put("hitRate", stats.hitRate()); + + allStats.put(entry.cacheName(), cacheStats); + } + + return allStats; + }Also applies to: 167-183
112-201: Solid summary aggregation; narrowcomputeOverallHitRatereturn typeThe summary endpoint correctly reuses
getCaffeineCacheEntries(), aggregates totals, and clearly defines hit‑rate semantics for “no data” and “perfect hit rate” cases; the logic looks good.Since all branches of
computeOverallHitRatereturnString, you can simplify the signature to returnStringinstead ofObjectfor better type clarity (and a tighter JSON contract):- private Object computeOverallHitRate(long totalHits, long totalMisses) { + private String computeOverallHitRate(long totalHits, long totalMisses) {No other changes are required.
src/main/java/com/rcs/ssf/controller/UserProfileController.java (1)
106-147: Consider streaming response instead of loading entire avatar into memory.Line 115 calls
is.readAllBytes()which loads the entire avatar file into memory. While the upload limit is 5MB (manageable), under high concurrency this could cause memory pressure. Consider streaming the response directly usingInputStreamResourceor similar to improve scalability:return Mono.fromCallable(() -> avatarUploadService.downloadAvatar(avatarKey)) .subscribeOn(Schedulers.boundedElastic()) .flatMap(avatarDataOpt -> { if (avatarDataOpt.isEmpty()) { return Mono.just((ResponseEntity<?>) ResponseEntity.notFound().build()); } InputStream stream = avatarDataOpt.get(); String contentType = determineContentType(avatarKey); InputStreamResource resource = new InputStreamResource(stream); return Mono.just((ResponseEntity<?>) ResponseEntity.ok() .header("Content-Type", contentType) .header("Cache-Control", "public, max-age=86400") .body(resource)); }) // ... error handlingThis approach streams the file directly to the client without buffering in memory.
src/main/java/com/rcs/ssf/graphql/SettingsMutation.java (1)
160-171: ReuseAccountConstants.DEFAULT_REASON_CODEinstead of duplicating"USER_INITIATED"Here:
reason.setReasonCode(reasonCode != null ? reasonCode : "USER_INITIATED");you already centralize the default in
AccountConstants.DEFAULT_REASON_CODE(used inAccountService). To avoid drift between layers, wire this through the constant instead of duplicating the literal.- DeactivationReasonDto reason = new DeactivationReasonDto(); - reason.setReasonCode(reasonCode != null ? reasonCode : "USER_INITIATED"); + DeactivationReasonDto reason = new DeactivationReasonDto(); + reason.setReasonCode(reasonCode != null ? reasonCode : AccountConstants.DEFAULT_REASON_CODE);(You’ll need to import
AccountConstantshere.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
HELP.md(1 hunks)db/migration/V500__user_settings_profile.sql(1 hunks)db/migration/V501__api_keys_add_preview.sql(1 hunks)frontend/src/app/core/services/settings.service.ts(1 hunks)frontend/src/app/features/settings/pages/notifications.component.ts(1 hunks)frontend/src/app/features/settings/pages/preferences.component.ts(1 hunks)frontend/src/app/features/settings/pages/profile.component.ts(1 hunks)src/main/java/com/rcs/ssf/controller/CacheMetricsController.java(1 hunks)src/main/java/com/rcs/ssf/controller/UserProfileController.java(1 hunks)src/main/java/com/rcs/ssf/dto/ApiKeyDto.java(1 hunks)src/main/java/com/rcs/ssf/dto/CreateApiKeyRequestDto.java(1 hunks)src/main/java/com/rcs/ssf/graphql/SettingsMutation.java(1 hunks)src/main/java/com/rcs/ssf/service/AccountService.java(1 hunks)src/main/java/com/rcs/ssf/service/ApiKeyService.java(1 hunks)src/main/java/com/rcs/ssf/service/AvatarUploadService.java(1 hunks)src/main/java/com/rcs/ssf/service/UserPreferencesService.java(1 hunks)src/main/java/com/rcs/ssf/service/UserService.java(3 hunks)src/main/java/com/rcs/ssf/util/AccountConstants.java(1 hunks)src/main/resources/graphql/schema.graphqls(4 hunks)src/test/java/com/rcs/ssf/graphql/AdminMutationIntegrationTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/rcs/ssf/dto/CreateApiKeyRequestDto.java
- src/main/java/com/rcs/ssf/dto/ApiKeyDto.java
🧰 Additional context used
🧬 Code graph analysis (11)
frontend/src/app/features/settings/pages/profile.component.ts (1)
frontend/src/app/core/services/auth.service.ts (1)
User(26-30)
src/main/java/com/rcs/ssf/controller/CacheMetricsController.java (1)
src/main/java/com/rcs/ssf/controller/ETagMetricsController.java (1)
RestController(13-37)
src/main/java/com/rcs/ssf/graphql/SettingsMutation.java (5)
src/main/java/com/rcs/ssf/security/AuthenticatedUser.java (1)
AuthenticatedUser(9-24)src/main/java/com/rcs/ssf/service/AccountService.java (1)
Slf4j(22-157)src/main/java/com/rcs/ssf/service/ApiKeyService.java (1)
Slf4j(24-337)src/main/java/com/rcs/ssf/service/UserPreferencesService.java (1)
Slf4j(22-135)src/main/java/com/rcs/ssf/service/UserService.java (1)
Slf4j(36-447)
src/main/java/com/rcs/ssf/service/UserPreferencesService.java (2)
src/main/java/com/rcs/ssf/graphql/SettingsMutation.java (1)
Slf4j(31-253)src/main/java/com/rcs/ssf/service/ETagCacheService.java (1)
Service(24-159)
src/main/java/com/rcs/ssf/service/AvatarUploadService.java (1)
frontend/src/app/core/services/settings.service.ts (1)
uploadAvatar(293-306)
frontend/src/app/features/settings/pages/preferences.component.ts (2)
frontend/src/app/features/settings/pages/notifications.component.ts (1)
Component(20-371)frontend/src/app/features/settings/settings.component.ts (1)
Component(16-99)
src/main/java/com/rcs/ssf/controller/UserProfileController.java (3)
src/main/java/com/rcs/ssf/dto/AvatarUploadResponseDto.java (1)
AvatarUploadResponseDto(6-51)frontend/src/app/core/services/auth.service.ts (1)
User(26-30)frontend/src/app/core/services/settings.service.ts (1)
uploadAvatar(293-306)
src/main/java/com/rcs/ssf/service/UserService.java (1)
frontend/src/app/core/services/settings.service.ts (1)
updatePassword(279-286)
src/main/java/com/rcs/ssf/service/ApiKeyService.java (1)
src/main/java/com/rcs/ssf/graphql/SettingsMutation.java (1)
Slf4j(31-253)
src/main/java/com/rcs/ssf/service/AccountService.java (2)
src/main/java/com/rcs/ssf/util/AccountConstants.java (1)
AccountConstants(7-41)src/main/java/com/rcs/ssf/graphql/SettingsMutation.java (1)
Slf4j(31-253)
frontend/src/app/features/settings/pages/notifications.component.ts (2)
frontend/src/app/features/settings/pages/preferences.component.ts (1)
Component(20-289)frontend/src/app/features/settings/settings.component.ts (1)
Component(16-99)
🪛 ast-grep (0.40.0)
src/main/java/com/rcs/ssf/service/AvatarUploadService.java
[warning] 173-173: Detected MD5 hash algorithm which is considered insecure. MD5 is not collision resistant and is therefore not suitable as a cryptographic signature. Use HMAC instead.
Context: "MD5"
Note: [CWE-328] Use of Weak Hash. [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(use-of-md5-java)
🔇 Additional comments (10)
src/main/java/com/rcs/ssf/controller/CacheMetricsController.java (1)
15-33: Clear controller design and security documentationConstructor injection, actuator‑like pathing, and the explicit security note align well with the existing
ETagMetricsControllerpattern; no issues here.src/main/java/com/rcs/ssf/service/UserService.java (2)
57-58: Password field now correctly hydrated for JDBC mode.This resolves the critical issue from the previous review where
updatePasswordwould fail in JDBC mode due to null password. The mapper now properly reads and sets the password field with SQL NULL handling.
227-244: LGTM: Improved error handling in both JDBC and R2DBC paths.The JDBC path now explicitly checks
affectedRowsand throws a descriptive error when no rows are updated, while the R2DBC path refetches the user after save to verify persistence. Both changes improve reliability and error detection for concurrent modifications.src/main/java/com/rcs/ssf/service/AvatarUploadService.java (2)
172-181: MD5 usage is acceptable here for non-cryptographic filename generation.The static analysis warning about MD5 being cryptographically weak is valid in general, but in this context MD5 is used only to generate unique object keys for avatar filenames, not for security-critical operations like password hashing or signatures. This usage is appropriate and efficient.
92-117: LGTM: Robust error handling using structured error codes.The
downloadAvatarmethod correctly usesErrorResponseException.errorResponse().code()and HTTP status codes for error detection rather than brittle message string matching. The distinction between "not found" (returns empty) and other errors (propagates exception) is appropriate for the caller's needs.src/main/java/com/rcs/ssf/controller/UserProfileController.java (1)
51-90: LGTM: Proper reactive flow for avatar upload with error handling.The upload endpoint correctly uses
boundedElastic()for the blocking MinIO operation, updates the user reactively, and provides comprehensive error handling with appropriate HTTP status codes.HELP.md (2)
102-106: LGTM: Comprehensive troubleshooting guidance for new features.The new troubleshooting entries clearly document common avatar upload, API key, preferences cache, and account deactivation issues with actionable solutions. This will help developers quickly resolve configuration and usage problems.
231-240: No action required—documentation matches schema.The GraphQL schema at
src/main/resources/graphql/schema.graphqls:43confirms thedeactivateAccountmutation signature matches the documented parameters exactly:deactivateAccount(reasonCode: String, justification: String): AccountStatusResponse!The documentation in HELP.md is accurate and consistent with the schema definition.
db/migration/V500__user_settings_profile.sql (1)
4-38: LGTM: Idempotent user table extensions with proper error handling.The migration safely extends the
userstable withavatar_key,account_status, andaccount_deactivated_atcolumns using exception handling to gracefully handle already-existing columns. The SQLCODE checks are appropriate for Oracle.src/main/java/com/rcs/ssf/util/AccountConstants.java (1)
1-41: LGTM: Well-structured constants class for account operations.The utility class follows standard patterns with a private constructor and clearly documented reason codes. The constants provide a centralized, type-safe way to reference deactivation reasons across the account management feature.
1 participant
Add comprehensive avatar upload/download/delete REST APIs and GraphQL mutations for user preferences, including caching with Redis. Includes troubleshooting docs for common issues like upload failures and cache invalidation. Enhances user settings with operations for API keys, password changes, and account status, driven by demand for personalized user experiences.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.