Quantcast

[GitHub] activemq-artemis pull request #1206: ARTEMIS-1116 - added role mapper for LD...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis pull request #1206: ARTEMIS-1116 - added role mapper for LD...

RomanIsko
GitHub user sjhiggs opened a pull request:

    https://github.com/apache/activemq-artemis/pull/1206

    ARTEMIS-1116 - added role mapper for LDAP roles to local ActiveMQ roles

    For ARTEMIS-1116, allows configuring LDAP role mapping to local ActiveMQ roles.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/sjhiggs/activemq-artemis role_mapping

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/activemq-artemis/pull/1206.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #1206
   
----
commit 1333024c659b6f1c3c37a3e2aa7b58c125973aa8
Author: Stephen Higgs <[hidden email]>
Date:   2017-04-16T02:05:25Z

    added role mapper

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1206: ARTEMIS-1116 - added role mapper for LDAP role...

RomanIsko
Github user jbertram commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1206
 
    Couple of things:
   
    - The commit message should follow the format outlined in the [Hacking Guide](https://github.com/apache/activemq-artemis/blob/master/docs/hacking-guide/en/maintainers.md#commit-messages).
    - You have a fair number of checkstyle violations. See more [here](https://builds.apache.org/blue/organizations/jenkins/ActiveMQ-Artemis-PR-Build/detail/ActiveMQ-Artemis-PR-Build/2607/pipeline). You run the checkstyle stuff locally by using 'mvn -Pdev install'.
    - I wonder if this feature would be better if it was more general so that it could be used with any login module rather than isolated only to the LDAP login module.  What do you think?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1206: ARTEMIS-1116 - added role mapper for LDAP role...

RomanIsko
In reply to this post by RomanIsko
Github user jbertram commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1206
 
    Thanks for fixing the commit message and checkstyle issues.
   
    The more I think about this the more I think this should be a general feature which all login modules can use rather than just the LDAP login module. In general, this functionality would only be useful for login modules pulling data from an external system like LDAP which makes it a natural fit for the LDAP login module, but other users might implement their own login module where this functionality would be useful (e.g. integrating with a legacy auth system) and I wouldn't want to require a re-implementation. Also, I think moving the configuration into broker.xml from login.config will be provide a better user experience.
   
    From a configuration perspective this would be a good fit in <security-settings> next to <security-setting> and <security-setting-plugin>. The name <role-mapping> seems appropriate. It could have a "from" and "to" attribute where "from" is delimited with e.g. a comma.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1206: ARTEMIS-1116 - added role mapper for LDAP role...

RomanIsko
In reply to this post by RomanIsko
Github user sjhiggs commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1206
 
    Proposed XML configuration structure:
    ~~~
    <role-mappings>
       <role-mapping>
          <role>admin</role>
          <mapped-roles>
             <mapped-role>Administrator</mapped-role>
             <mapped-role>amq</mapped-role>
          </mapped-roles>
       </role-mapping>
    <role-mappings>
    ~~~
   
    Thoughts?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1206: ARTEMIS-1116 - added role mapper for LDAP role...

RomanIsko
In reply to this post by RomanIsko
Github user jbertram commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1206
 
    I was thinking something a bit less verbose like...
   
        <security-settings>
           ...
           <role-mapping to="admin" from="Administrator,amq"/>
           ...
        <security-settings>


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1206: ARTEMIS-1116 - added role mapper for LDAP role...

RomanIsko
In reply to this post by RomanIsko
Github user sjhiggs commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1206
 
    That's fine, I was assuming "from" was the external/LDAP role and "to" is the internal AMQ role, do you see it the opposite?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1206: ARTEMIS-1116 - added role mapper for LDAP role...

RomanIsko
In reply to this post by RomanIsko
Github user jbertram commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1206
 
    > I was assuming "from" was the external/LDAP role and "to" is the internal AMQ role, do you see it the opposite?
   
    I see the "from" was the external role and "to" is the internal Artemis role as you said.
   
    > ...the from will probably need a different delimiter because the role name may be something like a DN.
   
    Whatever makes sense from an implementation perspective is fine with me. I was just going on the fact that the original commit used commas as a delimiter.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis issue #1206: ARTEMIS-1116 - added role mapper for LDAP role...

RomanIsko
In reply to this post by RomanIsko
Github user sjhiggs commented on the issue:

    https://github.com/apache/activemq-artemis/pull/1206
 
    Updated - it works a little differently than I originally anticipated; a set of roles is calculated when parsing the broker.xml taking into account not only the original roles but the role mappings as well.  This commit is quite a bit more intrusive than the previous one.  Tests are included, but needs review all the same.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] activemq-artemis pull request #1206: ARTEMIS-1116 - added role mapper for LD...

RomanIsko
In reply to this post by RomanIsko
Github user asfgit closed the pull request at:

    https://github.com/apache/activemq-artemis/pull/1206


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---
Loading...