Bug 45066 - Regression: Handling of boolean UDM attributes by UMC
Regression: Handling of boolean UDM attributes by UMC
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: UMC - Domain management (Generic)
UCS 4.2
Other Linux
: P5 normal (vote)
: UCS 4.2-2-errata
Assigned To: Florian Best
Erik Damrose
:
: 44866 45674 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2017-07-25 12:43 CEST by Erik Damrose
Modified: 2017-11-27 11:25 CET (History)
9 users (show)

See Also:
What kind of report is it?: Bug Report
What type of bug is this?: 5: Major Usability: Impairs usability in key scenarios
Who will be affected by this bug?: 3: Will affect average number of installed domains
How will those affected feel about the bug?: 4: A User would return the product
User Pain: 0.343
Enterprise Customer affected?: Yes
School Customer affected?: Yes
ISV affected?:
Waiting Support:
Ticket number: 2017072021000398, 2017080321000365, 2017090621000671, 2017100521000393, 2017102321000215
Bug group (optional): API change, External feedback, Usability, Workaround is available
Max CVSS v3 score:


Attachments
proposed patch (1019 bytes, patch)
2017-07-25 18:05 CEST, Jürn Brodersen
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Erik Damrose univentionstaff 2017-07-25 12:43:02 CEST
With UCS 4.2-1e99 a regression in handling of boolean udm attributes was found, when compared to UCS 4.2-0e0

With UCS 4.2-1e99, a boolean attribute is always shown as 'activated' in UMC, regardless of the mapped LDAP attribute. It is now impossible to deactivate or activate the user via UMC.

CLI workaround: udm users/user modify ... -set owncloudEnabled=[0/1]

Example udm attribute: owncloudEnabled

LDAP for
user activated: univention-ldapsearch uid=univention | grep own
ownCloudEnabled: 1
objectClass: ownCloudUser

user deactivated: univention-ldapsearch uid=univention | grep own
objectClass: ownCloudUser

With UCS 4.2-0e0, the browsers GET request of the deactivated user:
{"status": 200, "message": null, "result": [{"CtxCfgClientPrinters": "0", "CtxKeyboardLayout": "", "uidNumber": "2006", "CtxInitialProgram": "", "disabled": "none", "CtxCallback": "", "$references$": [], "CtxWFProfilePath": "", "CtxRASDialin": "E", "CtxShadow": "00000000", "CtxCfgTSLogon": "0", "CtxStartprogramClient": "0", "CtxBrokenSession": "0000", "CtxWFHomeDirDrive": "", "CtxMaxIdleTime": "", "CtxCfgPresent": "551e0bb0", "CtxWFHomeDir": "", "unixhome": "/home/univention", "CtxWorkDirectory": "", "CtxNWLogonServer": "", "$flags$": [], "$policies$": {}, "$dn$": "uid=univention,cn=users,dc=ucs,dc=local", "shell": "/bin/bash", "CtxMinEncryptionLevel": "", "CtxCallbackNumber": "", "lastname": "univention", "CtxCfgFlags1": "00000100", "gidNumber": "5001", "groups": ["cn=Domain Users,cn=groups,dc=ucs,dc=local"], "primaryGroup": "cn=Domain Users,cn=groups,dc=ucs,dc=local", "CtxMaxDisconnectionTime": "", "$operations$": ["add", "edit", "remove", "search", "move"], "CtxCfgDefaultClientPrinters": "0", "$labelObjectType$": "Benutzer", "displayName": "univention", "CtxReconnectSession": "0000", "CtxMaxConnectionTime": "", "CtxCfgClientDrivers": "0", "locked": "none", "$options$": {"samba": true, "kerberos": true, "person": true, "posix": true, "mail": true, "pki": false, "ldap_pwd": false}, "username": "univention", "gecos": "univention", "sambaRID": "5012"}]}

No owncloudEnabled attribute is transmitted

With UCS 4.2-1e99, the browsers GET request of the deactivated user:
{"status": 200, "message": null, "result": [{"$policies$": {}, "CtxCfgClientPrinters": "0", "CtxKeyboardLayout": "", "uidNumber": "2006", "CtxInitialProgram": "", "disabled": "none", "CtxCallback": "", "$references$": [], "CtxWFProfilePath": "", "CtxRASDialin": "E", "CtxShadow": "00000000", "CtxStartprogramClient": "0", "CtxBrokenSession": "0000", "homeSharePath": "univention", "CtxWFHomeDirDrive": "", "CtxMaxIdleTime": "", "e-mail": [""], "CtxCfgPresent": "551e0bb0", "CtxWFHomeDir": "", "unixhome": "/home/univention", "CtxWorkDirectory": "", "CtxNWLogonServer": "", "$flags$": [], "username": "univention", "$dn$": "uid=univention,cn=users,dc=ucs,dc=local", "shell": "/bin/bash", "CtxMinEncryptionLevel": "", "firstname": null, "CtxCallbackNumber": "", "lastname": "univention", "CtxCfgFlags1": "00000100", "gidNumber": "5001", "mailForwardCopyToSelf": "0", "groups": ["cn=Domain Users,cn=groups,dc=ucs,dc=local"], "primaryGroup": "cn=Domain Users,cn=groups,dc=ucs,dc=local", "CtxMaxDisconnectionTime": "", "$operations$": ["add", "edit", "remove", "search", "move", "copy"], "CtxCfgDefaultClientPrinters": "0", "$labelObjectType$": "Benutzer", "CtxReconnectSession": "0000", "owncloudEnabled": "1", "displayName": "univention", "mailPrimaryAddress": null, "CtxMaxConnectionTime": "", "CtxCfgClientDrivers": "0", "locked": "none", "$options$": {"samba": true, "kerberos": true, "person": true, "posix": true, "mail": true, "pki": false, "ldap_pwd": false}, "CtxCfgTSLogon": "0", "gecos": "univention", "sambaRID": "5012"}]}

Note the "owncloudEnabled": "1"!
Comment 1 Jürn Brodersen univentionstaff 2017-07-25 12:47:24 CEST
Probably introduced in bug 41053
Comment 2 Jürn Brodersen univentionstaff 2017-07-25 13:08:31 CEST
According to bug 41829 a not set attribute should always be interpreted as false.
Comment 3 Florian Best univentionstaff 2017-07-25 13:40:41 CEST
(In reply to Jürn Brodersen from comment #1)
> Probably introduced in bug 41053
Yes!

We do NOT store a "0" in LDAP for extended attributes (not for hardcoded properties!) which have the "boolean" syntax class.
This causes that no state is detected and the default value is used.
Comment 4 Florian Best univentionstaff 2017-07-25 16:15:50 CEST
I have no clue how this could be fixed. IMHO, the best would be to change owncloud so that it uses a extended option and decides by the presence of the object class if the user is enabled.
Comment 5 Erik Damrose univentionstaff 2017-07-25 16:31:56 CEST
(In reply to Florian Best from comment #4)
> I have no clue how this could be fixed.

Is it possible to revert to the old behavior?

It could also be implemented that an unchecked attribute is stored as false/"0" in LDAP, but that may introduce other unexpected behavior.
Comment 6 Florian Best univentionstaff 2017-07-25 16:34:40 CEST
(In reply to Erik Damrose from comment #5)
> (In reply to Florian Best from comment #4)
> > I have no clue how this could be fixed.
> 
> Is it possible to revert to the old behavior?
Not without loosing the new feature of Bug #41053.

> It could also be implemented that an unchecked attribute is stored as
> false/"0" in LDAP, but that may introduce other unexpected behavior.
At least owncloud could do this, yes. We can't change this in UDM generically because then the usercrt-cool-solution breaks.
Comment 7 Erik Damrose univentionstaff 2017-07-25 18:04:06 CEST
> > Is it possible to revert to the old behavior?
> Not without loosing the new feature of Bug #41053.

But that feature is not even working for old users if a boolean syntax attribute is present.

If i understand correctly, bug #41053 fixed that the default values are always set upon opening a user if they are not present in LDAP. For a single boolean attribute, the UMC then detects no changes, and when i click on 'save', nothing is changed. In my example, i cannot activate a user for owncloud via UMC.

> > It could also be implemented that an unchecked attribute is stored as
> > false/"0" in LDAP, but that may introduce other unexpected behavior.
> At least owncloud could do this, yes. We can't change this in UDM
> generically because then the usercrt-cool-solution breaks.

I don't think it is ok that the new feature breaks several apps that use syntax=boolean
(At least owncloud and kopano use it, a quick search also found it in u-radius)
Comment 8 Jürn Brodersen univentionstaff 2017-07-25 18:05:20 CEST
Created attachment 9055 [details]
proposed patch

Currently it's broken for all extended attributes with syntax class boolean and default value 1.

I added a patch that only sets booleans to the default value for new objects. Would that break the cool solution?
Comment 9 Christina Scheinig univentionstaff 2017-07-26 11:37:55 CEST
The problem is reported again by a customer, who had to activate and deactivate new Kopano users
Comment 10 Erik Damrose univentionstaff 2017-07-26 11:52:04 CEST
(In reply to Christina Scheinig from comment #9)
> The problem is reported again by a customer, who had to activate and
> deactivate new Kopano users

Where can i find more information about that case? It currently seems to be only an issue with boolean syntax attributes. Kopano user activation is done with a different syntax (kopano4ucsRole, which is derived from the select syntax)
Comment 11 Christina Scheinig univentionstaff 2017-07-26 12:06:26 CEST
(In reply to Christina Scheinig from comment #9)
> The problem is reported again by a customer, who had to activate and
> deactivate new Kopano users

Sorry, this was a missleading information from a call. The circumstances were different here, so this was not the problem
Comment 12 Florian Best univentionstaff 2017-08-11 18:46:22 CEST
(In reply to Jürn Brodersen from comment #8)
> Created attachment 9055 [details]
> proposed patch
The patch fixes the wrong place. If you do udm[property] the problem exists again  - e.g. in UCS@school importer, OX hooks or whereever. A exception in __getitem__ would be necessary. But this might have other side effects.

As possible solution I would suggest adding a new syntax class 'boolean2' which writes a "0" to LDAP instead of leaving it blank. Or my favorite would be to remove the special handling for the "boolean" syntax completely, write always a "0" to LDAP and fix the cool solution/etc. so that it can handle "0".
Comment 13 Erik Damrose univentionstaff 2017-08-14 09:26:47 CEST
(In reply to Florian Best from comment #12)
> As possible solution I would suggest adding a new syntax class 'boolean2'
> which writes a "0" to LDAP instead of leaving it blank.

There are already other syntaxes that are doing this or something similar. I am hesitating to accept making boolean deprecated and having to update all occurences of the syntax as a solution.

(In reply to Florian Best from comment #12)
> Or my favorite would
> be to remove the special handling for the "boolean" syntax completely, write
> always a "0" to LDAP and fix the cool solution/etc. so that it can handle
> "0".

Of course, we would have to check if all current users of the syntax are still working with that change
Comment 14 Erik Damrose univentionstaff 2017-08-14 10:12:07 CEST
*** Bug 44866 has been marked as a duplicate of this bug. ***
Comment 15 Stephan Luft univentionstaff 2017-10-04 15:00:22 CEST
A partner testing/setting up UCS with owncloud/netxtcloud for several of his customers was affected by this problem and gave me feedback via phone
Comment 16 Florian Best univentionstaff 2017-10-10 18:52:03 CEST
I found a really simple solution and the real bug:

The values of extended attributes with boolean syntax simply weren't ever unmapped.
So self.oldinfo[prop] was never set and always contained "None" (or "1"). While we say that unset values (=None) equals to false ("0"). We treat them specially in _create(), __setitem__ and _ldap_object_classes but never really unmapped them.
Now they are unmapped: If they are unset the internal value is set to a "0".
The UDM-CLI now displays them also correct;

udm users/user list | grep -i -e DN: -e owncloudEn

Before:
DN: uid=b.bluemchen,cn=schueler,cn=users,dc=school,dc=local
  owncloudEnabled: None

DN: uid=b.bluemchen,cn=schueler,cn=users,dc=school,dc=local
  owncloudEnabled: 0

univention-directory-manager-modules (12.0.18-12):
7f6b091ffbff | Bug #45066: Merge branch 'fbest/45066-boolean-syntax' into 4.2-2
ba7cca533027 | Bug #45066: unmap boolean extended attributes so that default values don't overwrite the value

univention-directory-manager-modules.yaml:
7f6b091ffbff | Bug #45066: Merge branch 'fbest/45066-boolean-syntax' into 4.2-2
fdd1eae5f1ef | YAML Bug #45066
Comment 17 Erik Damrose univentionstaff 2017-10-12 10:23:36 CEST
OK: enable / disable boolean value with extended attribute via udm / UMC
OK: yaml
Verified
Comment 18 Arvid Requate univentionstaff 2017-10-12 14:50:57 CEST
<http://errata.software-univention.de/ucs/4.2/198.html>
Comment 19 Nico Gulden univentionstaff 2017-11-09 09:24:03 CET
*** Bug 45674 has been marked as a duplicate of this bug. ***