-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable using LDAP groups with different objectclass #94
Enable using LDAP groups with different objectclass #94
Conversation
Hologram previously only searched for groups that had an objectclass of groupOfNames and wasn't configurable. This makes that attribute filter configurable. Fixes AdRoll#87
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -76,6 +76,7 @@ func main() { | |||
enableLDAPRoles = flag.Bool("ldaproles", false, "Enable role support using LDAP directory.") | |||
roleAttribute = flag.String("roleattribute", "", "Group attribute to get role from.") | |||
defaultRoleAttr = flag.String("defaultroleattr", "", "User attribute to check to determine a user's default role.") | |||
groupClassAttr = flag.String("groupclassattr", "", "LDAP objectclass to determine the groups in LDAP.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could set the default value here as groupOfNames
and avoid the conditional on line 154.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I didn't do that is because the config is read in from the config file on line 103, and this flag should be settable from both the config file and the command line, with the command line taking precedence if set. If I set the default value here, I wouldn't able to tell on lines 154-158 if the value was explicitly passed in on the command line with the value of groupOfNames
or if it was not specified and it's getting groupOfNames
from the default value, and thus it would effectively always override anything specified in the config file with the default, which wouldn't be desirable. Is there a better way of handling this behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see - that makes total sense. 👍
README.md
Outdated
@@ -172,7 +172,7 @@ The user must have permission to iam:GetUser on itself(resource "arn:aws:iam::AC | |||
|
|||
### LDAP Based Roles | |||
|
|||
Hologram supports assigning roles based on a user's LDAP group. Roles can be turned on by setting the `enableLDAPRoles` key to `true` in `config/server.json`. | |||
Hologram supports assigning roles based on a user's LDAP group. Roles can be turned on by setting the `enableLDAPRoles` key to `true` in `config/server.json`. Hologram will only search for groups that have an `objectclass` attribute of `groupOfNames`. To change this, set the `groupClassAttr` in your `config/server/json` to the correct objectclass. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: I'd change this slightly to say By default, Hologram will only...
just to be a bit more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@joelthompson We had some issues with cla-assistant on Hologram that are now fixed, however you'll have to sign the CLA again and then I can merge this PR. Sorry about that. |
@BillMedernach done |
Thanks! |
Hologram previously only searched for groups that had an objectclass of
groupOfNames and wasn't configurable. This makes that attribute filter
configurable.
Fixes #87