MM-893: Solved Authentication Bypass to System Administration #169
Conversation
| /** | ||
| * @see Filter#init(FilterConfig) | ||
| */ | ||
| public void init(FilterConfig filterConfig) throws ServletException { |
There was a problem hiding this comment.
Hey @sherrif10, I think It generally does nothing other than obeying java laws.
As far as I know, javax.servelt.Filter class is an interface so when implementing it, I have to implement all its methods otherwise the java-compiler will complain
There was a problem hiding this comment.
This mostly looks good to me, just had a few small adjustments.
@dkayiwa would you mind taking a look at this as well? This is a solution to the problem mentioned here (https://talk.openmrs.org/t/proposal-for-securing-the-legacy-ui-system-administration-pages/33988) although we took a slightly different approach to avoid having to edit every single admin page.
| public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain) throws IOException, ServletException { | ||
| HttpServletRequest httpReq = (HttpServletRequest) req; | ||
| String requestURI = httpReq.getRequestURI(); | ||
| if (requestURI.contains("/admin/")) { |
There was a problem hiding this comment.
Given that the filter is already limited to /admin/* in WebComponentRegistrar.java, is this check really necessary?
omod/src/main/java/org/openmrs/module/web/filter/AdminAuthorisationFilter.java
Show resolved
Hide resolved
| String requestURI = httpReq.getRequestURI(); | ||
| if (requestURI.contains("/admin/")) { | ||
| User user = Context.getAuthenticatedUser(); | ||
| if (user != null && !user.hasPrivilege("App: coreapps.systemAdministration")) { |
There was a problem hiding this comment.
How about those using the legacyui module without the reference application modules?
There was a problem hiding this comment.
Ah! I knew we were missing something! Could we instead implement this filter in the refapp module?
There was a problem hiding this comment.
Thank @isears and @dkayiwa for the rewie. I have really been thinking a lot about this.
what if we try something like
if (!(user.isSuperUser() || user.hasRole("Organizational: System Administrator") || user.hasRole("Organizational: Hospital Administrator")))
Will it also depend on the reference application
MM-893: Solved Authentication Bypass to System Administration
02.08.2021_17.30.39_REC.mp4Everything seems to be working fine |
| User user = Context.getAuthenticatedUser(); | ||
|
|
||
| if (!(user.isSuperUser() || user.hasRole("Organizational: System Administrator") || user.hasRole("Organizational: Hospital Administrator"))) { | ||
| httpReq.getSession().setAttribute(WebConstants.DENIED_PAGE, httpReq.getRequestURI()); |
There was a problem hiding this comment.
I think we have to revert to checking for only App: coreapps.systemAdministration, like you had before.
To resolve the issue that @dkayiwa mentioned, let's just check for the existence of the App: coreapps.systemAdministration privilege before we do anything else. So there will be three cases:
- The
App: coreapps.systemAdministrationprivilege exists, and the user has it. Therefore, the user should be allowed to view the /admin/* pages - The
App: coreapps.systemAdministrationprivilege exists, and the user does not have it. Therefore, the user should be prevented from viewing the /admin/* pages - The
App: coreapps.systemAdministrationdoes not exist, because the implementation is legacy-ui only. Therefore, we should let the user view the /admin/* pages to maintain backward compatibility.
There was a problem hiding this comment.
This looks very clear to me @isears and am sure it will work for us. I need to come up with a way of checking if the system has the App: coreapps.systemAdministration privilege
| public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain) throws IOException, ServletException { | ||
| HttpServletRequest httpReq = (HttpServletRequest) req; | ||
| Collection<Module> modules = ModuleFactory.getLoadedModules(); | ||
| if (modules.contains(ModuleFactory.getModuleById(COREAPPS_MODULE_ID))) { |
There was a problem hiding this comment.
hello @isears @sherrif10 @dkayiwa
@dkayiwa, won't this line cater for those using the legacyui module without reference application modules.
There was a problem hiding this comment.
Of course it would. But it is not a good practise for the legacyui module to be aware of any other modules.
|
It is still working fine 03.08.2021_15.42.16_REC.mp4 |
|
|
||
| private static final String COREAPPS_MODULE_ID = "coreapps"; | ||
|
|
||
| private static final String COREAPPS_SYSTEM_ADMINISTRATOR_PRIVELEGE = "App: coreapps.systemAdministration"; |
There was a problem hiding this comment.
I wish the privilege did not have coreapps in its name because the legacyui module should not be aware of any module.
|
i have created migrated this to coreapps basing on the discussion I had https://talk.openmrs.org/t/mm-893-solved-authentication-bypass-to-system-administration/34290/11 |
A user without the privilege could access the administration page and all other functionalities.
I have just created a Filter to redirect every user, without the above privilege, to the home page.
cc @isears @sherrif10
69ca3ccd-5a45-4d3b-bc40-cbcc4f576dc5.mp4