Bug 37171 - except in lookup() ignores every error for policies/* and settings/*
except in lookup() ignores every error for policies/* and settings/*
Status: CLOSED FIXED
Product: UCS
Classification: Unclassified
Component: UDM (Generic)
UCS 4.2
Other Linux
: P5 normal with 1 vote (vote)
: UCS 4.2-2-errata
Assigned To: Florian Best
Philipp Hahn
:
Depends on:
Blocks: 40854
  Show dependency treegraph
 
Reported: 2014-12-04 02:44 CET by Florian Best
Modified: 2017-11-01 13:49 CET (History)
2 users (show)

See Also:
What kind of report is it?: Development Internal
What type of bug is this?: ---
Who will be affected by this bug?: ---
How will those affected feel about the bug?: ---
User Pain:
Enterprise Customer affected?:
School Customer affected?:
ISV affected?:
Ticket number:
Bug group (optional): Cleanup, Error handling, Troubleshooting
Max CVSS v3 score:
best: Patch_Available+


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Florian Best univentionstaff 2014-12-04 02:44:43 CET
Is there a reason why many UDM modules catch everything in their lookup method without logging a exception/etc. ?
settings/lock, settings/license, settings/packages, …
113 »   try:
114 »   »   for dn, attrs in lo.search(unicode(filter), base, scope, [], unique, required, timeout, sizelimit):
115 »   »   »   res.append( object( co, lo, None, dn, attributes = attrs ) )
116 »   except:
117 »   »   pass

It costs time in debugging things...
Comment 1 Florian Best univentionstaff 2014-12-04 02:53:23 CET
$ for file in $(find -name '*.py'); do sed -ne '/def lookup(/,/def /p' "$file" | tr '\n' ' ' | grep 'except:\s\+pass' >/dev/null && echo $file; done                                                                                             
./legacy/policies/clientdevices.py
./legacy/policies/managedclientpackages.py
./legacy/policies/mobileclientpackages.py
./legacy/policies/sound.py
./legacy/policies/thinclient.py
./legacy/policies/xfree.py
./policies/admin_container.py
./policies/autostart.py
./policies/desktop.py
./policies/dhcp_boot.py
./policies/dhcp_dns.py
./policies/dhcp_dnsupdate.py
./policies/dhcp_leasetime.py
./policies/dhcp_netbios.py
./policies/dhcp_routing.py
./policies/dhcp_scope.py
./policies/dhcp_statements.py
./policies/ldapserver.py
./policies/mailquota.py
./policies/maintenance.py
./policies/masterpackages.py
./policies/memberpackages.py
./policies/nfsmounts.py
./policies/print_quota.py
./policies/printserver.py
./policies/pwhistory.py
./policies/registry.py
./policies/release.py
./policies/repositoryserver.py
./policies/repositorysync.py
./policies/share_userquota.py
./policies/slavepackages.py
./policies/umc.py
./settings/license.py
./settings/lock.py
./settings/packages.py
./settings/printermodel.py
./settings/printeruri.py
./settings/prohibited_username.py
./settings/umc_operationset.py
Comment 2 Alexander Kläser univentionstaff 2015-02-10 15:00:05 CET
(In reply to Florian Best from comment #0)
> Is there a reason why many UDM modules catch everything in their lookup
> method without logging a exception/etc. ?
> settings/lock, settings/license, settings/packages, …
> 113 »   try:
> 114 »   »   for dn, attrs in lo.search(unicode(filter), base, scope, [],
> unique, required, timeout, sizelimit):
> 115 »   »   »   res.append( object( co, lo, None, dn, attributes = attrs ) )
> 116 »   except:
> 117 »   »   pass
> 
> It costs time in debugging things...

Instead of "pass" the traceback could simply be logged.
Comment 3 Florian Best univentionstaff 2016-09-26 13:13:22 CEST
The impact of this bug is if there is 1 object which is broken - for whatever reason - no other object of that type is found anymore! This leads to e.g. wrong evaluation of policies or settings.

The reason why a object might be broken are:
* due to programmatically errors during development or corner cases of the environment
* invalid data in LDAP causing the validation or parsing to fail
* runtime errors like ldap connections down/timeout, IOError's/EnvironmentErrors…

As these errors are silently ignored we probably will also never receive feedback about this (or only feedback like "printing doesn't work" / "policies weren't evaluated").
Comment 4 Florian Best univentionstaff 2017-07-06 12:14:50 CEST
I suggest to change all lookup methods to the following style:

def lookup(co, lo, filter_s, base='', superordinate=None, scope='sub', unique=False, required=False, timeout=-1, sizelimit=0):
filter = lookup_filter(filter_s)
    result = []
    for dn, attrs in lo.search(unicode(filter), base, scope, [], unique, required, timeout, sizelimit):
        try:
            res.append(object(co, lo, None, dn, attributes=attrs))
        except univention.admin.uexception.base as exc:
            ud.debug(ud.ADMIN, ud.ERROR, 'object %s is broken: %s' % (dn, exc))
    return res
Comment 5 Florian Best univentionstaff 2017-08-01 16:38:25 CEST
This causes especially the following wrong behavior (e.g. Bug #45116):

>>> import univention.admin
>>> lo,po=univention.admin.uldap.getMachineConnection()
>>> univention.admin.modules.update()

E.g. expected behavior for a user:
>>> u = univention.admin.modules.get('users/user')
>>> u.lookup(None, lo, '', base='cn=DOESNOTEXISTS,dc=school,dc=local', required=True)[0]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/pymodules/python2.7/univention/admin/handlers/users/user.py", line 2846, in lookup
    for dn, attrs in lo.search(unicode(filter), base, scope, [], unique, required, timeout, sizelimit):
  File "/usr/lib/pymodules/python2.7/univention/admin/uldap.py", line 425, in search
    raise univention.admin.uexceptions.noObject(_err2str(msg))
univention.admin.uexceptions.noObject: No such object

Broken behavior for policies:

>>> p = univention.admin.modules.get('policies/registry')
>>> p.lookup(None, lo, '', base='cn=DOESNOTEXISTS,dc=school,dc=local', required=True)[0]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
IndexError: list index out of range
Comment 6 Florian Best univentionstaff 2017-10-27 16:42:31 CEST
The suggested idea from comment #4 has been implemented:

univention-directory-manager-modules (12.0.18-13)
f7ac2fc26c8a | Bug #37171: Merge branch 'fbest/37171-lookup-except-all' into 4.2-2
0ef4cfcb48f4 | Bug #37171: use the simpleLDAP.lookup() method in dhcp/common
67a350f4ceb9 | Bug #37171: use generic lookup method which does error handling
cdce74679c37 | Bug #37171: it is not required to catch IndexError anymore

univention-directory-manager-modules.yaml
f7ac2fc26c8a | Bug #37171: Merge branch 'fbest/37171-lookup-except-all' into 4.2-2
375cf27bbea4 | YAML Bug #37171
Comment 7 Philipp Hahn univentionstaff 2017-10-30 11:22:24 CET
OK: errata-announce -V --only univention-directory-manager-modules.yaml
FIXED: univention-directory-manager-modules.yaml → 602919ea99
OK: Jenkins
OK: Code-Review
OK: cdce74679c37 67a350f4ceb9 0ef4cfcb48f4
Comment 8 Arvid Requate univentionstaff 2017-11-01 13:49:18 CET
<http://errata.software-univention.de/ucs/4.2/206.html>