#50005 Ticket 50004 - lib389 - improve X-ORIGIN schema parsing
Closed 3 years ago by spichugi. Opened 5 years ago by mreynolds.
mreynolds/389-ds-base ticket50004  into  master

file modified
+6 -5
@@ -1,4 +1,4 @@ 

- #!/usr/bin/python

+ #!/usr/bin/python3

  #

  # --- BEGIN COPYRIGHT BLOCK ---

  # Copyright (C) 2016 Red Hat, Inc.
@@ -122,10 +122,11 @@ 

  

      for root, dirs, files in os.walk(tests_dir):

          for name in files:

-             with open(os.path.join(root, name), "r") as file:

-                 for line in file:

-                     if re.search(str(id_value), line):

-                         return False

+             if name.endswith('.py'):

+                 with open(os.path.join(root, name), "r") as cifile:

+                     for line in cifile:

+                         if re.search(str(id_value), line):

+                             return False

  

      return True

  

@@ -127,7 +127,7 @@ 

          }));

      });

    }).fail(function(select_data) {

-       console.log("failed: " + select_data.message);

+       console.log("Get schema failed: " + select_data.message);

        check_inst_alive(1);

    });

  }
@@ -219,7 +219,7 @@ 

        }]

      });

    }).fail(function(oc_data) {

-       console.log("failed: " + oc_data.message);

+       console.log("Get objectclasses failed: " + oc_data.message);

        check_inst_alive(1);

    });

  
@@ -342,12 +342,12 @@ 

          }]

        });

      }).fail(function(at_data) {

-         console.log("failed: " + at_data.message);

+         console.log("Get attributes failed: " + at_data.message);

          check_inst_alive(1);

      });

  

    }).fail(function(syntax_data) {

-       console.log("failed: " + syntax_data.message);

+       console.log("Get syntaxes failed: " + syntax_data.message);

        check_inst_alive(1);

    });

  
@@ -379,7 +379,7 @@ 

      console.log("Finished loading schema.");

  

    }).fail(function(mr_data) {

-       console.log("failed: " + mr_data.cmd);

+       console.log("Get matching rules failed: " + mr_data.cmd);

        check_inst_alive(1);

    });

  }
@@ -498,7 +498,7 @@ 

          }).

          fail(function(oc_data) {

            popup_err("err", oc_data.message);

-           console.log("failed: " + oc_data.message);

+           console.log("Search objectclasses failed: " + oc_data.message);

            check_inst_alive(1);

          });

          // Replace the option in 'Edit objectClass' window
@@ -693,7 +693,7 @@ 

          }).

          fail(function(at_data) {

            popup_err("err", at_data.message);

-           console.log("failed: " + at_data.message);

+           console.log("Query attributes failed: " + at_data.message);

            check_inst_alive(1);

          });

          if (!edit) {

file modified
+31 -16
@@ -13,6 +13,7 @@ 

  import glob

  import ldap

  import ldif

+ import re

  from itertools import count

  from json import dumps as dump_json

  from operator import itemgetter
@@ -41,7 +42,7 @@ 

                   "1.3.6.1.4.1.1466.115.121.1.7": "Boolean",

                   "1.3.6.1.4.1.1466.115.121.1.11": "Country String",

                   "1.3.6.1.4.1.1466.115.121.1.12": "DN",

-                  "1.3.6.1.4.1.1466.115.121.1.14":  "Delivery Method",

+                  "1.3.6.1.4.1.1466.115.121.1.14": "Delivery Method",

                   "1.3.6.1.4.1.1466.115.121.1.15": "Directory String",

                   "1.3.6.1.4.1.1466.115.121.1.21": "Enhanced Guide",

                   "1.3.6.1.4.1.1466.115.121.1.22": "Facsimile",
@@ -64,6 +65,9 @@ 

                   "1.3.6.1.4.1.1466.115.121.1.52": "Telex Number"}

  

  

+ X_ORIGIN_REGEX = r'\'(.*?)\''

+ 

+ 

  class Schema(DSLdapObject):

      """An object that represents the schema entry

  
@@ -113,35 +117,46 @@ 

          return result

  

      def _get_schema_objects(self, object_model, json=False):

+         """Get all the schema objects for a specific model: Attribute, Objectclass,

+         or Matchingreule.

+         """

          attr_name = self._get_attr_name_by_model(object_model)

- 

          results = self.get_attr_vals_utf8(attr_name)

  

          if json:

-             object_insts = [vars(object_model(obj_i)) for obj_i in results]

- 

-             for obj_i in object_insts:

-                 # Add normalized name for sorting. Some matching rules don't have a name

+             object_insts = []

+             for obj in results:

+                 obj_i = vars(object_model(obj))

                  if len(obj_i["names"]) == 1:

-                     obj_i['name'] = obj_i['names'][0]

+                     obj_i['name'] = obj_i['names'][0].lower()

                      obj_i['aliases'] = ""

                  elif len(obj_i["names"]) > 1:

-                     obj_i['name'] = obj_i['names'][0]

+                     obj_i['name'] = obj_i['names'][0].lower()

                      obj_i['aliases'] = obj_i['names'][1:]

                  else:

                      obj_i['name'] = ""

+ 

                  # Temporary workaround for X-ORIGIN in ObjectClass objects.

                  # It should be removed after https://github.com/python-ldap/python-ldap/pull/247 is merged

-                 if "x_origin" not in obj_i and object_model != MatchingRule:

-                     for object_str in results:

-                         if obj_i['names'] == vars(object_model(object_str))['names'] and "X-ORIGIN" in object_str:

-                             obj_i['x_origin'] = object_str.split("X-ORIGIN '")[1].split("'")[0]

-             object_insts = sorted(object_insts, key=itemgetter('name'))

-             result = {'type': 'list', 'items': object_insts}

+                 if " X-ORIGIN " in obj and obj_i['names'] == vars(object_model(obj))['names']:

+                     remainder = obj.split(" X-ORIGIN ")[1]

+                     if remainder[:1] == "(":

+                         # Have multiple values

+                         end = remainder.find(')')

+                         vals = remainder[1:end]

+                         vals = re.findall(X_ORIGIN_REGEX, vals)

+                         # For now use the first value, but this should be a set (another bug in python-ldap)

+                         obj_i['x_origin'] = vals[0]

+                     else:

+                         # Single X-ORIGIN value

+                         obj_i['x_origin'] = obj.split(" X-ORIGIN ")[1].split("'")[1]

+                 object_insts.append(obj_i)

  

-             return result

+             object_insts = sorted(object_insts, key=itemgetter('name'))

+             return {'type': 'list', 'items': object_insts}

          else:

-             return [object_model(obj_i) for obj_i in results]

+             object_insts = [object_model(obj_i) for obj_i in results]

+             return sorted(object_insts, key=lambda x: x.names, reverse=False)

  

      def _get_schema_object(self, name, object_model, json=False):

          objects = self._get_schema_objects(object_model, json=json)

@@ -0,0 +1,1 @@ 

+ # -*- coding: utf-8 -*-

@@ -1,72 +1,121 @@ 

  # --- BEGIN COPYRIGHT BLOCK ---

- # Copyright (C) 2015 Red Hat, Inc.

+ # Copyright (C) 2018 Red Hat, Inc.

  # All rights reserved.

  #

  # License: GPL (version 3 or any later version).

  # See LICENSE for details.

  # --- END COPYRIGHT BLOCK ---

  #

- import pytest

- 

- from lib389 import DirSrv

- from lib389._constants import SER_HOST, SER_PORT, SER_SERVERID_PROP, LOCALHOST

- from lib389.schema import Schema

- 

- INSTANCE_PORT = 54321

- INSTANCE_SERVERID = 'standalone'

- 

- 

- class TopologyInstance(object):

-     def __init__(self, instance):

-         instance.open()

-         self.instance = instance

  

  

- @pytest.fixture(scope="module")

- def topology(request):

-     instance = DirSrv(verbose=False)

-     instance.log.debug("Instance allocated")

-     args = {SER_HOST: LOCALHOST,

-             SER_PORT: INSTANCE_PORT,

-             SER_SERVERID_PROP: INSTANCE_SERVERID}

-     instance.allocate(args)

-     if instance.exists():

-         instance.delete()

-     instance.create()

-     instance.open()

+ import logging

+ import pytest

+ import os

+ from lib389._constants import *

+ from lib389.schema import Schema

+ from lib389.topologies import topology_st as topo

  

-     def fin():

-         instance.delete()

-     request.addfinalizer(fin)

+ DEBUGGING = os.getenv("DEBUGGING", default=False)

+ if DEBUGGING:

+     logging.getLogger(__name__).setLevel(logging.DEBUG)

+ else:

+     logging.getLogger(__name__).setLevel(logging.INFO)

+ log = logging.getLogger(__name__)

  

-     return TopologyInstance(instance)

  

+ def test_schema(topo):

+     """Basic schema querying test

  

- def test_schema(topology):

-     must_expect = ['uidObject', 'account', 'posixAccount', 'shadowAccount']

+     :id: 995acc60-243b-45b0-9c1c-12bbe6a2882f

+     :setup: Standalone Instance

+     :steps:

+         1. Get attribute info for 'uid'

+         2. Check the attribute is found in the expected objectclasses (must, may)

+         3. Check that the 'account' objectclass is found

+     :expectedresults:

+         1. Success

+         2. Success

+         3. Success

+     """

+     must_expect = ['uidObject', 'account', 'posixAccount', 'shadowAccount', 'ipaUser',

+                    'sambaSamAccount']

      may_expect = ['cosDefinition', 'inetOrgPerson', 'inetUser',

-                   'mailRecipient']

-     attrtype, must, may = topology.instance.schema.query_attributetype('uid')

+                   'mailRecipient', 'nsOrgPerson', 'ipaUserOverride']

+     attrtype, must, may = topo.standalone.schema.query_attributetype('uid')

      assert attrtype.names == ('uid', 'userid')

      for oc in must:

          assert oc.names[0] in must_expect

      for oc in may:

          assert oc.names[0] in may_expect

-     assert topology.instance.schema.query_objectclass('account').names == \

+     assert topo.standalone.schema.query_objectclass('account').names == \

          ('account', )

  

- def test_schema_reload(topology):

-     """Test that the appropriate task entry is created when reloading schema."""

-     schema = Schema(topology.instance)

+ 

+ def test_schema_reload(topo):

+     """Run a schema reload task

+ 

+     :id: 995acc60-243b-45b0-9c1c-12bbe6a2882e

+     :setup: Standalone Instance

+     :steps:

+         1. Add schema reload task

+         2. Schema reload task succeeds

+     :expectedresults:

+         1. Success

+         2. Success

+     """

+ 

+     schema = Schema(topo.standalone)

      task = schema.reload()

      assert task.exists()

      task.wait()

      assert task.get_exit_code() == 0

  

  

+ def test_x_origin(topo):

+     """ Test that the various forms of X-ORIGIN are handled correctly

+ 

+     :id: 995acc60-243b-45b0-9c1c-12bbe6a2882d

+     :setup: Standalone Instance

+     :steps:

+         1. Create schema file with specific/unique formats for X-ORIGIN

+         2. Reload the schema file (schema reload task)

+         3. List all attributes without error

+         4. Confirm the expected results

+     :expectedresults:

+         1. Success

+         2. Success

+         3. Success

+         4. Success

+     """

+ 

+     # Create a custom schema file so we can tests specific formats

+     schema_file_name = topo.standalone.schemadir + '/98test.ldif'

+     schema_file = open(schema_file_name, "w")

+     testschema = ("dn: cn=schema\n" +

+                   "attributetypes: ( 8.9.10.11.12.13.16 NAME 'testattr' SYNTAX 1.3.6.1.4.1.1466.115.121.1.15 X-ORIGIN 'USER_DEFINED' )\n" +

+                   "attributetypes: ( 8.9.10.11.12.13.17 NAME 'testattrtwo' SYNTAX 1.3.6.1.4.1.1466.115.121.1.15 X-ORIGIN ( 'USER_DEFINED' 'should be not ignored!!' ) )\n")

+     schema_file.write(testschema)

+     schema_file.close()

+ 

+     # Reload the schema

+     myschema = Schema(topo.standalone)

+     task = myschema.reload()

+     assert task.exists()

+     task.wait()

+     assert task.get_exit_code() == 0

+ 

+     # Now search attrs and make sure there are no errors

+     myschema.get_attributetypes()

+     myschema.get_objectclasses()

+ 

+     # Check we have X-ORIGIN as expected

+     assert " 'USER_DEFINED' " in str(myschema.query_attributetype("testattr"))

+     assert " 'USER_DEFINED' " in str(myschema.query_attributetype("testattrtwo"))

+ 

+ 

  if __name__ == '__main__':

      # Run isolated

      # -s for DEBUG mode

-     import os

      CURRENT_FILE = os.path.realpath(__file__)

-     pytest.main("-s %s" % CURRENT_FILE)

+     pytest.main(["-s", CURRENT_FILE])

+ 

Bug Description:

Schema parsing assumed X-ORIGIN was always in this format "X-ORIGIN '", but it can also be in other formats like: "X-ORIGIN (". So when it did not contain the original format we got list index errors.

Fix Description:

Loosen the format to " X-ORIGIN " which all the formats.

Also improve from UI schema error messages

https://pagure.io/389-ds-base/issue/50004

Ack from me, is there a chance of a lib389 testcase with this too?

rebased onto 883d2986fcd416e81d9da42a24ee5ab5f9fbf907

5 years ago

rebased onto ac6b01010c04288e088aab31c5408a8bceb6aa50

5 years ago

I updated the CI schema test, and I also converted create_test.py to work in python3

Please review one more time...

Thanks! Looks really good!

One thing, the objectClass is still not fixed in the python-ldap yet. So we can't test it properly here.
But the change will work for both attributeTypes and objectClasses.
So you can test it for attributeType and then you can add the next check in the end of the last test case:

    assert "'USER_DEFINED'" in str(myschema.query_objectclass("testoc"))
    assert "( 'two' 'user defined' )" in str(myschema.query_objectclass("testoctwo"))

rebased onto bb2310cbf8210b51ab6e7413d1bf5350740fb86b

5 years ago

So this kind of turned into a can of worms...

python-ldap's Attribute does not handle X-ORIGIN correctly. It's actually a multivalued key, and that's how DS uses it:

X-ORIGIN ( 'ACME Attr Desc' 'user defined' )

But python-ldap parses this in a single value (the first item): ACME Attr Desc We lose user defined This will break the current UI schema design if a custom attribute already sets its own unique X-ORIGIN value and we lose "user defined".

We would have to basically manage X-ORIGIN for both objectclasses and attributes. This will require us to add some ugly code to get it working correctly - we'll need to construct the "string" representation of the attr/oc object: adding parenthesis and dollar signs, etc, etc.

So we should get this fixed in python-ldap first @spichugi this might impact your PR as well (https://github.com/python-ldap/python-ldap/pull/247). As X-ORIGIN is multivalued so your PR should reflect that.

Okay, I mentioned it in the PR there.

The code here looks good to me. Ack.

Okay, I mentioned it in the PR there.
The code here looks good to me. Ack.

Actually this code does not work correctly. I have a new fix, but its all a hack IMO. I'd rather fix python-ldap than hack up lib389 code to work around it, but maybe we have to use it temporarily. Hmm okay well I might have a new rebase to review soon...

rebased onto 1e9c7a595ad1f34298cad0cb7697a72e39598f02

5 years ago

Well I got it working to match how python-ldap works today, but it's only accurate when you return the json object. A non-json request will not return the objectclass's x-origin. To do that we would have to do a huge rewrite in the schema code to work around the python-ldap bugs. I'd rather leave our code as is, and fix python-ldap for the correct/clean solution.

Please review one last time...

Okay, looks good to me. Ack.

P.S. actually, I didn't see that X-ORIGIN with more than one values is used anywhere in our server... But yeah, if we allow it in the source code we should process it correctly. Thank you!

rebased onto ab321cf

5 years ago

Pull-Request has been merged by mreynolds

5 years ago

Okay, looks good to me. Ack.
P.S. actually, I didn't see that X-ORIGIN with more than one values is used anywhere in our server... But yeah, if we allow it in the source code we should process it correctly. Thank you!

If you add schema to user99.ldif with X-ORIGIN set to something, then "user defined" will get appended when you do an ldapsearch

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This pull request has been cloned to Github as issue and is available here:
- https://github.com/389ds/389-ds-base/issues/3064

If you want to continue to work on the PR, please navigate to the github issue,
download the patch from the attachments and file a new pull request.

Thank you for understanding. We apologize for all inconvenience.

Pull-Request has been closed by spichugi

3 years ago