From 960f69ca745b5d1763302b5789277f58320d57e4 Mon Sep 17 00:00:00 2001 From: Bill Smith Date: Fri, 31 Jan 2025 15:45:26 -0700 Subject: [PATCH] Updated permissions handling. Additional logging. (#397) * Updated permissions handling. Additional logging. Also includes a smidge of logging / TODO notes around hibernate / cache weirdness experienced while working on this commit. * Fixed imports. My IDE's settings for switching direct imports to '*' was set too low, so I undid the '*' import but forgot to fix the missing direct imports. * chore: change pom parent (#377) Co-authored-by: Bill Smith * Updated permissions handling. Additional logging. Also includes a smidge of logging / TODO notes around hibernate / cache weirdness experienced while working on this commit. * Fixed imports. My IDE's settings for switching direct imports to '*' was set too low, so I undid the '*' import but forgot to fix the missing direct imports. --------- Co-authored-by: Benito Gonzalez --- .../newsreader/dao/HibernateNewsStore.java | 5 ++ .../portlet/reader/AdminNewsController.java | 27 +++++++- .../reader/EditUserRomeController.java | 65 +++++++++++++++++-- .../service/SharedNewsSetServiceImpl.java | 8 ++- src/main/webapp/WEB-INF/jsp/editNewsUrl.jsp | 6 ++ 5 files changed, 101 insertions(+), 10 deletions(-) diff --git a/src/main/java/org/jasig/portlet/newsreader/dao/HibernateNewsStore.java b/src/main/java/org/jasig/portlet/newsreader/dao/HibernateNewsStore.java index b1993a9d..b41553e1 100644 --- a/src/main/java/org/jasig/portlet/newsreader/dao/HibernateNewsStore.java +++ b/src/main/java/org/jasig/portlet/newsreader/dao/HibernateNewsStore.java @@ -356,6 +356,11 @@ public NewsSet getNewsSet(String userId, String setName) { NewsSet set = (NewsSet) q.uniqueResult(); if (logger.isDebugEnabled()) { logger.debug(this.getSessionFactory().getStatistics().toString()); + if (set != null) { + logger.debug("found " + set.getNewsConfigurations().size() + " news configurations"); + } else { + logger.debug("no news configurations found"); + } } return set; diff --git a/src/main/java/org/jasig/portlet/newsreader/mvc/portlet/reader/AdminNewsController.java b/src/main/java/org/jasig/portlet/newsreader/mvc/portlet/reader/AdminNewsController.java index 3c0bdb1b..0a5f56dd 100644 --- a/src/main/java/org/jasig/portlet/newsreader/mvc/portlet/reader/AdminNewsController.java +++ b/src/main/java/org/jasig/portlet/newsreader/mvc/portlet/reader/AdminNewsController.java @@ -20,10 +20,16 @@ import java.util.HashMap; import java.util.Map; +import java.util.Set; +import javax.portlet.ActionRequest; +import javax.portlet.PortletRequest; +import javax.portlet.PortletSecurityException; import javax.portlet.RenderRequest; -import javax.portlet.RenderResponse; +import javax.servlet.http.HttpServletRequest; +import org.jasig.portlet.newsreader.mvc.AbstractNewsController; +import org.jasig.portlet.newsreader.service.RolesService; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.jasig.portlet.newsreader.PredefinedNewsDefinition; @@ -35,6 +41,8 @@ import org.springframework.web.portlet.ModelAndView; import org.springframework.web.portlet.bind.annotation.ActionMapping; import org.springframework.web.portlet.bind.annotation.RenderMapping; +import org.springframework.web.portlet.context.PortletApplicationContextUtils; +import org.springframework.web.portlet.util.PortletUtils; /** @@ -54,7 +62,13 @@ public class AdminNewsController { private NewsStore newsStore; @RenderMapping(params="action=administration") - public ModelAndView getAdminView(RenderRequest request,RenderResponse response) { + public ModelAndView getAdminView(RenderRequest request) throws PortletSecurityException { + if (!request.isUserInRole(AbstractNewsController.NEWS_ADMIN_ROLE)) { + log.warn("User [ {} ] with IP [ {} ] tried to access news administration!", + request.getRemoteUser(), + request.getProperty("REMOTE_ADDR")); + throw new PortletSecurityException("User does not have required admin role"); + } log.debug("Entering news admin"); @@ -67,7 +81,14 @@ public ModelAndView getAdminView(RenderRequest request,RenderResponse response) } @ActionMapping(params="action=deletePredefinedFeed") - public void deleteFeed(@RequestParam("id") Long id) { + public void deleteFeed(@RequestParam("id") Long id, ActionRequest request) throws PortletSecurityException { + if (!request.isUserInRole(AbstractNewsController.NEWS_ADMIN_ROLE)) { + log.warn("User [ {} ] with IP [ {} ] tried to access news administration!", + request.getRemoteUser(), + request.getProperty("REMOTE_ADDR")); + throw new PortletSecurityException("User does not have required admin role"); + } + PredefinedNewsDefinition def = newsStore.getPredefinedNewsDefinition(id); newsStore.deleteNewsDefinition(def); } diff --git a/src/main/java/org/jasig/portlet/newsreader/mvc/portlet/reader/EditUserRomeController.java b/src/main/java/org/jasig/portlet/newsreader/mvc/portlet/reader/EditUserRomeController.java index 8ac829e3..8e4cef52 100644 --- a/src/main/java/org/jasig/portlet/newsreader/mvc/portlet/reader/EditUserRomeController.java +++ b/src/main/java/org/jasig/portlet/newsreader/mvc/portlet/reader/EditUserRomeController.java @@ -21,12 +21,17 @@ import javax.portlet.ActionRequest; import javax.portlet.ActionResponse; import javax.portlet.PortletRequest; +import javax.portlet.PortletURL; +import javax.portlet.RenderResponse; +import org.apache.commons.lang.StringUtils; import org.jasig.portlet.newsreader.NewsConfiguration; +import org.jasig.portlet.newsreader.PredefinedNewsDefinition; import org.jasig.portlet.newsreader.UserDefinedNewsConfiguration; import org.jasig.portlet.newsreader.UserDefinedNewsDefinition; import org.jasig.portlet.newsreader.adapter.RomeAdapter; import org.jasig.portlet.newsreader.dao.NewsStore; +import org.jasig.portlet.newsreader.mvc.AbstractNewsController; import org.jasig.portlet.newsreader.mvc.NewsListingCommand; import org.jasig.portlet.newsreader.service.NewsSetResolvingService; import org.slf4j.Logger; @@ -96,7 +101,32 @@ public NewsListingCommand getNewsForm(PortletRequest request) throws Exception { } @RenderMapping(params = "action=editUrl") - public String getUserEditView(PortletRequest request) { + public String getUserEditView(PortletRequest request, RenderResponse response) { + log.debug("Returning editNewsUrl view"); + + // get the to-be-edited news configuration id + String[] formIdValues = request.getParameterMap().get("id"); + String formId = null; + if (formIdValues != null && formIdValues.length > 0) { + formId = formIdValues[0]; + } + + // if user doesn't have permissions, redirect + if (StringUtils.isNotBlank(formId)) { + long lFormId = Long.parseLong(formId); + if (lFormId > -1) { + if (!canEditNewsConfiguration(request, lFormId)) { + log.warn("User [ {} ] with IP [ {} ] tried to edit news configuration [ {} ] without permission!", + request.getRemoteUser(), + request.getProperty("REMOTE_ADDR"), + lFormId); + PortletURL redirectUrl = response.createRenderURL(); + redirectUrl.setParameter("action", "editPreferences"); + request.setAttribute("redirectUrl", redirectUrl.toString()); + } + } + } + return "editNewsUrl"; } @@ -110,11 +140,19 @@ public void onSubmitAction(ActionRequest request, ActionResponse response, if (form.getId() > -1) { - config = (UserDefinedNewsConfiguration) newsStore.getNewsConfiguration(form.getId()); - definition = (UserDefinedNewsDefinition) config.getNewsDefinition(); - definition.addParameter("url", form.getUrl()); - definition.setName(form.getName()); - log.debug("Updating"); + if (canEditNewsConfiguration(request, form.getId())) { + config = (UserDefinedNewsConfiguration) newsStore.getNewsConfiguration(form.getId()); + log.debug("User [ {} ] is updating news", request.getRemoteUser()); + definition = (UserDefinedNewsDefinition) config.getNewsDefinition(); + definition.addParameter("url", form.getUrl()); + definition.setName(form.getName()); + } else { + log.warn("User [ {} ] with IP [ {} ] tried to edit news configuration [ {} ] without permission!", + request.getRemoteUser(), + request.getProperty("REMOTE_ADDR"), + form.getId()); + return; + } } else { @@ -143,4 +181,19 @@ public void onSubmitAction(ActionRequest request, ActionResponse response, } + private boolean isPredefinedNewsConfiguration(NewsConfiguration newsConfiguration) { + return newsConfiguration.getNewsDefinition() instanceof PredefinedNewsDefinition; + } + + private boolean canEditNewsConfiguration(PortletRequest request, long configurationId) { + boolean isAdmin = request.isUserInRole(AbstractNewsController.NEWS_ADMIN_ROLE); + NewsConfiguration configuration = newsStore.getNewsConfiguration(configurationId); + if (isPredefinedNewsConfiguration(configuration)) { + return isAdmin; + } else { + UserDefinedNewsConfiguration userConfiguration = (UserDefinedNewsConfiguration) configuration; + return isAdmin || userConfiguration.getNewsSet().getUserId().equals(request.getRemoteUser()); + } + } + } diff --git a/src/main/java/org/jasig/portlet/newsreader/service/SharedNewsSetServiceImpl.java b/src/main/java/org/jasig/portlet/newsreader/service/SharedNewsSetServiceImpl.java index 5c953820..92e2ca20 100644 --- a/src/main/java/org/jasig/portlet/newsreader/service/SharedNewsSetServiceImpl.java +++ b/src/main/java/org/jasig/portlet/newsreader/service/SharedNewsSetServiceImpl.java @@ -77,13 +77,19 @@ public NewsSet getNewsSet(String fname, PortletRequest request) { set.setUserId(userId); set.setName(fname); newsStore.storeNewsSet(set); + //TODO: the persisted set (line above) isn't always available to the line below. Hibernate being lazy? set = newsStore.getNewsSet(userId, fname); // get set_id } // Persistent set is now loaded but may still need re-initalising since last use. // by adding setId to session, we signal that initialisation has taken place. if (session.getAttribute("setId", PortletSession.PORTLET_SCOPE) == null) { - logger.debug("re-initalising loaded newsSet "+set.getName()); + if (set != null) { + logger.debug("re-initalising loaded newsSet " + set.getName()); + } else { + logger.debug("attempting to re-initialize loaded newsSet, but it is null"); + } + @SuppressWarnings("unchecked") final Set roles = rolesService.getUserRoles(request); diff --git a/src/main/webapp/WEB-INF/jsp/editNewsUrl.jsp b/src/main/webapp/WEB-INF/jsp/editNewsUrl.jsp index 084f46a4..18dc2d54 100644 --- a/src/main/webapp/WEB-INF/jsp/editNewsUrl.jsp +++ b/src/main/webapp/WEB-INF/jsp/editNewsUrl.jsp @@ -25,6 +25,12 @@ + + + +