fix(security): require administrator role for user and account management by Isaries · Pull Request #322 · WISE-Community/WISE-API

Isaries · 2026-07-06T15:31:55Z

Summary

Several user and account management endpoints under /admin were reachable by the researcher role through the broad /admin/** rule. A researcher could therefore grant themselves the administrator role or manage other users' accounts. This restricts those endpoints to administrators only.

Changes

Add @Secured("ROLE_ADMINISTRATOR") to the user, role, account, portal and news admin controllers.
Split the authorization rules so the sensitive paths (/admin/account/**, /admin/portal/**, /admin/news/**, /admin/mergeProjectMetadata, /admin/run/replacebase64withpng.html, /api/admin/**) require the administrator role. These more specific rules are listed before the broader /admin/** rule so they take precedence. Researchers keep access to the remaining /admin/** endpoints.

Testing

Builds on JDK 17 (mvnw package).
Method security is enabled with securedEnabled = true, so the annotations are active.

…ment The admin surface was gated by a single URL rule allowing both administrators and researchers, so a researcher could reach the user, role, portal and news management endpoints and grant themselves the administrator role, disable accounts, or read other users' details. Restrict those endpoints to administrators at the URL layer and add method-level enforcement on the account management controllers so the restriction does not depend on the URL configuration alone.

hirokiterashima

I don't think there's a need to add the comments in WebSecurityConfig- the code is clear enough.

Also, can you restrict access to those links (find user, enable/disable user, etc) from src/main/webapp/portal/admin/index.jsp using <sec:authorize>?

   <sec:authorize access="hasRole('ROLE_ADMINISTRATOR')">
      only admin users can see this
   </sec:authorize>

hirokiterashima · 2026-07-07T21:48:15Z

+        // User, role, portal, news and destructive maintenance endpoints must be
+        // restricted to administrators. They were previously reachable by researchers
+        // through the broad "/admin/**" rule, which let a researcher grant themselves
+        // the administrator role or manage other users' accounts. These more specific
+        // rules are listed first so they take precedence over the broader ones below.


OK to remove comments.

Suggested change

// User, role, portal, news and destructive maintenance endpoints must be

// restricted to administrators. They were previously reachable by researchers

// through the broad "/admin/**" rule, which let a researcher grant themselves

// the administrator role or manage other users' accounts. These more specific

// rules are listed first so they take precedence over the broader ones below.

The user management links in the admin index page pointed at endpoints that now require the administrator role, so a researcher would follow them into a 403. Wrap the whole user management section in <sec:authorize> instead, which also drops the nested check around the logged-in user lists. Require the administrator role for /admin/project/updatesharedprojects as well. The page only drives the /api/admin/project/shared endpoints, which administrators alone may call. Drop the WebSecurityConfig comments; the rules speak for themselves.

Isaries · 2026-07-09T15:07:43Z

Thanks for the review. Both addressed in 6c825b1.

Removed the comments in WebSecurityConfig.
Wrapped the user management section of src/main/webapp/portal/admin/index.jsp in <sec:authorize access="hasRole('ROLE_ADMINISTRATOR')">. Every link in that section (find user, list all users, enable/disable user, batch create accounts) is administrator-only now, so I wrapped the section header and content as a whole rather than each link individually, and removed the nested <sec:authorize> around the logged-in user lists since it became redundant. This way a researcher does not see an empty heading.

Two notes on the /api/admin/** rule this PR adds. There is no anyRequest() rule in WebSecurityConfig, and /admin/** does not match /api/admin/..., so before this change those endpoints had no URL-level authorization at all. In practice UpdateProjectSharedPermissionsAPIController guards both handlers with its own signedInUser.isAdmin() check, so I do not believe anything was exposed, but the URL rule and the @Secured annotation now make that explicit.

I also noticed /admin/project/updatesharedprojects itself was still reachable by researchers through the broad /admin/** rule, even though the link is hidden by <sec:authorize>. I added the path to the administrator rule and put @Secured on UpdateSharedProjectsController. Happy to drop that if you would rather keep this PR narrow.

Unrelated to this PR: /admin/memorymonitor.html in index.jsp has no controller mapping, view, or handler mapping anywhere in the repo, so that link looks dead. I left it alone.

The admin-only rules for /admin/mergeProjectMetadata and /admin/run/replacebase64withpng.html were exact ant patterns. Spring MVC matches a trailing slash and routes /admin/mergeProjectMetadata/ to the controller, but the ant matcher does not, so the request fell through to the broader /admin/** rule and a researcher reached both endpoints. Match these paths the way Spring MVC matches them, and secure the two controllers directly so the rules are not the only guard.

Isaries · 2026-07-09T15:29:27Z

While re-reading the rules I added, I found that two of them could be bypassed, so c4f9e4b0f fixes that.

/admin/mergeProjectMetadata and /admin/run/replacebase64withpng.html are exact patterns, not /** prefixes. Spring MVC's useTrailingSlashMatch is on by default in Spring 5.3, so GET /admin/mergeProjectMetadata/ still routes to AdminUtilsController, but AntPathRequestMatcher("/admin/mergeProjectMetadata") does not match that URL. The request fell through to .antMatchers("/admin/**").hasAnyRole("ADMINISTRATOR", "RESEARCHER") and a researcher reached both endpoints, which is exactly what this PR is meant to prevent. Neither controller had an @Secured annotation or its own admin check, so the URL rule was the only guard.

I confirmed the matcher behavior with a throwaway test before fixing it:

uri=/admin/mergeProjectMetadata    ant=true   mvc=true
uri=/admin/mergeProjectMetadata/   ant=false  mvc=true

The fix switches that rule from antMatchers to mvcMatchers, so the security matcher and the request mapping agree on path matching by construction, and adds @Secured("ROLE_ADMINISTRATOR") to AdminUtilsController and ReplaceBase64WithPNGController so the URL rule is not the only guard. /admin/project/updatesharedprojects is an exact pattern too, but it already picked up @Secured in the previous commit.

One thing I want to check with you. I put a two line comment above the mvcMatchers call saying why it is mvcMatchers and not antMatchers, which goes against your earlier note about comments in this file. My reasoning is that this one records a constraint the code cannot show: swapping it back to antMatchers reopens the bypass silently. Happy to drop it if you would rather keep the file comment free.

Two things I noticed but did not touch, since they are outside the scope of this PR. Let me know if you want either as a follow up.

authorizeRequests() has no terminal .anyRequest() rule, so any URL matching none of the listed patterns has no authorization rule at all. There is no general /api/** rule, so those endpoints rely entirely on method security. That is the underlying reason /api/admin/** had to be added by hand here, and the same trap is set for the next endpoint someone adds.
There is no test anywhere in src/test that exercises authorization. ShowAllUsersControllerTest calls new ShowAllUsersController() directly, which bypasses the proxy, so @Secured is never in its call path. A MockMvc test asserting that a researcher gets 403 on /admin/account/** and 200 on /admin/run/stats would keep this boundary from regressing.

Addressed suggestions/issues. We'll keep the comments because they explain the reason for the code.

hirokiterashima · 2026-07-09T21:24:18Z

LGTM. Thanks for making those changes. Keeping the two-line comment is fine as it explains why we need the code.

It would be great to address the additional issues you raised, especially the first one.

hirokiterashima · 2026-07-09T21:31:29Z

🎉 This PR is included in version 1.21.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Isaries mentioned this pull request Jul 6, 2026

Security hardening from a code audit: several small, self-contained PRs #328

Open

hirokiterashima previously requested changes Jul 7, 2026

View reviewed changes

hirokiterashima merged commit 59d194a into WISE-Community:develop Jul 9, 2026
1 check passed

hirokiterashima self-assigned this Jul 9, 2026

hirokiterashima added the released label Jul 9, 2026

This was referenced Jul 11, 2026

test(security): cover the admin-only authorization boundary #331

Open

fix(security): deny unauthenticated access by default #326

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(security): require administrator role for user and account management#322

fix(security): require administrator role for user and account management#322
hirokiterashima merged 3 commits into
WISE-Community:developfrom
Isaries:fix/admin-role-authorization

Isaries commented Jul 6, 2026

Uh oh!

hirokiterashima left a comment

Uh oh!

hirokiterashima Jul 7, 2026

Uh oh!

Isaries commented Jul 9, 2026

Uh oh!

Isaries commented Jul 9, 2026

Uh oh!

hirokiterashima commented Jul 9, 2026

Uh oh!

Uh oh!

hirokiterashima commented Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

Isaries commented Jul 6, 2026

Summary

Changes

Testing

Uh oh!

hirokiterashima left a comment

Choose a reason for hiding this comment

Uh oh!

hirokiterashima Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

Isaries commented Jul 9, 2026

Uh oh!

Isaries commented Jul 9, 2026

Uh oh!

hirokiterashima commented Jul 9, 2026

Uh oh!

Uh oh!

hirokiterashima commented Jul 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants