Implement join between entries for AD queries #273
Conversation
Add support of joining entries based on the DN of attributes to allow
expressions like `{member.userPrincipalName}` for role membership
definition when roles are defined from the `userPrincipalName`
attribute.
Codecov Report
@@ Coverage Diff @@
## master #273 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 12 12
Lines 1700 1758 +58
=====================================
+ Hits 1700 1758 +58
Continue to review full report at Codecov.
|
|
Wow, very interesting ! Did you found somethings hard to understand in the code base ? Some remark :
Did you know my naïve mock up of join feature : #128 (comment) ? Thanks for this huge contribution :-) |
|
Thanks for the review. The ldap2pg code base is very well structured which allowed to make this change quite small.
Yes, I've seen the mock up of the join feature when investigating how to link objects together. I took some latitude as I struggled to find a link between the configuration of the mock up and the configuration found in recent versions of ldap2pg.yml. The advantages of specifying the attribute (instead of join) is to allow multiple joins from the same entry (although I do not think of a usage scenario for that) and implicit configuration of the search base. The disadvantage is that it adds one more level in the configuration of the join. Would you prefer the implementation to follow more closely the mock up of issue #128? |
| ORDER BY 1; | ||
|
|
||
|
|
||
| privileges: |
There was a problem hiding this comment.
I think we can drop privileges management for this test.
There was a problem hiding this comment.
I agree with that, it does not impact what needs to be tested.
|
@hlecleme That's very good. For the format, I prefer list over dict. List item are more embeddable with all information. What do you think of a - ldap:
base: …
filter: …
join:
using: member
filter: …
role:
- name: '{sAMAccountName}'
member: '{member.sAMAccountName}'
- name: '{member.sAMAccountName}'
|
| except KeyError: | ||
| raise RDNError("Unknown RDN %s" % (path[0],), raw_dn) | ||
|
|
||
| if path[0] in DN_COMPONENTS: |
There was a problem hiding this comment.
If I understand properly, the trick is that member.dn does not triggers a subquery while member.unknownField will. Right ? I find this elegant. 👍
| values_with_attrs.append((value, sub_attrs)) | ||
| sub_attrs_cache[(attr, value)] = sub_attrs | ||
| except UserError as e: | ||
| logger.warn('Ignoring %s: %s' % (value, e)) |
There was a problem hiding this comment.
A detail this line should be:
logger.warning('Ignoring %s: %s', value, e)logger.warn is deprecated by logging. Formatting should be done by logging.
|
Note that it's just a matter of yaml formatting. Maybe you could accept both: - ldap:
join:
using: member
- ldap:
joins:
- using: member
- ldap:
joins:
member:
filter: …
...I tend to accept different formats in YAML. The inner representation must always be stable (in this case, a dict). The idea is that internal representation handle all cases. But user can write only what he need to define : a single join rather than a list of one join. Actually, lighter YAML syntax can be added later. The |
|
@hlecleme I'm ok to merge. We could improve this later (more YAML syntax, some refactoring, etc.). Can you confirm you're ready for merging ? |
|
@bersace Thanks again for the review. Yes, I'm ready. |
|
Bravo \o/ |
Allow joining LDAP entries together based on a DN attribute. The syntax is similar to the one used to give the CN of a DN attribute (e.g.
{member.cn}) and allow to give entry attributes through an additional LDAP query (e.g.{member.userPrincipalName}).The
ldapconfiguration has been extended with one additional attributejoinsto permit the user to specify the filter to be used for the query (e.g. to filter out entries not having the referenced attribute).Thanks in advance for the review.