[#1023] Delete every objectClass value a modification asks for - #1024
Open
vharseko wants to merge 1 commit into
Open
[#1023] Delete every objectClass value a modification asks for#1024vharseko wants to merge 1 commit into
vharseko wants to merge 1 commit into
Conversation
…tion asks for Entry.removeObjectClassAttribute() returned out of the whole method on the first value it removed, so a "delete: objectClass" carrying several values dropped only the first one and never looked at the rest: the remaining classes stayed on the entry, none of the values which the entry does not have was reported as missing, and the client got SUCCESS. Break out of the search through the object classes of the entry instead and carry on with the next value of the modification. The client-visible change is confined to "delete: objectClass" in a modify: the other callers of Entry.removeAttribute(Attribute, Collection) - the old RDN handling of modifyDN - ignore the return value and only read the missing values. Fixes OpenIdentityPlatform#1023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1023
The defect
Entry.removeObjectClassAttribute()returns out of the whole method on the first value itremoves, instead of breaking out of the search through the object classes of the entry and
carrying on with the next value of the modification:
Every
delete: objectClasstakes this path (LocalBackendModifyOperation.processDeleteModification()->
Entry.removeAttribute(Attribute, Collection)->removeObjectClassAttribute()), so amodification which deletes several object classes drops only the first of them, keeps the rest on
the entry, and answers
SUCCESS.The early return hides missing values as well:
missingValues- which the core turns intoNO_SUCH_ATTRIBUTE- is only filled for the values examined before the first removal, so deletinga class the entry has together with one it does not have succeeded silently.
Single-valued deletes and deletes of the whole objectClass attribute are unaffected, which is why
this has gone unnoticed. The same code is in WrenSecurity/wrends, so the defect is inherited from
upstream rather than introduced here.
The fix
Break out of the inner loop and carry on with the next value, the way the non-objectClass branch of
removeAttribute()walks every value of the modification.The return value keeps the meaning this branch has always given it (
allSuccessful), so theclient-visible change is confined to
delete: objectClass: every value is now deleted, and a valuethe entry does not have raises
NO_SUCH_ATTRIBUTEinstead of succeeding silently - unless thepermissive modify control is used, which drops the missing values as before. The other callers of
Entry.removeAttribute(Attribute, Collection)- the old RDN handling ofLocalBackendModifyDNOperationTests
Two tests in
TestEntry, both failing before the fix:testRemoveSeveralObjectClassValues- deletingorganizationalPersonandinetOrgPersonin onecall left
inetOrgPersonon the entry.testRemoveObjectClassValuesOneOfWhichIsMissing- deletinginetOrgPersontogether withdomain,which the entry does not have, claimed success and reported no missing value.
Local run of
TestEntry,ModifyOperationTestCase,EntrySchemaCheckingTestCase: 981 tests, 0 failures.Note on merge order
PR #1022 touches the same method (it resets the cached objectClass attribute next to
objectClasses.remove(oc)), so whichever of the two lands second conflicts in this hunk. Theresolution is to keep both changes: the reset of the cached attribute, and
matchFound/breakin place of the early return.