Uploaded image for project: '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
    • 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;
      }

        Gliffy Diagrams

          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:

                  Development

                    Git Source Code