Tynamo
  1. Tynamo
  2. TYNAMO-75

Security module does not protect urls correctly

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: security-0.3.0
    • Fix Version/s: security-0.2.2, security-0.3.1
    • Component/s: security
    • Labels:
      None
    • Environment:
      NA
    • Number of attachments :
      0

      Description

      If you want to protect /admin then your ini file contains something like

      [urls]
      /admin/** = authc, roles[administrator]

      Unfortunately Tapestry will also allow /Admin, /ADmin etc. and the filter does not run for those urls. I think what needs to happen is create a TapestryPathMatchingFilterChainResolver

      like this

      public class TapestryPathMatchingFilterChainResolver extends PathMatchingFilterChainResolver {
      @Override
      protected boolean pathMatches(String pattern, String path)

      { return super.pathMatches(pattern.toLowerCase(), path.toLowerCase()); }

      }

      and use it instead of PathMatchingFilterChainResolver in the SecurityRequestFilter. Unfortunately I think that only solves half the problem because the authc filter will not match correctly either so you need a TapestryFormAuthenticationFilter like

      public class TapestryFormAuthenticationFilter extends FormAuthenticationFilter {
      @Override
      protected boolean pathsMatch(String pattern, String path)

      { return super.pathsMatch(pattern.toLowerCase(), path.toLowerCase()); }

      }

      and then override the authc filter with that implementation. I suspect this needs to be done for all the default filters.

        Activity

        Hide
        Barry Books added a comment - - edited

        I worked on the SecurtyRequestFilter and added the following:

        PathMatchingFilterChainResolver chainResolver = (PathMatchingFilterChainResolver) shiroFilter.getFilterChainResolver();
        FilterChainManager m = chainResolver.getFilterChainManager();
        chainResolver = null;
        if (chainResolver == null)

        { //FilterChainManager manager = new DefaultFilterChainManager(); //Expose the constructed FilterChainManager by first wrapping it in a // FilterChainResolver implementation. The ShiroFilter implementations // do not know about FilterChainManagers - only resolvers: chainResolver = new TapestryPathMatchingFilterChainResolver(); chainResolver.setFilterChainManager(m); shiroFilter.setFilterChainResolver(chainResolver); }

        It's ugly but it works. The problem is you really need to call new TapestryPathMatchingFilterChainResolver(filterConfig) but you don't have the filterConfig. You can't use the IniFilterChainResolverFactory because it's got the PathMatchingFilterChainResolver hardcoded. So you really need a TapestryIniFilterChainResolverFactory and override createDefaultInstance.

        Show
        Barry Books added a comment - - edited I worked on the SecurtyRequestFilter and added the following: PathMatchingFilterChainResolver chainResolver = (PathMatchingFilterChainResolver) shiroFilter.getFilterChainResolver(); FilterChainManager m = chainResolver.getFilterChainManager(); chainResolver = null; if (chainResolver == null) { //FilterChainManager manager = new DefaultFilterChainManager(); //Expose the constructed FilterChainManager by first wrapping it in a // FilterChainResolver implementation. The ShiroFilter implementations // do not know about FilterChainManagers - only resolvers: chainResolver = new TapestryPathMatchingFilterChainResolver(); chainResolver.setFilterChainManager(m); shiroFilter.setFilterChainResolver(chainResolver); } It's ugly but it works. The problem is you really need to call new TapestryPathMatchingFilterChainResolver(filterConfig) but you don't have the filterConfig. You can't use the IniFilterChainResolverFactory because it's got the PathMatchingFilterChainResolver hardcoded. So you really need a TapestryIniFilterChainResolverFactory and override createDefaultInstance.
        Hide
        Kalle Korhonen added a comment -

        Thanks Barry for digging into it! Three things:

        • pattern.toLowerCase() probably not needed
        • need to make this configurable
        • should be able to set a new PatternMatcher rather than overriding patchsMatch()

        otherwise, the approach is right and I definitely think this should be the default. Should be able crank out a new release pretty quick.

        Show
        Kalle Korhonen added a comment - Thanks Barry for digging into it! Three things: pattern.toLowerCase() probably not needed need to make this configurable should be able to set a new PatternMatcher rather than overriding patchsMatch() otherwise, the approach is right and I definitely think this should be the default. Should be able crank out a new release pretty quick.
        Hide
        Barry Books added a comment -

        I put toLowerCase() on both so a config like

        /Admin = authc

        would work. I looked at replacing PatternMatcher. It's a protected field with no get/set but I guess you could add some. It might be nice to have a regex pattern matcher.

        Show
        Barry Books added a comment - I put toLowerCase() on both so a config like /Admin = authc would work. I looked at replacing PatternMatcher. It's a protected field with no get/set but I guess you could add some. It might be nice to have a regex pattern matcher.
        Hide
        Kalle Korhonen added a comment -

        Sure, understand but in incurs an unnecessary repeated cost. It's not the right place, if we want to do it, we need to lowercase when reading the configuration in. For PatternMatcher, something like:
        chainResolver.setPathMatcher(new AntPathMatcher() {
        @Override
        public boolean match(String pattern, String string)

        { return super.match(pattern, string.toLowerCase()); }

        });

        Just looking at this - Shiro filters is still an issue as the resolver doesn't delegate the matcher to the filter. Likely will open issue against Shiro but will fix in tapestry-security immediately. Btw, there's a RegExPatternMatter built-in to Shiro.

        Show
        Kalle Korhonen added a comment - Sure, understand but in incurs an unnecessary repeated cost. It's not the right place, if we want to do it, we need to lowercase when reading the configuration in. For PatternMatcher, something like: chainResolver.setPathMatcher(new AntPathMatcher() { @Override public boolean match(String pattern, String string) { return super.match(pattern, string.toLowerCase()); } }); Just looking at this - Shiro filters is still an issue as the resolver doesn't delegate the matcher to the filter. Likely will open issue against Shiro but will fix in tapestry-security immediately. Btw, there's a RegExPatternMatter built-in to Shiro.
        Hide
        Kalle Korhonen added a comment -

        As an immediate fix, lowercased requestURI before comparison. 0.3.1-SNAPSHOT should be available shortly. Will merge to 0.2.x and push out releases for both branches

        Show
        Kalle Korhonen added a comment - As an immediate fix, lowercased requestURI before comparison. 0.3.1-SNAPSHOT should be available shortly. Will merge to 0.2.x and push out releases for both branches
        Hide
        Alejandro Scandroli added a comment -

        bulk closing issues that have been resolved for more than a year.

        Show
        Alejandro Scandroli added a comment - bulk closing issues that have been resolved for more than a year.

          People

          • Assignee:
            Kalle Korhonen
            Reporter:
            Barry Books
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: