-
Notifications
You must be signed in to change notification settings - Fork 66
Additional group discovery and creation #342
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
Changes from all commits
73592db
ce76d86
cb82a0a
60e0872
cabc73d
20f2587
74b0eca
2f3ea47
fcb04ac
675bfe7
7379fe3
997fd1a
9b23000
ac17786
5caaecd
54a8ca6
141c85a
008a6cf
1ccae67
81d4c9f
e757f3d
d999650
3fe53ee
40b0abe
47295a9
22bf233
cb40834
4aae87b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -444,6 +444,7 @@ def get_rule_options(self): | |
options.update(self.invocation_options) | ||
|
||
# process directory configuration options | ||
new_account_type = None | ||
directory_config = self.main_config.get_dict_config('directory_users', True) | ||
if directory_config: | ||
# account type | ||
|
@@ -457,6 +458,15 @@ def get_rule_options(self): | |
default_country_code = directory_config.get_string('default_country_code', True) | ||
if default_country_code: | ||
options['default_country_code'] = default_country_code | ||
additional_groups = directory_config.get_list('additional_groups', True) or [] | ||
additional_groups = [{'source': re.compile(r['source']), 'target': r['target']} for r in additional_groups] | ||
options['additional_groups'] = additional_groups | ||
sync_options = directory_config.get_dict_config('group_sync_options', True) | ||
if sync_options: | ||
options['auto_create'] = sync_options.get_bool('auto_create', True) | ||
if not new_account_type: | ||
new_account_type = user_sync.identity_type.ENTERPRISE_IDENTITY_TYPE | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is really unclear. If you are trying to make sure there's a default new_account_type in the absence of a But I also think you may be guarding against a case that doesn't exist? Isn't the directory configuration section actually required as part of the config? So the if at the front of this entire section ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what is going on here either. I'm responsible for this change, but I don't remember the rationale behind it. I agree that the new_account_type check isn't needed, so I'll remove it. I'll also see if the directory_config check is necessary. The only reason that variable would be None is if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm keeping the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds like a fine approach. |
||
self.logger.debug("Using default for new_account_type: %s", new_account_type) | ||
|
||
# process exclusion configuration options | ||
adobe_config = self.main_config.get_dict_config('adobe_users', True) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,7 @@ | |
import user_sync.error | ||
import user_sync.identity_type | ||
from user_sync.error import AssertionException | ||
from ldap import dn | ||
|
||
|
||
def connector_metadata(): | ||
|
@@ -121,6 +122,7 @@ def __init__(self, caller_options): | |
self.connection = connection | ||
logger.debug('Connected') | ||
self.user_by_dn = {} | ||
self.additional_group_filters = None | ||
|
||
def load_users_and_groups(self, groups, extended_attributes, all_users): | ||
""" | ||
|
@@ -209,6 +211,7 @@ def iter_users(self, users_filter, extended_attributes): | |
user_attribute_names.extend(self.user_email_formatter.get_attribute_names()) | ||
user_attribute_names.extend(self.user_username_formatter.get_attribute_names()) | ||
user_attribute_names.extend(self.user_domain_formatter.get_attribute_names()) | ||
user_attribute_names.append('memberOf') | ||
|
||
extended_attributes = [six.text_type(attr) for attr in extended_attributes] | ||
extended_attributes = list(set(extended_attributes) - set(user_attribute_names)) | ||
|
@@ -289,6 +292,18 @@ def iter_users(self, users_filter, extended_attributes): | |
elif last_attribute_name: | ||
self.logger.warning('No country code attribute (%s) for user with dn: %s', last_attribute_name, dn) | ||
|
||
uid_value = LDAPValueFormatter.get_attribute_value(record, six.text_type('uid')) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is this for? Is it for use in the extension stuff or something like that? A comment is needed here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Appears to be a remnant of the original design, which had the LDAP connector make an additional LDAP query to get direct membership groups. See 73592db I don't think we need this anymore since we're now using |
||
source_attributes['uid'] = uid_value | ||
|
||
user['member_groups'] = [] | ||
if self.additional_group_filters: | ||
member_groups = [] | ||
for f in self.additional_group_filters: | ||
for g in self.get_member_groups(record): | ||
if f.match(g) and g not in member_groups: | ||
member_groups.append(g) | ||
user['member_groups'] = member_groups | ||
|
||
if extended_attributes is not None: | ||
for extended_attribute in extended_attributes: | ||
extended_attribute_value = LDAPValueFormatter.get_attribute_value(record, extended_attribute) | ||
|
@@ -301,6 +316,36 @@ def iter_users(self, users_filter, extended_attributes): | |
|
||
yield (dn, user) | ||
|
||
def get_member_groups(self, user): | ||
""" | ||
Get a list of member group common names for user | ||
Assumes groups are contained in attribute memberOf | ||
:param user: | ||
:return: | ||
""" | ||
group_names = [] | ||
groups = LDAPValueFormatter.get_attribute_value(user, 'memberOf') | ||
for group_dn in map(dn.str2dn, groups): | ||
group_cn = self.get_cn_from_dn(group_dn) | ||
if group_cn: | ||
group_names.append(group_cn) | ||
return group_names | ||
|
||
@staticmethod | ||
def get_cn_from_dn(group_dn): | ||
""" | ||
Take a DN parsed by ldap.dn.str2dn and locate and return the common name | ||
Returns None if no common name is found | ||
If common name is complex (e.g. cn=Bob [email protected]) then first part of CN is returned | ||
:param group_dn: | ||
:return: | ||
""" | ||
for rdn in group_dn: | ||
for rdn_part in rdn: | ||
if rdn_part[0].lower() == 'cn': | ||
return rdn_part[1] | ||
return None | ||
|
||
def iter_search_result(self, base_dn, scope, filter_string, attributes): | ||
""" | ||
type: filter_string: str | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -146,6 +146,32 @@ def iter_users(self): | |
except umapi_client.UnavailableError as e: | ||
raise AssertionException("Error contacting UMAPI server: %s" % e) | ||
|
||
def get_groups(self): | ||
return list(self.iter_groups()) | ||
|
||
def iter_groups(self): | ||
try: | ||
for g in umapi_client.GroupsQuery(self.connection): | ||
yield g | ||
except umapi_client.UnavailableError as e: | ||
raise AssertionException("Error contacting UMAPI server: %s" % e) | ||
|
||
def get_user_groups(self): | ||
return list(self.iter_user_groups()) | ||
|
||
def iter_user_groups(self): | ||
try: | ||
for g in umapi_client.UserGroupsQuery(self.connection): | ||
yield g | ||
except umapi_client.UnavailableError as e: | ||
raise AssertionException("Error contacting UMAPI server: %s" % e) | ||
|
||
def create_group(self, name): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you should allow passing in the source group name so that you can use it in the comment: "Created to match directory group ... by User Sync Tool" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it would make sense to do that here. Auto-creation is designed to be a separate feature from the additional group discovery/mapping feature. It can be enabled independently of it and be used solely to auto-create Adobe groups targeted in the group mapping and/or the extension config. (conversely, the "additional groups" option can be enabled without group auto-creation) It would probably make more sense to log the "additional group" mapping somewhere in the additional group resolution workflow. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So then make the source group name be an optional argument and don't use it if it's not passed. The comment you're currently using is fairly useless in the auto-mapping case and would be a lot better if it had the source group. |
||
if name: | ||
group = umapi_client.UserGroupAction(group_name=name) | ||
group.create(description="Automatically created by User Sync Tool") | ||
return self.connection.execute_single(group) | ||
|
||
def get_action_manager(self): | ||
return self.action_manager | ||
|
||
|
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.
re.compile can throw (e.g.,
syntax_error
). you should catch the error and exit gracefully.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.
Will do