Microsoft Active Directory you can have groups in groups support.

johansmitsnl commented 3 years ago

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)
[x] 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

[x] Ready
[ ] In development
[ ] Hold

Todo List

[ ] Implementation
[x] Tests
[ ] Documentation
[ ] ...

How has this been tested ?

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

AndrewSav commented 3 years ago

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

johansmitsnl commented 3 years ago

@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 commented 3 years ago

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

SaraSmiseth commented 3 years ago

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:

https://github.com/mailserver2/mailserver/blob/639ec3b3fa145cf7cb4ce5abc40f212ca0331081/rootfs/usr/local/bin/setup.sh#L93

johansmitsnl commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

It does not appear that we have any ldap people here

mailserver2 / mailserver