fix: Token auth principal type#581
Conversation
Users without a linked Member record (e.g. seeded admin accounts with memberId=0) hit the fallback auth path which previously set a raw UserAccount as the Security principal. AuthService.getCurrentUser() only accepts UserAccount.User, so every @RequiresRole-guarded endpoint threw ForbiddenException for these users. Wrap the fallback UserAccount in UserAccount.User(account, null) so the principal type is consistent. Derive GrantedAuthorities from the account's own roles instead of hard-coding ADMIN; null-member handling in User.getAllRoles() ensures the account roles are still found.
auth.getName() is unreliable when the principal is a UserAccount.User record; extract the email directly from the principal to avoid a null lookup. Also adds OpenAPI security requirements and @ResponseStatus to the /me endpoint for consistency with other secured endpoints.
goelsonali
requested review from
dricazenck,
gpaOliveira,
lauracabtay,
nora-weisser,
nverbic,
silkenodwell and
wilsonadenuga
Replace explicit cast with Java 16 pattern matching to resolve the Sonar 'instanceof check and cast' violation.
dricazenck
changed the title
womencodingcommunity
left a comment
womencodingcommunity
left a comment
There was a problem hiding this comment.
Great direction on this fix, Sonali! Correcting the principal type to UserAccount.User and deriving authorities from actual account roles rather than hardcoding ADMIN is exactly the right approach — it unblocks the /me endpoint and lays the foundation for proper role-based access going forward.
Two things to address before merge:
[WARNING]account.getRoles().stream()inTokenAuthFilter— ifrolesisnull, this throws NPE in the auth filter (500 for affected users). See inline comment for a one-liner fix.[WARNING]The ternary inAuthController.currentUser()passesnulltofindUserByEmailwhen the principal type does not match — should be an explicit early return instead. See inline comment.
Also a question on the @RequiresRole addition to /me (see inline) — just needs a quick confirm that restricting it to ADMIN/LEADER is intentional.
…into fix/token-auth-principal-type
Guard against null roles list in TokenAuthFilter to prevent NPE for accounts that have no roles assigned. Replace the ternary null propagation in AuthController with an explicit early return when the principal is not a UserAccount.User, making the intent clear and removing reliance on database null-equality behaviour.
|
womencodingcommunity
left a comment
womencodingcommunity
left a comment
There was a problem hiding this comment.
Both warnings addressed — great follow-through, Sonali! The null roles guard in TokenAuthFilter with the typed List.<SimpleGrantedAuthority>of() fallback is exactly right, and the explicit instanceof early return in AuthController.currentUser() makes the principal type contract clear and safe. The fix as a whole correctly resolves the principal type mismatch that was causing the original issue. Ready to merge!
3 participants
Description
Fixing the issue
Closes #561
Change Type
Screenshots
Pull request checklist
Please check if your PR fulfills the following requirements: