New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MM-893: Solved Authentication Bypass to System Administration #426
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jnsereko Nice work! Just a few small things and this LGTM.
|
||
private static final Logger log = LoggerFactory.getLogger(AdminAuthorisationFilter.class); | ||
|
||
private static final String COREAPPS_SYSTEM_ADMINISTRATOR_PRIVELEGE = "App: coreapps.systemAdministration"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small thing: we should probably put this in the CoreAppsConstants file like the other privileges.
omod/src/main/java/org/openmrs/module/coreapps/filter/AdminAuthorisationFilter.java
Outdated
Show resolved
Hide resolved
httpReq.getSession().setAttribute(WebConstants.DENIED_PAGE, httpReq.getRequestURI()); | ||
HttpServletResponse httpRes = (HttpServletResponse) res; | ||
log.info("User " + user + " has no privilage \"" + COREAPPS_SYSTEM_ADMINISTRATOR_PRIVELEGE + "\""); | ||
httpRes.sendRedirect(httpReq.getContextPath() + "/login.htm"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After calling sendRedirect()
it's best to have a return
so that we don't continue running this request through the filter chain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ibacher I really don't know how much to thank you. I think I have added the request correctly.
Hello @ibacher @isears @dkayiwa @sherrif10 Uploading 05.08.2021_00.20.57_REC.mp4… |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! Thanks @jnsereko! And thanks to @ibacher, @dkayiwa, @sherrif10 for helping come up with a high-quality solution.
I'll wait another few days for comments but otherwise unless there are objections I think this is ready to merge in.
public void doFilter(ServletRequest req, ServletResponse res, FilterChain chain) throws IOException, ServletException { | ||
HttpServletRequest httpReq = (HttpServletRequest) req; | ||
User user = Context.getAuthenticatedUser(); | ||
if (user != null && !user.hasPrivilege(CoreAppsConstants.PRIVILEGE_SYSTEM_ADMINISTRATOR)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when the user is null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when the user is null?
@dkayiwa I think this happens when the user has not logged in. He/she goes to the login page
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 @dkayiwa @ibacher @sherrif10