Kernel
  1. Kernel
  2. KNL-428

add isVisible method to determine tool visibility in a site for a user

    Details

    • Type: Contributed Patch Contributed Patch
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.2.1
    • Component/s: API, Impl
    • Labels:
      None
    • Previous Issue Keys:

      Description

      The portal implements it's own method to check whether a given tool Placement is hidden or not. Suggest this or a similar method is implemented in the Kernel as this is a useful feature.

      Current methods from:
      portal/portal-impl/impl/src/java/org/sakaiproject/portal/charon/ToolHelperImpl.java

      public static final String TOOLCONFIG_REQUIRED_PERMISSIONS = "functions.require";

       public boolean isHidden(Placement placement)
      {
      if (placement == null) return true;
      String requiredPermissionsString = StringUtils.trimToNull(placement.getConfig().getProperty(TOOLCONFIG_REQUIRED_PERMISSIONS));
      if (requiredPermissionsString == null)
      return false;
      return requiredPermissionsString.contains("site.upd");
      }

      /**
      * The optional tool configuration tag "functions.require" describes a
      * set of permission lists which decide the visibility of the tool link
      * for this site user. Lists are separated by "|" and permissions within a
      * list are separated by ",". Users must have all the permissions included in
      * at least one of the permission lists.
      *
      * For example, a value like "section.role.student,annc.new|section.role.ta"
      * would let a user with "section.role.ta" see the tool, and let a user with
      * both "section.role.student" AND "annc.new" see the tool, but not let a user
      * who only had "section.role.student" see the tool.
      *
      * If the configuration tag is not set or is null, then all users see the tool.
      */
      public boolean allowTool(Site site, Placement placement)
      {
      // No way to render an opinion
      if (placement == null || site == null) return true;

      String requiredPermissionsString = placement.getConfig().getProperty(TOOLCONFIG_REQUIRED_PERMISSIONS);
      if (log.isDebugEnabled()) log.debug("requiredPermissionsString=" + requiredPermissionsString + " for " + placement.getToolId());
      if (requiredPermissionsString == null)
      return true;
      requiredPermissionsString = requiredPermissionsString.trim();
      if (requiredPermissionsString.length() == 0)
      return true;

      String[] allowedPermissionSets = requiredPermissionsString.split("\\|");
      for (int i = 0; i < allowedPermissionSets.length; i++)
      {
      String[] requiredPermissions = allowedPermissionSets[i].split(",");
      if (log.isDebugEnabled()) log.debug("requiredPermissions=" + Arrays.asList(requiredPermissions));
      boolean gotAllInList = true;
      for (int j = 0; j < requiredPermissions.length; j++)
      {
      if (!SecurityService.unlock(requiredPermissions[j].trim(), site.getReference()))
      {
      gotAllInList = false;
      break;
      }
      }
      if (gotAllInList)
      {
      return true;
      }
      }

      // No permission sets were matched.
      return false;
      }

        Issue Links

          Activity

          Hide
          Stephen Marquard added a comment -
          Why would this only check on site.upd ? I thought the point of functions.require is that you could use an arbitrary set of permissions.
          Show
          Stephen Marquard added a comment - Why would this only check on site.upd ? I thought the point of functions.require is that you could use an arbitrary set of permissions.
          Hide
          Steve Swinsburg added a comment - - edited
          If you hide a page using the Page Order tool, it sets site.upd as a required function for all tools in that page. When the portal is rendering, it checks if any tools on the page have the 'site.upd' functions set, and if so, hides the *entire* page.

          That method above is straight out of:
          portal/portal-impl/impl/src/java/org/sakaiproject/portal/charon/ToolHelperImpl.java

          So setting site.upd as a required function is the only way to hide a tool from showing in the portal (because of the above). Of course, each tool can use their own functions internally, and hide certain parts of themselves.

          See also SAK-18126.
          Show
          Steve Swinsburg added a comment - - edited If you hide a page using the Page Order tool, it sets site.upd as a required function for all tools in that page. When the portal is rendering, it checks if any tools on the page have the 'site.upd' functions set, and if so, hides the *entire* page. That method above is straight out of: portal/portal-impl/impl/src/java/org/sakaiproject/portal/charon/ToolHelperImpl.java So setting site.upd as a required function is the only way to hide a tool from showing in the portal (because of the above). Of course, each tool can use their own functions internally, and hide certain parts of themselves. See also SAK-18126 .
          Hide
          Steve Swinsburg added a comment - - edited
          Sorry, I should clarify, this method is what the portal uses to determine if the tool is hidden to normal users. It doesn't take into account the additional permissions that a tool can impose as a requirement before viewing the tool and hence page. There is also, public boolean allowTool(Site site, Placement placement) which does that.

          Perhaps these should be combined into one method that can report whether a tool/page is visible to given user in a site.
          Show
          Steve Swinsburg added a comment - - edited Sorry, I should clarify, this method is what the portal uses to determine if the tool is hidden to normal users. It doesn't take into account the additional permissions that a tool can impose as a requirement before viewing the tool and hence page. There is also, public boolean allowTool(Site site, Placement placement) which does that. Perhaps these should be combined into one method that can report whether a tool/page is visible to given user in a site.
          Hide
          Steve Swinsburg added a comment -
          Updated description to include additional method that is used to determine if a tool should be visible, based on it's functions.
          Show
          Steve Swinsburg added a comment - Updated description to include additional method that is used to determine if a tool should be visible, based on it's functions.
          Hide
          Steve Swinsburg added a comment -
          On reflection, of more use would be an isVisible() method which checks all permissions to see if the user is allowed to view the tool. Whether or not that should hide the entire parent page is a separate issue (and only affects the portal).
          Show
          Steve Swinsburg added a comment - On reflection, of more use would be an isVisible() method which checks all permissions to see if the user is allowed to view the tool. Whether or not that should hide the entire parent page is a separate issue (and only affects the portal).
          Hide
          Steve Swinsburg added a comment -
          Updated title. A tool may be invisible to users in one of two ways:

          1. It is hidden via the Page Order tool. This simply sets a functions.require=site.upd property in the database.
          2. The tool specifies it's own functions.require values in its tool registration, and the user lacks one or more of the permissions based on their role in the site.
          Show
          Steve Swinsburg added a comment - Updated title. A tool may be invisible to users in one of two ways: 1. It is hidden via the Page Order tool. This simply sets a functions.require=site.upd property in the database. 2. The tool specifies it's own functions.require values in its tool registration, and the user lacks one or more of the permissions based on their role in the site.
          Hide
          Steve Swinsburg added a comment -
          I have attached a patch which adds this functionality.

          The list of required permissions are retrieved for a tool in a site (functions.require), the list processed into the required sets and then each set checked via SecurityService.unlock(). If any of the required permissions in a set are not present for a user in a site, the tool is not visible.

          It is javadoc'd.

          This is modelled on:
          allowTool() from portal/portal-impl/impl/src/java/org/sakaiproject/portal/charon/ToolHelperImpl.java

          Here is the javadoc for reference:

          /**
          * Check whether a tool is visible to the current user in this site,
          * depending on permissions required to view the tool.
          *
          * The optional tool configuration tag "functions.require" describes a
          * set of permission lists which decide the visibility of the tool link
          * for this site user. Lists are separated by "|" and permissions within a
          * list are separated by ",". Users must have all the permissions included in
          * at least one of the permission lists.
          *
          * For example, a value like "section.role.student,annc.new|section.role.ta"
          * would let a user with "section.role.ta" see the tool, and let a user with
          * both "section.role.student" AND "annc.new" see the tool, but not let a user
          * who only had "section.role.student" see the tool.
          *
          * If the configuration tag is not set or is null, then all users see the tool.
          *
          * Based on: portal/portal-impl/impl/src/java/org/sakaiproject/portal/charon/ToolHelperImpl.java
          *
          * @param site Site this tool is in
          * @param config ToolConfiguration of the tool in the site
          * @return
          */
          public boolean isVisible(Site site, ToolConfiguration config);
          Show
          Steve Swinsburg added a comment - I have attached a patch which adds this functionality. The list of required permissions are retrieved for a tool in a site (functions.require), the list processed into the required sets and then each set checked via SecurityService.unlock(). If any of the required permissions in a set are not present for a user in a site, the tool is not visible. It is javadoc'd. This is modelled on: allowTool() from portal/portal-impl/impl/src/java/org/sakaiproject/portal/charon/ToolHelperImpl.java Here is the javadoc for reference: /** * Check whether a tool is visible to the current user in this site, * depending on permissions required to view the tool. * * The optional tool configuration tag "functions.require" describes a * set of permission lists which decide the visibility of the tool link * for this site user. Lists are separated by "|" and permissions within a * list are separated by ",". Users must have all the permissions included in * at least one of the permission lists. * * For example, a value like "section.role.student,annc.new|section.role.ta" * would let a user with "section.role.ta" see the tool, and let a user with * both "section.role.student" AND "annc.new" see the tool, but not let a user * who only had "section.role.student" see the tool. * * If the configuration tag is not set or is null, then all users see the tool. * * Based on: portal/portal-impl/impl/src/java/org/sakaiproject/portal/charon/ToolHelperImpl.java * * @param site Site this tool is in * @param config ToolConfiguration of the tool in the site * @return */ public boolean isVisible(Site site, ToolConfiguration config);
          Hide
          Matthew Buckett added a comment -
          Currently attached patch doesn't look right for OR permission checks, eg, when you have "section.role.student,annc.new|section.role.ta", here someone with section.role.ta won't be allowed to see the tool as you're returning false rather than falling through to the second set of permissions.

          + //since all in a set are required, if we are missing just one permission, deny access
          + if (!securityService().unlock(requiredPermissions[j].trim(), site.getReference())){
          + return false;
          + }
          Show
          Matthew Buckett added a comment - Currently attached patch doesn't look right for OR permission checks, eg, when you have "section.role.student,annc.new|section.role.ta", here someone with section.role.ta won't be allowed to see the tool as you're returning false rather than falling through to the second set of permissions. + //since all in a set are required, if we are missing just one permission, deny access + if (!securityService().unlock(requiredPermissions[j].trim(), site.getReference())){ + return false; + }
          Hide
          Matthew Buckett added a comment -
          I'm not sure pulling dependencies on Site and Security parts of the kernel into the ToolComponent is the right thing todo.
          Show
          Matthew Buckett added a comment - I'm not sure pulling dependencies on Site and Security parts of the kernel into the ToolComponent is the right thing todo.
          Hide
          Steve Swinsburg added a comment -
          Well spotted, I'll update the code to set a flag and continue to check the rest of the sets.

          If not the ToolManager, where else would be an appropriate place for it? It makes sense to me to have it here, but can move it if need be. This fix is required to resolve another important issue so need to resolve this quickly.
          Show
          Steve Swinsburg added a comment - Well spotted, I'll update the code to set a flag and continue to check the rest of the sets. If not the ToolManager, where else would be an appropriate place for it? It makes sense to me to have it here, but can move it if need be. This fix is required to resolve another important issue so need to resolve this quickly.
          Hide
          Steve Swinsburg added a comment -
          Attached revised patch.
          Show
          Steve Swinsburg added a comment - Attached revised patch.
          Hide
          Steve Swinsburg added a comment -
          Has anyone had a chance to review this? It works well. WIthout it, the same code is being duplicated in several places:

          portal - SAK-18164
          web services - SAK-18124
          basic lti - BLTI-34
          Show
          Steve Swinsburg added a comment - Has anyone had a chance to review this? It works well. WIthout it, the same code is being duplicated in several places: portal - SAK-18164 web services - SAK-18124 basic lti - BLTI-34
          Hide
          Stephen Marquard added a comment -
          This patch should go along with a corresponding patch to portal and webservices that removes the logic there in favour of calling this method, otherwise we'll have several places implementing the same logic which would be undesireable.
          Show
          Stephen Marquard added a comment - This patch should go along with a corresponding patch to portal and webservices that removes the logic there in favour of calling this method, otherwise we'll have several places implementing the same logic which would be undesireable.
          Hide
          Steve Swinsburg added a comment -
          If this gets in then I will pull it from the other places, it doesn't need to happen simultaneously. Those were added as a stopgap until something like this gets into kernel.

          I'm thinking though, that this logic should be incorporated into the actual list of tools that are returned automatically. So that the list returned is always cleaned, rather than having to clean it up afterwards.
          Show
          Steve Swinsburg added a comment - If this gets in then I will pull it from the other places, it doesn't need to happen simultaneously. Those were added as a stopgap until something like this gets into kernel. I'm thinking though, that this logic should be incorporated into the actual list of tools that are returned automatically. So that the list returned is always cleaned, rather than having to clean it up afterwards.
          Hide
          David Horwitz added a comment -
          Applied Steves patch - thanks. Will push a fresh snapshot shortly
          Show
          David Horwitz added a comment - Applied Steves patch - thanks. Will push a fresh snapshot shortly
          Hide
          Steve Swinsburg added a comment -
          Have assigned this to me and reopened so I can remove the duplicated code from the other locations.
          Show
          Steve Swinsburg added a comment - Have assigned this to me and reopened so I can remove the duplicated code from the other locations.
          Hide
          Charles Severance added a comment -
          Removing the redundant code tom ToolHandler.java in portal.
          Show
          Charles Severance added a comment - Removing the redundant code tom ToolHandler.java in portal.
          Hide
          Hudson CI Server added a comment -
          Integrated in portal trunk #1610 (See [http://builds.sakaiproject.org:8080/job/portal%20trunk/1610/])
              SAK-25506

          Redundant code removed - see also KNL-428. (Revision 132922)

               Result = SUCCESS
          Show
          Hudson CI Server added a comment - Integrated in portal trunk #1610 (See [ http://builds.sakaiproject.org:8080/job/portal%20trunk/1610/ ])      SAK-25506 Redundant code removed - see also KNL-428 . (Revision 132922)      Result = SUCCESS

            People

            • Assignee:
              Steve Swinsburg
              Reporter:
              Steve Swinsburg
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: