Univention Bugzilla – Full Text Bug Listing |
Summary: | [UDM HTTP API] Implement generic data types in UDM which replace the use of the Simple UDM API encoders | ||
---|---|---|---|
Product: | UCS | Reporter: | Florian Best <best> |
Component: | UDM - REST API | Assignee: | Florian Best <best> |
Status: | CLOSED FIXED | QA Contact: | Felix Botner <botner> |
Severity: | enhancement | ||
Priority: | P3 | CC: | hahn, troeder |
Version: | UCS 4.4 | ||
Target Milestone: | UCS 4.4-1-errata | ||
Hardware: | Other | ||
OS: | Linux | ||
See Also: |
https://forge.univention.org/bugzilla/show_bug.cgi?id=40854 https://forge.univention.org/bugzilla/show_bug.cgi?id=38762 https://forge.univention.org/bugzilla/show_bug.cgi?id=27816 |
||
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?: | ||
Waiting Support: | Flags outvoted (downgraded) after PO Review: | ||
Ticket number: | Bug group (optional): | ||
Max CVSS v3 score: | |||
Bug Depends on: | |||
Bug Blocks: | 50178, 50172 |
Description
Florian Best
2019-08-23 22:45:41 CEST
Removing the support for bash-like data types (str / int) and accept only Python types (bool, int, list, dict, datatime etc.) is IMHO very important work! It is API breaking though. IMHO it should be considered the default for the UDM-Python-API for UCS 5, but it can be implemented as already usable _now_ by the UDM REST API: The simple UDM API hides all that from the user and was never intended as a library, so it is difficult to reuse the code. Moving it into UDM would make it usable, and improvements available, to all four: simple UDM API, UDM-REST-API-in-UCS-4, Python-UDM-in-UCS-5 and the UCS@school HTTP APIs input validators. AFAIK the best Python date parsing library is the dateutils library: https://pypi.org/project/python-dateutil/ (In reply to Daniel Tröder from comment #1) > AFAIK the best Python date parsing library is the dateutils library: > https://pypi.org/project/python-dateutil/ I don't find any library which can genericly parse our stupid iso8601Date values: >>> from dateutil.parser import parse >>> parse('2009-213') Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/usr/lib/python2.7/dist-packages/dateutil/parser.py", line 1164, in parse return DEFAULTPARSER.parse(timestr, **kwargs) File "/usr/lib/python2.7/dist-packages/dateutil/parser.py", line 574, in parse if cday > monthrange(cyear, cmonth)[1]: File "/usr/lib/python2.7/calendar.py", line 120, in monthrange raise IllegalMonthError(month) calendar.IllegalMonthError: bad month number 213; must be 1-12 >>> parse('2009-05') datetime.datetime(2009, 5, 22, 0, 0) >>> parse('2009-05-13') datetime.datetime(2009, 5, 13, 0, 0) >>> parse('2009-W21') Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/usr/lib/python2.7/dist-packages/dateutil/parser.py", line 1164, in parse return DEFAULTPARSER.parse(timestr, **kwargs) File "/usr/lib/python2.7/dist-packages/dateutil/parser.py", line 555, in parse raise ValueError("Unknown string format") ValueError: Unknown string format >>> parse('2009-W21-4') Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/usr/lib/python2.7/dist-packages/dateutil/parser.py", line 1164, in parse return DEFAULTPARSER.parse(timestr, **kwargs) File "/usr/lib/python2.7/dist-packages/dateutil/parser.py", line 555, in parse raise ValueError("Unknown string format") ValueError: Unknown string format OK... I hadn't expected this zoo. The date formats can be distinguish using regex and can be parsed with the standard library: (In reply to Florian Best from comment #2) > (In reply to Daniel Tröder from comment #1) > > AFAIK the best Python date parsing library is the dateutils library: > > https://pypi.org/project/python-dateutil/ > I don't find any library which can genericly parse our stupid iso8601Date > values: > >>> from dateutil.parser import parse > >>> parse('2009-213') re.match(r"\d+-\d+$", "2009-213") datetime.datetime.strptime("2009-213", "%Y-%j") datetime.datetime(2009, 8, 1, 0, 0) > >>> parse('2009-W21') re.match(r"\d+-W\d+$", "2009-W21") raise RuntimeError("This date string is hilarious!") A date cannot be specified with a week number without a day. Even the Python docs say "When used with the strptime() method, %U and %W are only used in calculations when the day of the week and the year are specified." > >>> parse('2009-W21-4') re.match(r"\d+-W\d+-\d+$", "2009-W21-4") datetime.datetime.strptime("2009-W21-4", "%Y-W%U-%w") datetime.datetime(2009, 5, 28, 0, 0) (In reply to Daniel Tröder from comment #3) > A date cannot be specified with a week number without a day. > Even the Python docs say "When used with the strptime() method, %U and %W > are only used in calculations when the day of the week and the year are > specified." I guess that not specifying a day means day=0. Not sure. Raising an exception if the format doesn't match is probably not a good idea. Especially not when we support this format. We should be graceful in what comes from the LDAP and we *could* be strict in what we accept in the future i.e. reject everything which doesn't conform to "food" values. This would mean that if we cannot parse the value from an ldap-attribute/udm-property we just send what's there. We have UDM properties with syntax for integer or LDAP DN, etc. and their belonging LDAP attribute has a string syntax set, so that it would allow invalid inputs to exists in the LDAP dirctory. This *must not* break the functionality of UDM, otherwise users with LDAP write access can cause Denial of Service (Bug #40854). And when the objects is PUT for modification with that same value, it is rejected, because it doesn't fit anymore. RFC ?!? (In reply to Florian Best from comment #4) > (In reply to Daniel Tröder from comment #3) > > A date cannot be specified with a week number without a day. > > Even the Python docs say "When used with the strptime() method, %U and %W > > are only used in calculations when the day of the week and the year are > > specified." > I guess that not specifying a day means day=0. Not sure. > > Raising an exception if the format doesn't match is probably not a good > idea. Especially not when we support this format. > We should be graceful in what comes from the LDAP and we *could* be strict > in what we accept in the future i.e. reject everything which doesn't conform > to "food" values. > This would mean that if we cannot parse the value from an > ldap-attribute/udm-property we just send what's there. > We have UDM properties with syntax for integer or LDAP DN, etc. and their > belonging LDAP attribute has a string syntax set, so that it would allow > invalid inputs to exists in the LDAP dirctory. > This *must not* break the functionality of UDM, otherwise users with LDAP > write access can cause Denial of Service (Bug #40854). > > And when the objects is PUT for modification with that same value, it is > rejected, because it doesn't fit anymore. > > RFC ?!? Instead of us also sending invalid data - and thus causing errors in clients - and thus creating a chain of workarounds - we should send a default value, when we encounter invalid data. The default value means, that we 'ignore' the invalid data. Like that we don't send invalid data and at the same time we don't choke on invalid data ourselves. *** Bug 50027 has been marked as a duplicate of this bug. *** Any opinions about the shares/share properties which use the syntax class UNIX_AccessRight / UNIX_AccessRight_extended: They are currently strings like "0755" or "20755". Should we transmit them as octal integers? 0755 → 493 2755 → 8685 The string version is more human readable. The integer version is more machine readable(?). With both versions looking like integers to me (the user), and being used to those numbers on the command line, I would always interpret them intuitively as octals (0755). Transforming them to base10 (493) will confuse me, because its not the way it has ever been presented to me. So in this case I vote for the user friendly variant, because 1) I think it's safer (less misunderstandings and thus bad values) and 2) it can be safely parsed. (In reply to Florian Best from comment #7) > Any opinions about the shares/share properties which use the syntax class > UNIX_AccessRight / UNIX_AccessRight_extended: > > They are currently strings like "0755" or "20755". Hopefully 0o2755 ... > Should we transmit them as octal integers? > 0755 → 493 > 2755 → 8685 > > The string version is more human readable. The integer version is more > machine readable(?). (In reply to Daniel Tröder from comment #8) > With both versions looking like integers to me (the user), and being used to > those numbers on the command line, I would always interpret them intuitively > as octals (0755). Transforming them to base10 (493) will confuse me, because > its not the way it has ever been presented to me. > So in this case I vote for the user friendly variant, because 1) I think > it's safer (less misunderstandings and thus bad values) and 2) it can be > safely parsed. The REST-API is for machines and should be as easy to handle as possible: especially no external knowledge should be required to do it correctly ("attribute X is baseY encoded, so you cannot upload your binary data, but first must apply algorithm Z to be able to do at. And for attribute A,B,C you need other special handling...") REST gains its power because you can combine the APIs from different applications and can build mashups easily. That special knowledge should only be necessary for the presentation layer. Humans should use UMC. I quick search shows for example that Hadop uses "stringly octal": <https://hadoop.apache.org/docs/r1.0.4/webhdfs.html> > "permission": > { > "description": "The permission represented as a octal string.", > "type" : "string" libvirt/docs/schemas/basictypes.rng also uses octal for permissions, but uses "int" restricted to [0-7]+ for the definition. More correctly that would be "string". So I would recommend to got with "string with octal" as JavaScript like Python interprets integers literals starting with "0" as octal. And getting back another value than sent looks confusing. It is human understandable, the backend can easily do the conversion and for REST it is just a string. (In reply to Philipp Hahn from comment #9) > The REST-API is for machines and should be as easy to handle as possible: That's only half the story. The data format has been chosen to be JSON, because it is well readable by humans. Developers need to interpret the sent data, so their programs do the correct thing with it. > especially no external knowledge should be required to do it correctly 1. External knowledge is always required, because everything is encoded somehow. 2. Furthermore data becomes information only in the scope of a certain context, which again is knowledge that has to be shared. That's why I suggest to use something that (hopefully) meets the users expectation. If it does, we take advantage of the users prior experience with similar data in a similar situation, which will allow him to understand what the data representation means. (Just to be sure: I am also in favor of "The permission represented as a octal string.".) Do we want properties with syntax class "UNIX_TimeInterval" as dictionary or as list?: "ttl": { "amount": "3", "unit": "hours" }, VS "ttl": [ "3", "hours" ], Affected properties: dns/alias zonettl dns/forward_zone expire dns/forward_zone refresh dns/forward_zone retry dns/forward_zone ttl dns/forward_zone zonettl dns/host_record zonettl dns/ns_record zonettl dns/reverse_zone expire dns/reverse_zone refresh dns/reverse_zone retry dns/reverse_zone ttl dns/reverse_zone zonettl dns/srv_record zonettl dns/txt_record zonettl policies/dhcp_leasetime lease_time_default policies/dhcp_leasetime lease_time_max policies/dhcp_leasetime lease_time_min settings/sambaconfig disconnectTime settings/sambaconfig lockoutDuration settings/sambaconfig maxPasswordAge settings/sambaconfig minPasswordAge settings/sambadomain maxPasswordAge settings/sambadomain minPasswordAge settings/sambadomain disconnectTime settings/sambadomain lockoutDuration I made some progress: git:fbest/50047-udm-rest-typing QA: you can start reviewing the branch. Some simple changes will follow but the basic structure is finished and working. All the types are compatible to the Simple UDM API. Here is some listing of some values which are of type dictionary: # __udm mail/folder list DN: cn=test@example.com,l=school,l=dev URL: https://master100.school.dev/univention/udm/mail/folder/cn%3Dtest%40example.com%2Cl%3Dschool%2Cl%3Ddev Object-Type: mail/folder mailDomain: example.com mailHomeServer: nowhere mailPrimaryAddress: mailQuota: name: test sharedFolderGroupACL: {} sharedFolderUserACL: { "foo@bar": "none" } # __udm users/user list DN: uid=fbest,l=school,l=dev URL: https://master100.school.dev/univention/udm/users/user/uid%3Dfbest%2Cl%3Dschool%2Cl%3Ddev Object-Type: users/user … homePostalAddress: {"city": "city", "street": "street", "zipcode": "12345"} homePostalAddress: {"city": "gotham", "street": "elsewhere", "zipcode": "56789"} sambaLogonHours: Sun 9-10 sambaLogonHours: Sun 19-20 sambaLogonHours: Mon 17-18 sambaLogonHours: Tue 8-9 sambaLogonHours: Tue 15-16 umcProperty: { "foo": "bar", "bar": "baz" } # __udm computers/domaincontroller_master list DN: cn=master100,cn=dc,cn=computers,l=school,l=dev URL: https://master100.school.dev/univention/udm/computers/domaincontroller_master/cn%3Dmaster100%2Ccn%3Ddc%2Ccn%3Dcomputers%2Cl%3Dschool%2Cl%3Ddev Object-Type: computers/domaincontroller_master … dnsAlias: f8c007f8-1bfe-428a-8157-ee21001dd7bc._msdcs dnsEntryZoneAlias: {"alias": "f8c007f8-1bfe-428a-8157-ee21001dd7bc._msdcs", "forward-zone": "zoneName=school.dev,cn=dns,l=school,l=dev", "zone": "school.dev"} dnsEntryZoneForward: {"ip": "10.200.27.100", "forward-zone": "zoneName=school.dev,cn=dns,l=school,l=dev"} dnsEntryZoneReverse: {"ip": "10.200.27.100", "reverse-zone": "zoneName=27.200.10.in-addr.arpa,cn=dns,l=school,l=dev"} # __udm policies/registry list DN: cn=ou-default-ucr-policy,cn=policies,ou=anncpq3oxe69,l=school,l=dev URL: https://master100.school.dev/univention/udm/policies/registry/cn%3Dou-default-ucr-policy%2Ccn%3Dpolicies%2Cou%3Danncpq3oxe69%2Cl%3Dschool%2Cl%3Ddev Object-Type: policies/registry ldapFilter: name: ou-default-ucr-policy registry: { "ucc/cups/server": "master100.school.dev", "ucc/italc/key/sambasource": "\\\\master100\\netlogon", "ucc/proxy/http": "http://master100.school.dev:3128", "ucc/mount/cifshome/server": "master100.school.dev", "ucc/italc/key/filename": "italc-key_master100.pub", "ucc/timeserver": "master100.school.dev", "dhcpd/ldap/base": "cn=dhcp,ou=anncpq3oxe69,l=school,l=dev" } # __udm settings/portal list --filter cn=domain DN: cn=domain,cn=portal,cn=univention,l=school,l=dev URL: https://master100.school.dev/univention/udm/settings/portal/cn%3Ddomain%2Ccn%3Dportal%2Ccn%3Dunivention%2Cl%3Dschool%2Cl%3Ddev Object-Type: settings/portal anonymousEmpty: { "de_DE": "Leer\n", "en_US": "empty\n" } autoLayoutCategories: False background: content: ["cn=service,cn=categories,cn=portal,cn=univention,l=school,l=dev", ["cn=teacherconsole,cn=portal,cn=univention,l=school,l=dev", "cn=egroupware,cn=portal,cn=univention,l=school,l=dev", "cn=owncloud,cn=portal,cn=univention,l=school,l=dev", "cn=self-service,cn=portal,cn=univention,l=school,l=dev"]] content: ["cn=admin,cn=categories,cn=portal,cn=univention,l=school,l=dev", ["cn=umc-domain,cn=portal,cn=univention,l=school,l=dev", "cn=egroupware-adm,cn=portal,cn=univention,l=school,l=dev", "cn=opsi-server,cn=portal,cn=univention,l=school,l=dev", "cn=nagios,cn=portal,cn=univention,l=school,l=dev"]] cssBackground: defaultLinkTarget: samewindow displayName: { "de_DE": "Univention Portal DE", "en_US": "Univention Portal EN", "fr_FR": "Portail Univention FR" } # __udm settings/udm_hook list DN: cn=ucsschool_user_options,cn=udm_hook,cn=univention,l=school,l=dev URL: https://master100.school.dev/univention/udm/settings/udm_hook/cn%3Ducsschool_user_options%2Ccn%3Dudm_hook%2Ccn%3Dunivention%2Cl%3Dschool%2Cl%3Ddev Object-Type: settings/udm_hook active: True data: base64== filename: ucsschool_user_options.py messagecatalog: { "de": "base64==" } name: ucsschool_user_options package: ucs-school-import packageversion: 17.0.10A~4.4.0.201908051526 ucsversionend: ucsversionstart: I suggest something like this for the QA: import subprocess import univention.admin.modules univention.admin.modules.update() c = len(univention.admin.modules.modules) for i, mod in enumerate(sorted(univention.admin.modules.modules)): cmd = '__udm %s list > %s.txt' % (mod, mod.replace('/', '_')) print('%s # %s/%s' % (cmd, i, c)) subprocess.call(cmd, shell=True) TODO: users/user: certificateDateNotBefore: if the string is empty a valueError exception is raised during decode(). (In reply to Florian Best from comment #12) > Do we want properties with syntax class "UNIX_TimeInterval" as dictionary or > as list?: > > "ttl": { > "amount": "3", > "unit": "hours" > }, > VS > "ttl": [ > "3", > "hours" > ], I generally prefer the version that is less ambiguous. In this case both versions are OK. In the version with the list, technically order matters, but not semantically. The dict-version is better in that different types can be used. So "amount" can be an int, instead of a string. (Ofc a list can have different types too, but that's just asking for problems.) But that's all besides the point: The pythonic answer is to use a timedelta object. (In reply to Florian Best from comment #12) > Do we want properties with syntax class "UNIX_TimeInterval" as dictionary or > as list?: > > "ttl": { > "amount": "3", > "unit": "hours" > }, > VS > "ttl": [ > "3", > "hours" > ], just seconds. It's a timespan and the SI unit for that is seconds. That syntax class only displays them as something human readable, but fails miserable if it is not a multiple of an exact minute/hour/day - it will fall back to display seconds for something like "1*60*60+1". (In reply to Philipp Hahn from comment #15) > (In reply to Florian Best from comment #12) > > Do we want properties with syntax class "UNIX_TimeInterval" as dictionary or > > as list?: > > > > "ttl": { > > "amount": "3", > > "unit": "hours" > > }, > > VS > > "ttl": [ > > "3", > > "hours" > > ], > > just seconds. It's a timespan and the SI unit for that is seconds. That > syntax class only displays them as something human readable, but fails > miserable if it is not a multiple of an exact minute/hour/day - it will fall > back to display seconds for something like "1*60*60+1". Thanks, seconds is very good. Implemented in 2bc31b064c907b762a29f10997a58de5681dce8d. There was also the discussion in gitlab about the name of the python module: * typing is a stdlib module * types is a stdlib module Should we find another name? (In reply to Florian Best from comment #17) > There was also the discussion in gitlab about the name of the python module: > * typing is a stdlib module > * types is a stdlib module > > Should we find another name? I think "univention.admin.types" is the best choice: "typing" would clash more often with the "mypy" annotations and most uses of "types" in the current core are dubios at best. The changes have been merged: univention-management-console-module-udm (9.0.12-35) eae5cae25864 | Bug #50047: replace Simple UDM property encoders with the new UDM types interface univention-directory-manager-modules (14.0.13-14) 8d6d51541adc | Bug #50047: debian changelog univention-directory-manager-modules (14.0.13-13) 728a2e16bfa9 | Bug #50047: Use new univention.admin.types in syntax.py dd5267ecac58 | Bug #50047: Introduce univention.admin.types interface 90e11cac3495 | Bug #50047: syntax: date syntaxes can now be converted to python-datetime objects 96c4f1e6ea7b | Bug #50047: Fix detection of options in settings/license 6a63e5041868 | Bug #50047: Remove unnecessary use of "types" module b32b50fd27c6 | Bug #50047: Add new package dependencies 1e16e31f9a68 | Bug #50047: Fix python-support in postinst 51a0f051f696 | Bug #50047: syntax "integer" can now handle non-strings eda2d26915fb | Bug #50047: add names for subsyntaxes of complex syntax The changes have not been merged into the simple Python UDM API. The simple Python UDM API is the only public Python API of UCS and must be kept up to date, including the pyi files! (In reply to Daniel Tröder from comment #20) > The changes have not been merged into the simple Python UDM API. > The simple Python UDM API is the only public Python API of UCS and must be > kept up to date, including the pyi files! I created Bug #50178 for this. please update univention-directory-manager-modules.yaml the yaml should mention this bug here (not 50034) and also update the message (added new type properties to the udm syntax class or something like that) (In reply to Felix Botner from comment #22) > please update univention-directory-manager-modules.yaml > > the yaml should mention this bug here (not 50034) and also update the > message (added new type properties to the udm syntax class or something like > that) OK, fixed the YAML entry. On this bug we only check the availability of "generic data types" in the UDM REST API. No in-depth analysis of the software architecture (can/should be done later). Also i do not check every single property and type, just if it works general. OK - yaml OK - type definition in python-univention-directory-manager OK - types are used in the UDM REST API OK - jenkins tests OK - package dependencies $ curl -s 'https://Administrator:univention@master.four.four/univention/udm/users/user/uid=Administrator,cn=users,dc=four,dc=four' -H 'Accept: application/json' | jq { "dn": "uid=Administrator,cn=users,dc=four,dc=four", "properties": { "mobileTelephoneNumber": [], "objectFlag": [], "sambahome": null, "umcProperty": { "appcenterDockerSeen": "false", "appcenterSeen": "2", "favorites": "appcenter:appcenter,updater,udm:users/user,udm:groups/group,udm:computers/computer,apps:egroupware" }, "overridePWLength": null, "uidNumber": 2002, "disabled": false, "unlock": false, "street": null, "postcode": null, "scriptpath": null, "sambaPrivileges": [], "homeTelephoneNumber": [], "homePostalAddress": [], "city": null, $ curl -s 'https://Administrator:univention@master.four.four/univention/udm/users/user/uid=Administrator,cn=users,dc=four,dc=four' -X PUT -d '{"position": "cn=users,dc=four,dc=four", "properties": {"description": 1}}' -H 'Accept: application/json' -H "Content-Type: application/json" | jq { "_links": { "curies": [ { "href": "https://master.four.four/univention/udm/relation/{rel}", "name": "udm", "templated": true } ] }, "error": { "message": "1 error(s) occurred:\nRequest argument \"description\" The property description has an invalid value: Value must be of type string not int.\n", "code": 422, "traceback": null, "error": { "description": "The property description has an invalid value: Value must be of type string not int." }, "title": "Unprocessable Entity" } } $ curl -s 'https://Administrator:univention@master.four.four/univention/udm/users/user/uid=Administrator,cn=users,dc=four,dc=four' -X PUT -d '{"position": "cn=users,dc=four,dc=four", "properties": {"description": "aaa"}}' -H 'Accept: application/json' -H "Content-Type: application/json" | jq { "_links": { "curies": [ { "href": "https://master.four.four/univention/udm/relation/{rel}", "name": "udm", "templated": true } ] } } *** Bug 50184 has been marked as a duplicate of this bug. *** |