Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow renaming of sudo and HBAC rules #617

Closed
wants to merge 3 commits into from

Conversation

stlaz
Copy link
Contributor

@stlaz stlaz commented Mar 17, 2017

This simple hack adds a rename option to client side sudorule-mod
command.

https://pagure.io/freeipa/issue/2466

@abbra
Copy link
Contributor

abbra commented Mar 17, 2017

I don't like it is done on the client side. This will not work for Web UI, for example.
Additionally, no validation of cn={newname} is here to be a single value RDN. If we add this as --setattr, we probably want to return meaningful error, not a general --setattr error.

@stlaz
Copy link
Contributor Author

stlaz commented Mar 22, 2017

Thank you Alexander for your insight. Since this was a hack, I did not want to do it server-wise. I chose a different approach to the problem and reworked the original idea so the rename option is now worked with on server.
With this approach, we are able to white-list objects which we think may be allowed renaming even though their primary keys are not in their RDN.

Just for the record, the names of sudo rules are still not checked for CN compatibility since their primary key is not part of their DN, but that's how things have been since for ever, I am afraid (you can try ipa sudorule-add bad,cn=rule).

@stlaz stlaz changed the title Allow renaming of sudo rules Allow renaming of sudo and HBAC rules Mar 22, 2017
@@ -550,6 +550,7 @@ class LDAPObject(Object):
uuid_attribute = ''
attribute_members = {}
rdn_is_primary_key = False # Do we need RDN change to do a rename?
allow_rename = False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why introduce a new class attribute when rdn_is_primary_key does exactly the same?

Copy link
Contributor Author

@stlaz stlaz Mar 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we want to rename objects whose primary key is not RDN. But you're right, perhaps the attribute is just ill-named.

edit: nope, we are not changing DN in these objects.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the attribute is entirely ill, as it does 2 different things: it allows renaming and forces DN update on rename.

I think we should finally get rid of it and use

  • the allow_rename attribute to allow renaming and
  • decide whether a DN update is required or not based on the current DN.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the above should be done in a separate commit and linked to https://pagure.io/freeipa/issue/2466.

@abbra
Copy link
Contributor

abbra commented Mar 22, 2017

I like the idea but please address @HonzaCholasta comments.

@stlaz
Copy link
Contributor Author

stlaz commented Mar 23, 2017

For the record, and I might be wrong, I did a bit of researching, the rdn_is_primary_key is actually misused in some cases, as RDN is the primary key for e.g. pwpolicy and idrange but these have this attribute set to False.
I believe in the above cases, rdn_is_primary_key might have been used this way just so that those objects do not show the rename (they are not allowed to change the primary key anyway). I thought we won't need allow_rename at all in the end but for these cases we will probably need to keep it.

@stlaz
Copy link
Contributor Author

stlaz commented Mar 23, 2017

The latest patch removes the rdn_is_private_key attribute, replaces it with allow_rename which actually says correctly what's happening. Also, the decision whether primary key is in RDN is decided on checking whether the primary key is in RDN rather than on anything else.
Also added a comment explaining that the modrdn operation is performed also when setattr is doing changes to the primary key + RDN because it was far from obvious in the code.

@MartinBasti
Copy link
Contributor

I'd like to see this in 3 different patches: needed refactoring, sudo, hbac

@MartinBasti
Copy link
Contributor

I like the allow_rename attribute, as it really explains what is happaning, Also I like the reworked check if primary key is in DN because original self.obj.primary_key.name in entry_attrs may return false positive results.

I'm afraid about one thing. This will basically break custom user plugins if they used rdn_is_private_key. Shall we do some backward compatibility magic?

@abbra
Copy link
Contributor

abbra commented Mar 24, 2017

I haven't seen any custom plugin that used rdn_is_private_key. We can document the change in release notes.

@MartinBasti
Copy link
Contributor

Please provide tests, LGTM otherwise

@stlaz
Copy link
Contributor Author

stlaz commented Mar 24, 2017

Added the tests but did not test them so we may want to see what Travis has to say about that.

@MartinBasti
Copy link
Contributor

************* Module ipatests.test_xmlrpc.test_sudorule_plugin
ipatests/test_xmlrpc/test_sudorule_plugin.py:786: [E0001(syntax-error), ] unindent does not match any outer indentation level)

And please split it into multiple commits as I requested

The rename operation on *_mod commands was only allowed when
the primary key of an entry was also its RDN. With these changes,
it should be possible to rename the rest of the entries as well.

An attribute to the base LDAPObject was added to whitelist the
objects we want to allow to be renamed. It replaced an old
attribute rdn_is_primary_key which was used for the very same
purpose but the name was confusing because it was not set
correctly for certain objects.

https://pagure.io/freeipa/issue/2466
https://pagure.io/freeipa/issue/6784
The recent changes allow HBAC rule objects to be renamed.

https://pagure.io/freeipa/issue/6784
The recent changes allow the sudorule objects to be renamed.

https://pagure.io/freeipa/issue/2466
@stlaz
Copy link
Contributor Author

stlaz commented Mar 27, 2017

*sigh* there was a rogue space.

Split into three separate commits.

@MartinBasti MartinBasti self-assigned this Mar 27, 2017
@MartinBasti
Copy link
Contributor

Please update release notes (changelog)

@MartinBasti MartinBasti added the ack Pull Request approved, can be merged label Mar 27, 2017
@stlaz
Copy link
Contributor Author

stlaz commented Mar 27, 2017

Changelogs were updated.

@pvomacka
Copy link

ipa-4-5:

  • 28db6cd Reworked the renaming mechanism

  • 85f2a19 Allow renaming of the HBAC rule objects

  • 7d3229b Allow renaming of the sudorule objects
    master:

  • 8e4408e Reworked the renaming mechanism

  • 55424c8 Allow renaming of the HBAC rule objects

  • 8c14091 Allow renaming of the sudorule objects

@pvomacka pvomacka added the pushed Pull Request has already been pushed label Mar 27, 2017
@pvomacka pvomacka closed this Mar 27, 2017
@stlaz stlaz deleted the sudorule_rename branch September 11, 2017 10:47
stanislavlevin added a commit to stanislavlevin/freeipa-desktop-profile that referenced this pull request Mar 12, 2019
FreeIPA renaming operations were reworked in
freeipa/freeipa#617

The rdn_is_primary_key attribute was replaced with
new allow_rename one.

Fixes: abbra#11
Signed-off-by: Stanislav Levin <slev@altlinux.org>
abbra pushed a commit to abbra/freeipa-desktop-profile that referenced this pull request Apr 18, 2019
FreeIPA renaming operations were reworked in
freeipa/freeipa#617

The rdn_is_primary_key attribute was replaced with
new allow_rename one.

Fixes: #11
Signed-off-by: Stanislav Levin <slev@altlinux.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
5 participants