LDAP alias provisioning by kesselb · Pull Request #5198 · nextcloud/mail

kesselb · 2021-06-16T12:45:41Z

Fix #5173

Requires: Add method to read multi-value attributes from ldap server#27515.

Todo

Add option to enable/disable alias provisioning
Add option to customize the ldap attribute
~~Add option to filter aliases by regex~~
Alias section is currently hidden for provisioned accounts. Show the section but disable add alias / delete alias
It should be possible to change the name but not the email/alias
Test that editing the signature for alias works
Add a database field (or virtual field) to alias if provisioned or not
~~Add unique constraint to mail_aliases account_id and alias.~~
UI to enable/disable, ldap attribute, set filter.

lib/Db/Alias.php

src/components/AliasForm.vue

kesselb · 2021-07-06T14:26:29Z

lib/Service/Provisioning/Manager.php

+			return $provisioning;
+		}
+
+		if ($this->ldapProviderFactory->isAvailable() === false) {


Snap! isAvailable is new in 21 🤷‍♂️

Do an additional server version comparison here and don't offer the feature for Nextcloud 20 or older. It's a fair tradeoff IMO.

Thanks! I added @psalm-suppress UndefinedInterfaceMethod to make Psalm on Nextcloud 20 happy. The check for the right Nextcloud version is already done some lines earlier: https://github.com/nextcloud/mail/pull/5198/files#diff-fe646e6e2f7561cf6a8731328355c7432be7647f7114e1619fe2bf5ff2638c42R205. If getMultiValueUserAttribute exist isAvailable also exist.

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>

ChristophWurst

Code looks good otherwise :)

ChristophWurst · 2021-07-07T09:53:38Z

lib/Controller/SettingsController.php

 			$this->provisioningManager->newProvisioning($data);
 		} catch (ValidationException $e) {
 			return HttpJsonResponse::fail([$e->getFields()]);
+		} catch (\Exception $e) {


could you elaborate why this was widened?

ChristophWurst · 2021-07-07T09:54:49Z

lib/Controller/SettingsController.php

+			$this->provisioningManager->updateProvisioning(array_merge($data, ['id' => $id]));
 		} catch (ValidationException $e) {
 			return HttpJsonResponse::fail([$e->getFields()]);
+		} catch (\Exception $e) {


same. Could this possibly be a ClientException? \Error will throw almost anything. That anything could also be from unexpected service errors, then the HTTP4xx isn't appropriate IMO

$this->provisioningManager->newProvisioning($data); throws a validation exception if input data is invalid or a exception related to the database. I guess we can migrate this to Throwable and HttpJsonResponse.errorFromThrowable.

ChristophWurst · 2021-07-07T09:56:40Z

lib/Db/AliasMapper.php

+	 * Exception for Nextcloud 20: \Doctrine\DBAL\DBALException
+	 * Exception for Nextcloud 21 and newer: \OCP\DB\Exception
+	 *
+	 * @TODO: Change throws to \OCP\DB\Exception once Mail does not support Nextcloud 20.


FYI I think we do this soon-ish so we only have to support two major versions

kesselb · 2021-07-12T09:36:13Z

lib/Service/Provisioning/Manager.php

+
+		$ldapProvider = $this->ldapProviderFactory->getLDAPProvider();
+		/** @psalm-suppress UndefinedInterfaceMethod */
+		$provisioning->setAliases($ldapProvider->getMultiValueUserAttribute($user->getUID(), $provisioning->getLdapAliasesAttribute()));


@kesselb add validation for data

st3iny

Setup a test environment using your guide and tested it locally with multiple aliases. Works fine.

The only issue I noticed it that once an account has been provisioned, changes of the aliases in LDAP won't be forwarded to the mail alias settings.

E.g. If I remove one of multiple aliases from LDAP and then run the provisioning again, all aliases are still configured in mail.

kesselb · 2021-07-13T13:10:41Z

Setup a test environment using your guide and tested it locally with multiple aliases. Works fine.

The only issue I noticed it that once an account has been provisioned, changes of the aliases in LDAP won't be forwarded to the mail alias settings.

E.g. If I remove one of multiple aliases from LDAP and then run the provisioning again, all aliases are still configured in mail.

That should work.

kesselb · 2021-07-13T13:25:35Z

Setup a test environment using your guide and tested it locally with multiple aliases. Works fine.
The only issue I noticed it that once an account has been provisioned, changes of the aliases in LDAP won't be forwarded to the mail alias settings.
E.g. If I remove one of multiple aliases from LDAP and then run the provisioning again, all aliases are still configured in mail.

That should work.

The provisioning run with every http request in mail. To reduce the load to the ldap server the response for getMultiValueUserAttribute is cached (in server).

If a distributed cache is configured the value is cached. You may set ldapCacheTTL to 0 for testing.

st3iny · 2021-07-16T12:54:56Z

If a distributed cache is configured the value is cached. You may set ldapCacheTTL to 0 for testing.

That has been the issue. My dev setup has a distributed cache setup. Thanks for clarifying :)

ChristophWurst reviewed Jun 16, 2021

View reviewed changes

lib/Db/Alias.php Outdated Show resolved Hide resolved

kesselb commented Jun 22, 2021

View reviewed changes

src/components/AliasForm.vue Outdated Show resolved Hide resolved

kesselb force-pushed the enh/5173/ldap-aliases-provisioning branch 3 times, most recently from 7a87327 to e6c824d Compare June 25, 2021 10:15

kesselb commented Jul 6, 2021

View reviewed changes

kesselb force-pushed the enh/5173/ldap-aliases-provisioning branch from 4226ec5 to 0ee5cc6 Compare July 6, 2021 19:40

Add provisioning for aliases

8fb14b5

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>

kesselb force-pushed the enh/5173/ldap-aliases-provisioning branch from 0ee5cc6 to 8fb14b5 Compare July 6, 2021 20:04

kesselb added 3. to review enhancement labels Jul 6, 2021

kesselb changed the title ~~WIP: Ldap alias provisioning~~ LDAP alias provisioning Jul 6, 2021

kesselb marked this pull request as ready for review July 6, 2021 20:06

ChristophWurst requested review from ChristophWurst, miaulalala and st3iny July 7, 2021 07:34

ChristophWurst added this to the v1.11.0 milestone Jul 7, 2021

ChristophWurst reviewed Jul 7, 2021

View reviewed changes

kesselb mentioned this pull request Jul 7, 2021

Add guide how to setup ldap, imap and provisioning #5293

Merged

kesselb commented Jul 12, 2021

View reviewed changes

st3iny approved these changes Jul 13, 2021

View reviewed changes

ChristophWurst merged commit a32f165 into master Jul 14, 2021

ChristophWurst deleted the enh/5173/ldap-aliases-provisioning branch July 14, 2021 13:48

Conversation

kesselb commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristophWurst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

st3iny left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kesselb commented Jul 13, 2021

Uh oh!

kesselb commented Jul 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

st3iny commented Jul 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kesselb commented Jun 16, 2021 •

edited

Loading

st3iny left a comment •

edited

Loading

kesselb commented Jul 13, 2021 •

edited

Loading