Skip to content
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

Microsoft Active Directory you can have groups in groups support. #40

Merged

Conversation

johansmitsnl
Copy link

To support this you need to set the leaf group member option for it
to follow the sub groups (if any).

Thanks for submitting a pull request ! Please provide enough information so that others can review your changes.

For more information, see the CONTRIBUTING guide.

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

  • What is the current behavior (you can also link to an open issue here) ?
  • What is the new behavior (if this is a feature change) ?

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation only
  • Performance improvement
  • Refactoring (a change that neither fixes a bug nor adds a feature)
  • Test (adding missing tests or correcting existing ones)
  • Other...

Status

  • Ready
  • In development
  • Hold

Todo List

  • Implementation
  • Tests
  • Documentation
  • ...

How has this been tested ?

It has been tested against my AD. Not sure how to make the tests with OpenLDAP.

@AndrewSav
Copy link
Collaborator

@johansmitsnl did you check that this change does not break the existing tests?

@johansmitsnl
Copy link
Author

@AndrewSav id did break one test because I forgot to add it. I updated the MR with a fix for the variable replacement check. Whould be ok now.

@AndrewSav
Copy link
Collaborator

AndrewSav commented Sep 23, 2021

@johansmitsnl may I suggest you try and run the tests locally? They still seems to have failed.

@SaraSmiseth
Copy link
Member

New variables have to be added to setup.sh and they have to be exported so that they can be used in the templates. LDAP variables should go here:

if [ "$DBDRIVER" = "ldap" ]; then

To support this you need to set the leaf group member option for it
to follow the sub groups (if any).
@johansmitsnl
Copy link
Author

@SaraSmiseth thanks for the pointer. I'm having some difficulties installing docker (for some reason that is unclear to me yet) But I have added the suggested variables.

@johansmitsnl
Copy link
Author

@SaraSmiseth and @AndrewSav I fixed my docker install and tests pass locally as they also did here on Github I see. Is this good to merge or do you need more info?

@SaraSmiseth
Copy link
Member

@SaraSmiseth and @AndrewSav I fixed my docker install and tests pass locally as they also did here on Github I see. Is this good to merge or do you need more info?

Looks good. I'm not exactly sure how the ldap stuff works, but I think we should add tests that use these new variables. You added the variables to init_ldap but I think these tests would work even if they were not set right?

Maybe change the ldap configuration here or add a new config that uses group variables. Then we can use config with variables in ldap tests and the old config without the new variables in ldap2 tests.

What about merging this as it is and add specific tests for this later?

@AndrewSav
Copy link
Collaborator

AndrewSav commented Sep 30, 2021

Looks good. I'm not exactly sure how the ldap stuff works, but I think we should add tests that use these new variables. You added the variables to init_ldap but I think these tests would work even if they were not set right?

We do not have an AD in our tests to test against, so writing tests here could be quite tricky. On OpenLdap testing them could be pointless if they are added with the specific purpose of supporting AD.

I'll try to merge and push a new build on the weekend if time permits and no one does that before me.

@johansmitsnl
Copy link
Author

I'm not so familiar with OpenLdap, but these variables are needed when you use groups in groups with users in them. When you don't walk through the groups you will end up with no users.

-- Group 1
         |-- Group 2
         |         |-- User 1
         |         |-- User 2
         |
         |-- Group 3
                  |-- User 3

The result without the change is no users at all, with the change, User 1,2 and 3.

From the Postfix manual:

       leaf_result_attribute (default: empty)
              When one or more  special  result  attributes  are  found  in  a
              non-terminal  (see above) LDAP entry, leaf result attributes are
              excluded from the expansion of that entry. This is  useful  when
              expanding  groups  and  the desired mail address attribute(s) of
              the member objects obtained via DN or  URI  recursion  are  also
              present in the group object. To only return the attribute values
              from the leaf objects and not  the  containing  group,  add  the
              attribute   to  the  leaf_result_attribute  list,  and  not  the
              result_attribute list,  which  is  always  expanded.  Note,  the
              default  value  of "result_attribute" is not empty, you may want
              to set it explicitly empty when using "leaf_result_attribute" to
              expand  the  group  to  a list of member DN addresses. If groups
              have both member DN references AND attributes that hold multiple
              string valued rfc822 addresses, then the string attributes go in
              "result_attribute".  The attributes  that  represent  the  email
              addresses  of  objects  referenced  via a DN (or LDAP URI) go in
              "leaf_result_attribute".

Maybe some ldap people know how to translate this into a test?

@AndrewSav
Copy link
Collaborator

It does not appear that we have any ldap people here

@SaraSmiseth SaraSmiseth merged commit 7168853 into mailserver2:master Oct 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants