#50245 Ticket 50137 - create should not check in non-stateful mode for exist
Closed 3 years ago by spichugi. Opened 5 years ago by firstyear.
firstyear/389-ds-base 50137-anon-create  into  master

@@ -766,11 +766,14 @@ 

  

          exists = False

  

-         try:

-             self._instance.search_ext_s(dn, ldap.SCOPE_BASE, self._object_filter, attrsonly=1, serverctrls=self._server_controls, clientctrls=self._client_controls, escapehatch='i am sure')

-             exists = True

-         except ldap.NO_SUCH_OBJECT:

-             pass

+         if ensure:

+             # If we are running in stateful ensure mode, we need to check if the object exists, and

+             # we can see the state that it is in.

+             try:

+                 self._instance.search_ext_s(dn, ldap.SCOPE_BASE, self._object_filter, attrsonly=1, serverctrls=self._server_controls, clientctrls=self._client_controls, escapehatch='i am sure')

+                 exists = True

+             except ldap.NO_SUCH_OBJECT:

+                 pass

  

          if exists and ensure:

              # update properties
@@ -781,10 +784,11 @@ 

              for k, v in list(valid_props.items()):

                  mods.append((ldap.MOD_REPLACE, k, v))

              self._instance.modify_ext_s(self._dn, mods, serverctrls=self._server_controls, clientctrls=self._client_controls, escapehatch='i am sure')

-         elif exists and not ensure:

-             # raise "already exists."

-             raise ldap.ALREADY_EXISTS("Entry %s already exists" % dn)

-         if not exists:

+         elif not exists:

+             # This case is reached in two cases. One is we are in ensure mode, and we KNOW the entry

+             # doesn't exist.

+             # The alternate, is that we are in a non-stateful create, so we "just create" and see

+             # what happens. I believe the technical term is "yolo create".

              self._log.debug('Creating %s' % dn)

              e = Entry(dn)

              e.update({'objectclass': ensure_list_bytes(self._create_objectclasses)})
@@ -792,8 +796,15 @@ 

              # We rely on exceptions here to indicate failure to the parent.

              self._log.debug('Creating entry %s : %s' % (dn, e))

              self._instance.add_ext_s(e, serverctrls=self._server_controls, clientctrls=self._client_controls, escapehatch='i am sure')

-             # If it worked, we need to fix our instance dn

+             # If it worked, we need to fix our instance dn for the object's self reference. Because

+             # we may not have a self reference yet (just created), it may have changed (someone

+             # set dn, but validate altered it).

              self._dn = dn

+         else:

+             # This case can't be reached now that we only check existance on ensure.

+             # However, it's good to keep it for "complete" behaviour, exhausting all states.

+             # We could highlight bugs ;)

+             raise AssertionError("Impossible State Reached in _create")

          return self

  

      def create(self, rdn=None, properties=None, basedn=None):

Bug Description: In def create, we should do a existance check for an
entry before creating. However, depending on access control this may not
work as intended because you can create without sight of the target, and
then this may cause misleading exceptions preventing the create.

Fix Description: In stateless mode, don't check the existance of the
entry before create.

In stateful ensure mode, continue to check for the existance.

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

Author: William Brown william@blackhats.net.au

Review by: ???

Should default to None as we really don't know (and will never know if it doesn't). False brings a false impression of "we know it does not".

This may happen even if we have no access, if I am not mistaken, no only if it is not there (which is why we should default to exists = None). And we should put here exists = None for clarity.

Is there really need for a comment here? If it is not clear what we're doing on the next line (and we're doing this at couple of places around) then we should make a method, like self._change_identity(new_dn=...), that clearly says it all.

I'd probably go with AssertionError in this case as it is more accurate than a general exception.

Do we also really need the two four-line comment blocks? I believe the conditional structure is clear enough to get the idea even without them. They may eventually become misleading when the code around changes, especially because they refer to other parts of the function (even the function itself) which they are not close enough to.

Anyways, functionally this looks good to me (except for the case of insufficient privileges I mentioned above - we'd have to handle it more complexly, but so far so good).

default to None:

No, because now we have a TERNARY state. None, False, or True. Then we set True/False if Exist, and None is if not exist. Now we have more states and possibilities of failure if someone changes this later.

The behavior is correct - if false, we assume it's not there and try to create which is the behaviour that is requested.

This may happen if we have no access

No, this only happens if someone royally messes up and manages to change the function in such a way that allows ensure mode == False, and exists True. This means that the if condition around the test of existance has broken, and has led to an invalid state.

If we took your "= None" procedure, we still need to test for this state anyway, which is why I won't add exists= None because it's a state that doesn't matter and doesn't add any value (only complexity).

Do we need a comment.

Yes. We absolutely need comments. Code is hard to read and follow, and there is extra context in the comment that the code doesn't allude to, it makes it easier to communicate. It discusses reasons that the code doesn't highlight. I will never comment my code "less", because william of the future is an idiot and needs the help.

AssertionError

Yeah, that probably reasonable to change.

rebased onto 559e7ddb1837e51eceaee54b1c5ecd107cb00425

5 years ago

rebased onto 12dcdbb4aab7d3cb086dd37d9fa1747b6535d9dd

5 years ago

rebased onto 28fe160

5 years ago

Pull-Request has been merged by firstyear

5 years ago

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/3304

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
Metadata