Bug 49092 - S4-Connector: Synchronize more attributes
S4-Connector: Synchronize more attributes
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: S4 Connector
UCS 4.4
Other Linux
: P5 normal (vote)
: UCS 4.4-1-errata
Assigned To: Florian Best
Arvid Requate
:
Depends on: 49008
Blocks: 50033
  Show dependency treegraph
 
Reported: 2019-03-25 20:57 CET by Arvid Requate
Modified: 2020-08-04 10:35 CEST (History)
3 users (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 2: Improvement: Would be a product improvement
Who will be affected by this bug?: 2: Will only affect a few installed domains
How will those affected feel about the bug?: 1: Nuisance – not a big deal but noticeable
User Pain: 0.023
Enterprise Customer affected?: Yes
School Customer affected?:
ISV affected?:
Waiting Support:
Flags outvoted (downgraded) after PO Review:
Ticket number:
Bug group (optional):
Max CVSS v3 score:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Arvid Requate univentionstaff 2019-03-25 20:57:02 CET
We should check which additional attributes would be useful to synchronize between UDM/OpenLDAP and Samba/AD.

When updating existing UCS installations, care must be taken to

1. not overwrite live attribute values
2. synchronize all new attribute values

This update handling may need more effort than the attribute mapping itself, so it makes sense to do this once for a bunch of additional attributes.

I'll add a couple of old bugs to the See Also field.


+++ This bug was initially created as a clone of Bug #49008 +++

+++ This bug was initially created as a clone of Bug #49007 +++

A customer wanted to have the LDAP attribute employeeNumber synchronized to an attribute of the same name in AD and S4. I think it makes sense to integrate the resulting patch into the default mapping.
Comment 1 Arvid Requate univentionstaff 2019-07-31 21:34:07 CEST
Please pull branch jbremer/bug49092, during QA I commited some minor wording fixes (269904ef4f). You may proceed an merge the branch into 4.4-1, update teh changelog, push the changes to gitlab and build the package.
Comment 2 Daniel Tröder univentionstaff 2019-08-02 13:08:02 CEST
Not sure this is the correct bug, but if "departmentNumber" was changed, that had an implication: http://jenkins.knut.univention.de:8080/job/UCSschool-4.4/job/Install%20Singleserver/178/Config=s4,TestGroup=import1/testReport/junit/90_ucsschool/231_import-users_checks_in_dry-run_birthday/master201/
Comment 3 Florian Best univentionstaff 2019-08-02 13:36:46 CEST
(In reply to Daniel Tröder from comment #2)
> Not sure this is the correct bug, but if "departmentNumber" was changed,
> that had an implication:
> http://jenkins.knut.univention.de:8080/job/UCSschool-4.4/job/
> Install%20Singleserver/178/Config=s4,TestGroup=import1/testReport/junit/
> 90_ucsschool/231_import-users_checks_in_dry-run_birthday/master201/
Yes, the change changed departmentNumber and roomNumber from single value to multi value.
Comment 4 Florian Best univentionstaff 2019-08-02 22:03:34 CEST
We should especially take care (maybe write a test case) for newly added properties which also exists as extended attributes with a) the same propertey name and b) a different property name.
If we add properties and don't add them to the layout, extended attributes must be able to display them in the layout, while not destroying other functionality.
Comment 5 Florian Best univentionstaff 2019-08-14 13:09:31 CEST
There is no problems with extended attributes when the user is added on AD side:
$ samba-tool user create test1113 U.niven@99 --surname=foo --initials=X.Y --department=foo --given-name=foo

But there is a reject in the S4-Connector if an extended attribute with different name than the property name exists and the user is added via UCS/UDM:

udm users/user create --set myInitials=foo --set username=test1112 --set password=univention --set lastname=foo

04.08.2019 13:16:21.496 LDAP        (PROCESS): sync from ucs: [          user] [       add] cn=test1112,DC=school,DC=dev
04.08.2019 13:16:21.609 LDAP        (PROCESS): sync from ucs: [          user] [    modify] cn=test1112,DC=school,DC=dev
04.08.2019 13:16:23.104 LDAP        (PROCESS): sync to ucs:   [          user] [    modify] uid=test1112,l=school,l=dev
04.08.2019 13:16:23.258 LDAP        (ERROR  ): Unknown Exception during sync_to_ucs
04.08.2019 13:16:23.260 LDAP        (ERROR  ): Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/univention/s4connector/__init__.py", line 1543, in sync_to_ucs
    result = self.modify_in_ucs(property_type, object, module, position)
  File "/usr/lib/python2.7/dist-packages/univention/s4connector/__init__.py", line 1293, in modify_in_ucs
    res = ucs_object.modify(serverctrls=serverctrls, response=response)
  File "/usr/lib/pymodules/python2.7/univention/admin/handlers/users/user.py", line 1426, in modify
    return super(object, self).modify(*args, **kwargs)
  File "/usr/lib/pymodules/python2.7/univention/admin/handlers/__init__.py", line 647, in modify
    dn = self._modify(modify_childs, ignore_license=ignore_license, response=response)
  File "/usr/lib/pymodules/python2.7/univention/admin/handlers/__init__.py", line 1322, in _modify
    self.dn = self.lo.modify(self.dn, ml, ignore_license=ignore_license, serverctrls=serverctrls, response=response)
  File "/usr/lib/pymodules/python2.7/univention/admin/uldap.py", line 900, in modify
    raise univention.admin.uexceptions.ldapError(_err2str(msg), original_exception=msg)
ldapError: Type or value exists: modify/add: initials: value #0 already exists

04.08.2019 13:16:23.260 LDAP        (WARNING): sync to ucs was not successful, save rejected
04.08.2019 13:16:23.260 LDAP        (WARNING): object was: CN=test1112,DC=school,DC=dev
04.08.2019 13:16:28.273 LDAP        (PROCESS): sync from ucs: [         group] [    modify] cn=domain users,cn=groups,DC=school,DC=dev

The state is correct on both sides then. But the S4-Connector cannot handle that the attribute already exists, it tries to add it again on UCS side. This wouldn't be a problem If we had Bug #43286 fixed.
Comment 6 Florian Best univentionstaff 2019-08-14 15:36:39 CEST
I reverted the problematic properties: 'initials': 'physicalDeliveryOfficeName' 'postOfficeBox' 'preferredLanguage'.
We should fix those in the next minor release UCS 4.4-2.
All other changes are still applied.

univention-directory-manager-modules (14.0.13-9)
3ab72efbc06b | Bug #49092: revert adding properties
5ff78a2f31d1 | Bug #49092: revert adding properties

univention-s4-connector (13.0.2-37)
3127b4fa7d48 | Bug #49092: revert adding properties
5ff78a2f31d1 | Bug #49092: revert adding properties
Comment 7 Arvid Requate univentionstaff 2019-08-20 12:04:41 CEST
In 5ff78a2f31 you added

+mapping.register('initials', 'initials', None, univention.admin.mapping.ListToS

to usertemplate.py but you commented out the rest, I guess this one needs to be commented out too?

In users/user.py too.


I think the removal of the transaltion strings (3ab72efbc06b) was not strictly necessary, that caused a lot of noise to QA.
Comment 9 Florian Best univentionstaff 2019-08-20 12:38:02 CEST
(In reply to Arvid Requate from comment #8)
> The tests now fail:
> http://jenkins.knut.univention.de:8080/job/UCS-4.4/job/UCS-4.4-1/job/
> AutotestJoinReleased/SambaVersion=s4connector,Systemrolle=master/
> lastCompletedBuild/testReport/  -- probably things ned to be reverted there
> too?
The test have already been reverted in ucs-test:
Successful build
Package: ucs-test
Version: 9.0.3-10A~4.4.0.201908171126
Branch: ucs_4.4-0
Scope: errata4.4-1
https://git.knut.univention.de/univention/ucs/commit/bcdb7b5ade94a70a41eeb14ba72e24cad84ff399

But in Jenkins this version of ucs-test ist not installed:
http://jenkins.knut.univention.de:8080/job/UCS-4.4/job/UCS-4.4-1/job/AutotestJoinReleased/ws/SambaVersion/s4connector/Systemrolle/master/test/packages-under-test.log

install ok installed	ucs-test-s4connector	9.0.3-8A~4.4.0.201908121611

Both tests pass on my system.
Comment 10 Florian Best univentionstaff 2019-08-20 12:46:20 CEST
(In reply to Arvid Requate from comment #7)
> In 5ff78a2f31 you added
> 
> +mapping.register('initials', 'initials', None,
> univention.admin.mapping.ListToS
> 
> to usertemplate.py but you commented out the rest, I guess this one needs to
> be commented out too?
> 
> In users/user.py too.
Fixed in:
univention-directory-manager-modules (14.0.13-11)
3b1d4997c081 | Bug #49092: remove initials from mapping
99016d7ccbea | Bug #49092: remove initials from mapping

> I think the removal of the transaltion strings (3ab72efbc06b) was not
> strictly necessary, that caused a lot of noise to QA.
It happens automatically if they are commented out in the code, with `univention-l10n-build de` I keep the language files up2date.
If this causes noise when you want to view the diff, I suggest you to use a git-driver to only the the real diff:

~/.gitconfig
[core]
        attributesfile = ~/.gitattributes
[diff "po"]
        #textconv=msgcat --no-location --no-wrap --sort-output
        textconv=bash -c 'msgcat --no-location --no-wrap --sort-output "$1" || cat "$1"' --
[merge "pofile"]
        name = custom merge driver for gettext po files
        driver = git-po-merge %A %O %B

And ~/.gitattributes:
*.po merge=pofile diff=po
*.pot merge=pofile


Then you can do `git show -U0 3ab72efbc06bc87ea5dc10d3f098a3976a8a87c5` and everything is displayed very nice!
Comment 11 Arvid Requate univentionstaff 2019-08-20 18:37:13 CEST
Ah, thanks for the hint!

Now we still add "initials,physicalDeliveryOfficeName,postOfficeBox,preferredLanguage" to the UCR variable connector/s4/mapping/user/attributes/ignorelist. I guess it would be better to avoid that too.
Comment 12 Florian Best univentionstaff 2019-08-20 20:00:40 CEST
(In reply to Arvid Requate from comment #11)
> Ah, thanks for the hint!
> 
> Now we still add
> "initials,physicalDeliveryOfficeName,postOfficeBox,preferredLanguage" to the
> UCR variable connector/s4/mapping/user/attributes/ignorelist. I guess it
> would be better to avoid that too.

Okay, adapted the tests:
46a8eaa0c4aa36e2bc5d42be619a4e14e3dcdb2f
Package: ucs-test
Version: 9.0.3-12A~4.4.0.201908201959
Comment 13 Arvid Requate univentionstaff 2019-08-20 20:05:21 CEST
Oh, misunderstanding, I was refering to debian/univention-s4-connector.postinst
Comment 14 Florian Best univentionstaff 2019-08-20 20:15:17 CEST
(In reply to Arvid Requate from comment #13)
> Oh, misunderstanding, I was refering to
> debian/univention-s4-connector.postinst

Oh yes, fixed also that:
09a4f10cd01e5b399cbe84666cbe4b7c58efc9c8
Package: univention-s4-connector
Version: 13.0.2-41A~4.4.0.201908202014
Comment 15 Florian Best univentionstaff 2019-08-22 13:37:39 CEST
I reverted the mapping of country, because in UDM country is mapped to "st" (state). This would be inconsistent, let's fix this correctly.

ucs-test (9.0.3-17)
d9cb6b4a053c | Bug #49092: adjust tests for non syncing st / c

univention-s4-connector (13.0.2-42)
fee1d4db9037 | Bug #49092: do not sync country / state
Comment 16 Arvid Requate univentionstaff 2019-08-22 15:00:40 CEST
Verified:
* Code review
* Functional tests (UCS, UCS@school slave)
* Update compatibility tests (fresh install of UCS@school slave into domain with old S4-Connector mapping)
* Advisory