Skip to content

bug: authorization is no longer hardcoded#14

Merged
Youssef-codin merged 2 commits intomainfrom
bug-roles
Mar 17, 2026
Merged

bug: authorization is no longer hardcoded#14
Youssef-codin merged 2 commits intomainfrom
bug-roles

Conversation

@Youssef-codin
Copy link
Copy Markdown
Owner

@Youssef-codin Youssef-codin commented Mar 17, 2026

Summary by CodeRabbit

  • Bug Fixes
    • Fixed role-to-authority mapping so admin privileges are reliably granted when appropriate.
    • Removed duplicate authorities to ensure consistent and predictable access control across the app.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 17, 2026

📝 Walkthrough

Walkthrough

The buildUserDetails method in AppUserDetails now expands each role into one or more authorities using flatMap, adding the role's name and conditionally an ADMIN authority when role.isAdmin() is true, then removes duplicates before collecting.

Changes

Cohort / File(s) Summary
Authority aggregation
src/main/java/com/smf/security/AppUserDetails.java
Reworked buildUserDetails to use flatMap to emit the role's authority and, if role.isAdmin() is true, an additional ADMIN authority; duplicates are filtered with distinct() before collection. Previously used a single map producing one authority per role.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hop through roles both small and grand,
I add a tag when admin takes a stand,
I weed out twins and keep the list bright,
Each role now shines with proper light. ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: removing hardcoded authorization by implementing dynamic role-based authority expansion with admin detection.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch bug-roles
📝 Coding Plan
  • Generate coding plan for human review comments

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

You can enable review details to help with troubleshooting, context usage and more.

Enable the reviews.review_details setting to include review details such as the model used, the time taken for each step and more in the review comments.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/main/java/com/smf/security/AppUserDetails.java`:
- Around line 21-30: The authorities list may contain duplicate "ADMIN" entries
when multiple roles have isAdmin() true; update the logic in AppUserDetails that
builds the authorities (currently using user.getRoles().stream().flatMap(...)
collecting to List) to deduplicate entries—e.g., emit authorities as a Stream of
SimpleGrantedAuthority for each role name and the "ADMIN" marker and then apply
distinct() (or collect into a Set and convert to List) before assigning to the
authorities variable so each granted authority (including "ADMIN") appears only
once.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 830717e0-6b57-45e6-9c9d-26e8f87f0e45

📥 Commits

Reviewing files that changed from the base of the PR and between c7d0e63 and 686fd32.

📒 Files selected for processing (1)
  • src/main/java/com/smf/security/AppUserDetails.java

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/main/java/com/smf/security/AppUserDetails.java`:
- Line 28: Replace the hardcoded "ADMIN" literal used when creating the granted
authority in AppUserDetails (the line roleAuths.add(new
SimpleGrantedAuthority("ADMIN"))) with a shared constant; define a single public
constant (for example ADMIN = "ADMIN") in a central place such as an
AuthorityConstants class or a Role enum, import and reference that constant
inside AppUserDetails (and update other usages in filters, annotations, and
tests to use the same constant) so all authorities are driven from one canonical
symbol instead of repeated string literals.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2d5b2480-cff0-4f97-85a6-7ebceb9cc8fd

📥 Commits

Reviewing files that changed from the base of the PR and between 686fd32 and 9ee21c2.

📒 Files selected for processing (1)
  • src/main/java/com/smf/security/AppUserDetails.java

@Youssef-codin Youssef-codin merged commit a28eee1 into main Mar 17, 2026
2 checks passed
@Youssef-codin Youssef-codin deleted the bug-roles branch March 18, 2026 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant