Uploaded image for project: 'Solr'
  1. Solr
  2. SOLR-7692

Implement BasicAuth based impl for the new Authentication/Authorization APIs

    Details

    • Type: New Feature
    • Status: Closed
    • Priority: Blocker
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.3, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      This involves various components

      Authentication

      A basic auth based authentication filter. This should retrieve the user credentials from ZK. The user name and sha1 hash of password should be stored in ZK

      sample authentication json

      {
        "authentication":{
      
          "class": "solr.BasicAuthPlugin",
          "users" :{
            "john" :"09fljnklnoiuy98 buygujkjnlk",
            "david":"f678njfgfjnklno iuy9865ty",
            "pete": "87ykjnklndfhjh8 98uyiy98",
             }
        }
      }
      

      authorization plugin

      This would store the roles of various users and their privileges in ZK

      sample authorization.json

      {
        "authorization": {
          "class": "solr.ZKAuthorization",
         "user-role" :{
        "john" : ["admin", "guest"]
        "tom" : 'dev'
         }
          "permissions": [
             {"name":"collection-edit",
               "role": "admin" 
             },
             {"name":"coreadmin",
               "role":"admin"
             },
             {"name": "mycoll_update",
              "collection": "mycoll",
              "path":["/update/*"],
              "role": ["guest","admin"]
            }]
          }
        }
      }
      

      We will also need to provide APIs to create users and assign them roles

      1. SOLR-7692.patch
        29 kB
        Noble Paul
      2. SOLR-7692.patch
        33 kB
        Noble Paul
      3. SOLR-7692.patch
        70 kB
        Noble Paul
      4. SOLR-7692.patch
        111 kB
        Noble Paul
      5. SOLR-7692.patch
        118 kB
        Noble Paul
      6. SOLR-7692.patch
        122 kB
        Noble Paul
      7. SOLR-7692.patch
        122 kB
        Noble Paul
      8. SOLR-7692.patch
        122 kB
        Noble Paul
      9. SOLR-7692.patch
        125 kB
        Noble Paul
      10. SOLR-7692.patch
        128 kB
        Noble Paul
      11. SOLR-7757.patch
        64 kB
        Noble Paul
      12. SOLR-7692.patch
        63 kB
        Noble Paul
      13. SOLR-7692.patch
        64 kB
        Anshum Gupta
      14. SOLR-7757.patch
        64 kB
        Anshum Gupta
      15. SOLR-7692.patch
        64 kB
        Ishan Chattopadhyaya
      16. SOLR-7757.patch
        63 kB
        Ishan Chattopadhyaya
      17. SOLR-7692.patch
        64 kB
        Anshum Gupta
      18. SOLR-7692.patch
        64 kB
        Ishan Chattopadhyaya

        Issue Links

          Activity

          Hide
          shalinmangar Shalin Shekhar Mangar added a comment -

          Bulk close for 5.3.0 release

          Show
          shalinmangar Shalin Shekhar Mangar added a comment - Bulk close for 5.3.0 release
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment - - edited

          1. In Sha256AuthenticationProvider, line 106

              try {
                digest = MessageDigest.getInstance("SHA-256");
              } catch (NoSuchAlgorithmException e) {
                BasicAuthPlugin.log.error(e.getMessage(), e);
                return null;//should not happen
              }
          

          Shouldn't this be an exception, e.g. SolrException, thrown?

          2. In the sha256() method (same place as above),

          public static String sha256(String password, String saltKey) {
              MessageDigest digest;
              try {
                digest = MessageDigest.getInstance("SHA-256");
              } catch (NoSuchAlgorithmException e) {
                BasicAuthPlugin.log.error(e.getMessage(), e);
                return null;//should not happen
             }
              if (saltKey != null) {
                digest.reset();
                digest.update(Base64.decodeBase64(saltKey));
              }
          
              byte[] btPass = digest.digest(password.getBytes(StandardCharsets.UTF_8));
              digest.reset();
              btPass = digest.digest(btPass);
              return Base64.encodeBase64String(btPass);
            }
          

          I think we should reuse a digest instance, instead of creating one using the factory method for every request, as there are significant overheads to creating a new digest algorithm instance.
          Reference: https://books.google.co.in/books?id=42etT_9-_9MC&pg=PT254&lpg=PT254

          3. For SolrJ support, I've added SOLR-7839.

          4. For internode communication, I think (please correct me if I'm wrong) the ThreadLocal approach won't work for cases when the internode request is made from a threadpool, from where the headers of the original request thread's ThreadLocal won't be accessible. I think we need something like SOLR-6625, where the request object can store the user principal / headers etc. and pass it along to the request interceptor as a context.

          5. As per our discussion offline, the internode request which are originated from a Solr node (not a subrequest of a main user request) cannot be secured this way. Either each node uses its own principal/credentials to send internode requests in such cases, or there's another secure mechanism of internode requests internal to Solr (e.g. asymmetric cryptographic mechanism, e.g. PKI), irrespective of the authc plugins used for user requests.

          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - - edited 1. In Sha256AuthenticationProvider, line 106 try { digest = MessageDigest.getInstance("SHA-256"); } catch (NoSuchAlgorithmException e) { BasicAuthPlugin.log.error(e.getMessage(), e); return null;//should not happen } Shouldn't this be an exception, e.g. SolrException, thrown? 2. In the sha256() method (same place as above), public static String sha256(String password, String saltKey) { MessageDigest digest; try { digest = MessageDigest.getInstance("SHA-256"); } catch (NoSuchAlgorithmException e) { BasicAuthPlugin.log.error(e.getMessage(), e); return null;//should not happen } if (saltKey != null) { digest.reset(); digest.update(Base64.decodeBase64(saltKey)); } byte[] btPass = digest.digest(password.getBytes(StandardCharsets.UTF_8)); digest.reset(); btPass = digest.digest(btPass); return Base64.encodeBase64String(btPass); } I think we should reuse a digest instance, instead of creating one using the factory method for every request, as there are significant overheads to creating a new digest algorithm instance. Reference: https://books.google.co.in/books?id=42etT_9-_9MC&pg=PT254&lpg=PT254 3. For SolrJ support, I've added SOLR-7839 . 4. For internode communication, I think (please correct me if I'm wrong) the ThreadLocal approach won't work for cases when the internode request is made from a threadpool, from where the headers of the original request thread's ThreadLocal won't be accessible. I think we need something like SOLR-6625 , where the request object can store the user principal / headers etc. and pass it along to the request interceptor as a context. 5. As per our discussion offline, the internode request which are originated from a Solr node (not a subrequest of a main user request) cannot be secured this way. Either each node uses its own principal/credentials to send internode requests in such cases, or there's another secure mechanism of internode requests internal to Solr (e.g. asymmetric cryptographic mechanism, e.g. PKI), irrespective of the authc plugins used for user requests.
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment -

          Let's separate out the authentication and authorization patches into different issues. As they are orthogonal, we should commit them separately.

          +1. However, although I agree that we should separate out the authc and authz parts into different issues, the current integration test (BasicAuthIntegrationTest) would need to be rewritten as it is using both the plugins together.

          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - Let's separate out the authentication and authorization patches into different issues. As they are orthogonal, we should commit them separately. +1. However, although I agree that we should separate out the authc and authz parts into different issues, the current integration test (BasicAuthIntegrationTest) would need to be rewritten as it is using both the plugins together.
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment -

          Updated the patch, fixing the test failure for BasicAuthIntegrationTest.

          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - Updated the patch, fixing the test failure for BasicAuthIntegrationTest.
          Hide
          anshumg Anshum Gupta added a comment -

          I've also moved the patch for SOLR-7757 to it's own issue.

          Let's separate out the authentication and authorization patches into different issues. As they are orthogonal, we should commit them separately.

          P.S: The unit test still fails consistently for me.

          Show
          anshumg Anshum Gupta added a comment - I've also moved the patch for SOLR-7757 to it's own issue. Let's separate out the authentication and authorization patches into different issues. As they are orthogonal, we should commit them separately. P.S: The unit test still fails consistently for me.
          Hide
          anshumg Anshum Gupta added a comment -

          BasicAuthPlugin no longer extends javax.servlet.Filter.

          Show
          anshumg Anshum Gupta added a comment - BasicAuthPlugin no longer extends javax.servlet.Filter .
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment - - edited

          The SOLR-7757 patch still had compilation errors, since it depended on getValueMap() of RuleBasedAuthorizationPlugin, which is present in SOLR-7692.

          Fixed that, and added patches based on the latest patches by Anshum. SOLR-7757 patch compiles fine now, in isolation.

          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - - edited The SOLR-7757 patch still had compilation errors, since it depended on getValueMap() of RuleBasedAuthorizationPlugin, which is present in SOLR-7692 . Fixed that, and added patches based on the latest patches by Anshum. SOLR-7757 patch compiles fine now, in isolation.
          Hide
          anshumg Anshum Gupta added a comment -

          Updated patch with some cleanup. I'm still hitting a test failure consistently with both the patches applied:

          ant test -Dtestcase=BasicAuthIntegrationTest -Dtests.method=testBasics -Dtests.seed=ADE11563338421BA -Dtests.slow=true -Dtests.locale=hi_IN -Dtests.timezone=America/Mazatlan -Dtests.asserts=true -Dtests.file.encoding=UTF-8

          Also, does BasicAuthPlugin need to implement javax.servlet.Filter?

          Show
          anshumg Anshum Gupta added a comment - Updated patch with some cleanup. I'm still hitting a test failure consistently with both the patches applied: ant test -Dtestcase=BasicAuthIntegrationTest -Dtests.method=testBasics -Dtests.seed=ADE11563338421BA -Dtests.slow=true -Dtests.locale=hi_IN -Dtests.timezone=America/Mazatlan -Dtests.asserts=true -Dtests.file.encoding=UTF-8 Also, does BasicAuthPlugin need to implement javax.servlet.Filter ?
          Hide
          anshumg Anshum Gupta added a comment - - edited

          I'm getting the following compilation error on applying SOLR-7757.patch independently so breaking up wouldn't make sense unless at least one of them can compile/runs independently. Once that happens, it'd make sense for the second patch to depend on the first.

              [mkdir] Created dir: /Users/anshumgupta/workspace/lucene-trunk3/solr/build/solr-core/classes/java
              [javac] Compiling 854 source files to /Users/anshumgupta/workspace/lucene-trunk3/solr/build/solr-core/classes/java
              [javac] /Users/anshumgupta/workspace/lucene-trunk3/solr/core/src/java/org/apache/solr/handler/admin/SecurityConfHandler.java:36: error: cannot find symbol
              [javac] import org.apache.solr.security.RuleBasedAuthorizationPlugin;
              [javac]                                ^
              [javac]   symbol:   class RuleBasedAuthorizationPlugin
              [javac]   location: package org.apache.solr.security
              [javac] /Users/anshumgupta/workspace/lucene-trunk3/solr/core/src/java/org/apache/solr/handler/admin/SecurityConfHandler.java:101: error: cannot find symbol
              [javac]         Map meta = RuleBasedAuthorizationPlugin.getMapValue(out, "");
              [javac]                    ^
              [javac]   symbol:   variable RuleBasedAuthorizationPlugin
              [javac]   location: class SecurityConfHandler
              [javac] Note: Some input files use or override a deprecated API.
              [javac] Note: Recompile with -Xlint:deprecation for details.
              [javac] Note: Some input files use unchecked or unsafe operations.
              [javac] Note: Recompile with -Xlint:unchecked for details.
              [javac] 2 errors
          
          Show
          anshumg Anshum Gupta added a comment - - edited I'm getting the following compilation error on applying SOLR-7757 .patch independently so breaking up wouldn't make sense unless at least one of them can compile/runs independently. Once that happens, it'd make sense for the second patch to depend on the first. [mkdir] Created dir: /Users/anshumgupta/workspace/lucene-trunk3/solr/build/solr-core/classes/java [javac] Compiling 854 source files to /Users/anshumgupta/workspace/lucene-trunk3/solr/build/solr-core/classes/java [javac] /Users/anshumgupta/workspace/lucene-trunk3/solr/core/src/java/org/apache/solr/handler/admin/SecurityConfHandler.java:36: error: cannot find symbol [javac] import org.apache.solr.security.RuleBasedAuthorizationPlugin; [javac] ^ [javac] symbol: class RuleBasedAuthorizationPlugin [javac] location: package org.apache.solr.security [javac] /Users/anshumgupta/workspace/lucene-trunk3/solr/core/src/java/org/apache/solr/handler/admin/SecurityConfHandler.java:101: error: cannot find symbol [javac] Map meta = RuleBasedAuthorizationPlugin.getMapValue(out, ""); [javac] ^ [javac] symbol: variable RuleBasedAuthorizationPlugin [javac] location: class SecurityConfHandler [javac] Note: Some input files use or override a deprecated API. [javac] Note: Recompile with -Xlint:deprecation for details. [javac] Note: Some input files use unchecked or unsafe operations. [javac] Note: Recompile with -Xlint:unchecked for details. [javac] 2 errors
          Show
          noble.paul Noble Paul added a comment - sorry , you have to apply https://issues.apache.org/jira/secure/attachment/12747308/SOLR-7757.patch this first and then https://issues.apache.org/jira/secure/attachment/12747309/SOLR-7692.patch
          Hide
          anshumg Anshum Gupta added a comment - - edited

          Noble Paul The last patch you uploaded fails compilation. To be clear, I'm talking about SOLR-7692.patch in isolation.

          Show
          anshumg Anshum Gupta added a comment - - edited Noble Paul The last patch you uploaded fails compilation. To be clear, I'm talking about SOLR-7692 .patch in isolation.
          Hide
          bosco Don Bosco Durai added a comment -

          Noble Paul, thanks.

          Show
          bosco Don Bosco Durai added a comment - Noble Paul , thanks.
          Hide
          noble.paul Noble Paul added a comment -

          Don Bosco Durai extend the class BasicAuthPlugin and override the method

           protected AuthenticationProvider getAuthenticationProvider(Map<String, Object> pluginConfig) }}
          

          which means , you will need to implement the AuthenticationProvider interface

          Show
          noble.paul Noble Paul added a comment - Don Bosco Durai extend the class BasicAuthPlugin and override the method protected AuthenticationProvider getAuthenticationProvider(Map< String , Object > pluginConfig) }} which means , you will need to implement the AuthenticationProvider interface
          Hide
          bosco Don Bosco Durai added a comment -

          Noble Paul, sorry if this was discussed before in any other thread. Seems the user/password is now stored in ZK. Were there any consideration to support users from LDAP/AD?

          Assuming it is out of scope for this JIRA, what would it take to implement a configurable user repository? E.g. Use basic auth, but user can get authenticated against LDAP/AD or custom auth provider?

          Thanks

          Show
          bosco Don Bosco Durai added a comment - Noble Paul , sorry if this was discussed before in any other thread. Seems the user/password is now stored in ZK. Were there any consideration to support users from LDAP/AD? Assuming it is out of scope for this JIRA, what would it take to implement a configurable user repository? E.g. Use basic auth, but user can get authenticated against LDAP/AD or custom auth provider? Thanks
          Hide
          noble.paul Noble Paul added a comment -

          standard permissions names are streamlined

          Show
          noble.paul Noble Paul added a comment - standard permissions names are streamlined
          Hide
          anshumg Anshum Gupta added a comment -

          Noble, can you please split this into 2 sub-tasks -> 2 patches so we finally have 2 independent commits for 2 independent features?
          I plan to look at it later tonight/tomorrow.

          Show
          anshumg Anshum Gupta added a comment - Noble, can you please split this into 2 sub-tasks -> 2 patches so we finally have 2 independent commits for 2 independent features? I plan to look at it later tonight/tomorrow.
          Hide
          noble.paul Noble Paul added a comment -

          some cleanup

          Show
          noble.paul Noble Paul added a comment - some cleanup
          Hide
          noble.paul Noble Paul added a comment -

          I plan to commit this pretty soon. All inputs/comments are welcome

          Show
          noble.paul Noble Paul added a comment - I plan to commit this pretty soon. All inputs/comments are welcome
          Hide
          noble.paul Noble Paul added a comment -

          more tests . renamed The ZkBasedAuthorizationPlugin to RulesBased AuthorizationPlugin

          Show
          noble.paul Noble Paul added a comment - more tests . renamed The ZkBasedAuthorizationPlugin to RulesBased AuthorizationPlugin
          Hide
          noble.paul Noble Paul added a comment -

          more tests and some bug fixes.

          Ishan Chattopadhyaya I have renamed some classes as you suggested

          Show
          noble.paul Noble Paul added a comment - more tests and some bug fixes. Ishan Chattopadhyaya I have renamed some classes as you suggested
          Hide
          noble.paul Noble Paul added a comment -

          Would it make sense to split out the authc/authz framework changes and the plugins themselves into two separate issues?

          I'm thinking of doing that..

          Can we rename TestZkAuthentication to something more appropriate?

          sure

          make ZK as one of many possible (and configurable) sources for credential stores for these plugins based on basicauth?

          The most common use case would be to use basic auth plugin and store the credentials elsewhere. The code is organized so that the credentials check is done in a separate class.
          ZkBasedAuthorizationPlugin cannot have another option . You can either use that or use something else altogether . Say, RangerAuthorizationPlugin ?

          Show
          noble.paul Noble Paul added a comment - Would it make sense to split out the authc/authz framework changes and the plugins themselves into two separate issues? I'm thinking of doing that.. Can we rename TestZkAuthentication to something more appropriate? sure make ZK as one of many possible (and configurable) sources for credential stores for these plugins based on basicauth? The most common use case would be to use basic auth plugin and store the credentials elsewhere. The code is organized so that the credentials check is done in a separate class. ZkBasedAuthorizationPlugin cannot have another option . You can either use that or use something else altogether . Say, RangerAuthorizationPlugin ?
          Hide
          ichattopadhyaya Ishan Chattopadhyaya added a comment -

          Would it make sense to split out the authc/authz framework changes and the plugins themselves into two separate issues? I think doing so will make it easier to follow the patches.

          Can we rename TestZkAuthentication to something more appropriate? It gives an impression that this is a test for ZK authentication, whereas it is actually a test suite for an authentication plugin that uses ZK as backing store.

          I haven't looked into the patch in great detail, but is it possible to drop the "ZK" part from the naming of the plugins and make ZK as one of many possible (and configurable) sources for credential stores for these plugins based on basicauth?

          Show
          ichattopadhyaya Ishan Chattopadhyaya added a comment - Would it make sense to split out the authc/authz framework changes and the plugins themselves into two separate issues? I think doing so will make it easier to follow the patches. Can we rename TestZkAuthentication to something more appropriate? It gives an impression that this is a test for ZK authentication, whereas it is actually a test suite for an authentication plugin that uses ZK as backing store. I haven't looked into the patch in great detail, but is it possible to drop the "ZK" part from the naming of the plugins and make ZK as one of many possible (and configurable) sources for credential stores for these plugins based on basicauth?
          Hide
          noble.paul Noble Paul added a comment -

          watch added to /security.json . The plugins are reloaded when the content is modified

          Show
          noble.paul Noble Paul added a comment - watch added to /security.json . The plugins are reloaded when the content is modified
          Hide
          noble.paul Noble Paul added a comment -

          These are totally independent. So, users can mix and match these plugins as they wish

          Show
          noble.paul Noble Paul added a comment - These are totally independent. So, users can mix and match these plugins as they wish
          Hide
          anshumg Anshum Gupta added a comment -

          Noble, can you split this out into 2 issues that can be committed and used independently? e.g. a user might just want to use basic auth e.g. if I know a person, I'll let him in but if I don't I won't and that shouldn't require an authorization plugin at all. Or use a different authorization mechanism with the simple auth.

          Show
          anshumg Anshum Gupta added a comment - Noble, can you split this out into 2 issues that can be committed and used independently? e.g. a user might just want to use basic auth e.g. if I know a person, I'll let him in but if I don't I won't and that shouldn't require an authorization plugin at all. Or use a different authorization mechanism with the simple auth.
          Hide
          noble.paul Noble Paul added a comment -

          added APIs to edit BasicAuth/ZkBasedAuthorization

          Show
          noble.paul Noble Paul added a comment - added APIs to edit BasicAuth/ZkBasedAuthorization
          Hide
          noble.paul Noble Paul added a comment - - edited

          more features, support for http method, request parameters in permissions etc

          well known permissions such as collection-admin-read , collection-admin

          Show
          noble.paul Noble Paul added a comment - - edited more features, support for http method, request parameters in permissions etc well known permissions such as collection-admin-read , collection-admin
          Hide
          noble.paul Noble Paul added a comment -

          Thanks for your comments

          This comment is misleading - probably left over from an earlier iteration.

          The patch is Work in Progress . So the comments are from a former iteration

          Please add a test case that uses the salt when authenticating.

          The test case indeed checks with salt. There will be a test w/o salt as well

          Do you think it would be reasonable to split out the dependency between BasicAuthPlugin and ZkAuthentication

          Yes, That is the plan . I've extracted separated the HTTP part and authentication part to two distinct classes. You should be able to extend the BasicAuthPlugin to provide your own Authentication impl

          The name might mislead users.

          The names are subject to change. Suggestions are welcome

          can you separate out the 2 issues i.e. an authentication and an authorization?

          There are a bunch of sub-tasks required
          1) Authentication
          2) Authorization
          3) API to manage the users/roles/permissions

          Show
          noble.paul Noble Paul added a comment - Thanks for your comments This comment is misleading - probably left over from an earlier iteration. The patch is Work in Progress . So the comments are from a former iteration Please add a test case that uses the salt when authenticating. The test case indeed checks with salt. There will be a test w/o salt as well Do you think it would be reasonable to split out the dependency between BasicAuthPlugin and ZkAuthentication Yes, That is the plan . I've extracted separated the HTTP part and authentication part to two distinct classes. You should be able to extend the BasicAuthPlugin to provide your own Authentication impl The name might mislead users. The names are subject to change. Suggestions are welcome can you separate out the 2 issues i.e. an authentication and an authorization? There are a bunch of sub-tasks required 1) Authentication 2) Authorization 3) API to manage the users/roles/permissions
          Hide
          anshumg Anshum Gupta added a comment -

          Thanks Noble! this is much needed!

          I am yet to look at this, but can you separate out the 2 issues i.e. an authentication and an authorization?

          Also, let's not call it zkAuth* plugins as they don't authenticate zk, but just use zk for implementation. The name might mislead users.

          I'll take a look at the actual code over the weekend.

          Show
          anshumg Anshum Gupta added a comment - Thanks Noble! this is much needed! I am yet to look at this, but can you separate out the 2 issues i.e. an authentication and an authorization? Also, let's not call it zkAuth* plugins as they don't authenticate zk, but just use zk for implementation. The name might mislead users. I'll take a look at the actual code over the weekend.
          Hide
          mdrob Mike Drob added a comment -
          +  public static AuthorizationResponse OK = new AuthorizationResponse(200);
          +  public static AuthorizationResponse FORBIDDEN = new AuthorizationResponse(403);
          +  public static AuthorizationResponse PROMPT = new AuthorizationResponse(401);
          

          Please make these final.

          +  private static Set<String> EMPTY_NULL_SET;
          

          Also final.

          +  @Override
          +  public void init(Map<String, Object> initInfo) {
          +    mapping.put(null, new WildCardSupportMap());
          +    Map map = (Map) initInfo.get("roles");
          +    for (Object o : map.entrySet()) {
          +      Map.Entry e = (Map.Entry) o;
          +      String roleName = (String) e.getKey();
          +      usersVsRoles.put(roleName, readSet(map, roleName));
          +    }
          +    map = (Map) initInfo.get("permissions");
          +    for (Object o : map.entrySet()) {
          +      Map.Entry e = (Map.Entry) o;
          +      Permission p = new Permission((String) e.getKey(), (Map) e.getValue());
          +      permissions.add(p);
          +      add2Mapping(p);
          +    }
          +  }
          

          Is it possible to use generic types instead of doing a bunch of casts? There's a bunch of other places with raw Map as well.

          +  //check permissions for a collection
          +  //return true = allowed, false=not allowed, null= resource requires a principal but none available
          +  private MatchStatus checkCollPerm(Map<String, List<Permission>> pathVsPerms,
          +                                AuthorizationContext context) {
          

          This comment is misleading - probably left over from an earlier iteration.

          Please add a test case that uses the salt when authenticating.

          Do you think it would be reasonable to split out the dependency between BasicAuthPlugin and ZkAuthentication? I could imagine somebody wanting to do BasicAuth backed by a different store, were it available.

          Will continue to dive deeper in a bit.

          Show
          mdrob Mike Drob added a comment - + public static AuthorizationResponse OK = new AuthorizationResponse(200); + public static AuthorizationResponse FORBIDDEN = new AuthorizationResponse(403); + public static AuthorizationResponse PROMPT = new AuthorizationResponse(401); Please make these final. + private static Set< String > EMPTY_NULL_SET; Also final. + @Override + public void init(Map< String , Object > initInfo) { + mapping.put( null , new WildCardSupportMap()); + Map map = (Map) initInfo.get( "roles" ); + for ( Object o : map.entrySet()) { + Map.Entry e = (Map.Entry) o; + String roleName = ( String ) e.getKey(); + usersVsRoles.put(roleName, readSet(map, roleName)); + } + map = (Map) initInfo.get( "permissions" ); + for ( Object o : map.entrySet()) { + Map.Entry e = (Map.Entry) o; + Permission p = new Permission(( String ) e.getKey(), (Map) e.getValue()); + permissions.add(p); + add2Mapping(p); + } + } Is it possible to use generic types instead of doing a bunch of casts? There's a bunch of other places with raw Map as well. + //check permissions for a collection + // return true = allowed, false =not allowed, null = resource requires a principal but none available + private MatchStatus checkCollPerm(Map< String , List<Permission>> pathVsPerms, + AuthorizationContext context) { This comment is misleading - probably left over from an earlier iteration. Please add a test case that uses the salt when authenticating. Do you think it would be reasonable to split out the dependency between BasicAuthPlugin and ZkAuthentication? I could imagine somebody wanting to do BasicAuth backed by a different store, were it available. Will continue to dive deeper in a bit.
          Hide
          noble.paul Noble Paul added a comment -

          first patch with basic tests

          Show
          noble.paul Noble Paul added a comment - first patch with basic tests

            People

            • Assignee:
              noble.paul Noble Paul
              Reporter:
              noble.paul Noble Paul
            • Votes:
              1 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development