[Freeipa-devel] [freeipa PR#617][opened] Allow renaming of sudo rules

[stlaz]

URL: https://github.com/freeipa/freeipa/pull/617
Author: stlaz
Title: #617: Allow renaming of sudo rules
Action: opened

PR body:
"""
This simple hack adds a rename option to client side sudorule-mod
command.

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

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/617/head:pr617
git checkout pr617

[abbra]

URL: https://github.com/freeipa/freeipa/pull/617
Title: #617: Allow renaming of sudo rules

abbra commented:
"""
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.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/617#issuecomment-287358727

[stlaz]

URL: https://github.com/freeipa/freeipa/pull/617
Author: stlaz
Title: #617: Allow renaming of sudo rules
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/617/head:pr617
git checkout pr617

[stlaz]

URL: https://github.com/freeipa/freeipa/pull/617
Title: #617: Allow renaming of sudo rules

stlaz commented:
"""
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`).
"""

See the full comment at https://github.com/freeipa/freeipa/pull/617#issuecomment-288389417

[stlaz]

URL: https://github.com/freeipa/freeipa/pull/617
Author: stlaz
Title: #617: Allow renaming of sudo and HBAC rules
Action: edited

Changed field: title
Original value:
"""
Allow renaming of sudo rules
"""

[abbra]

URL: https://github.com/freeipa/freeipa/pull/617
Title: #617: Allow renaming of sudo and HBAC rules

abbra commented:
"""
I like the idea but please address @HonzaCholasta comments.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/617#issuecomment-288411495

[stlaz]

URL: https://github.com/freeipa/freeipa/pull/617
Title: #617: Allow renaming of sudo and HBAC rules

stlaz commented:
"""
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.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/617#issuecomment-288664689

[stlaz]

URL: https://github.com/freeipa/freeipa/pull/617
Author: stlaz
Title: #617: Allow renaming of sudo and HBAC rules
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/617/head:pr617
git checkout pr617

[stlaz]

URL: https://github.com/freeipa/freeipa/pull/617
Title: #617: Allow renaming of sudo and HBAC rules

stlaz commented:
"""
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.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/617#issuecomment-288705598

[MartinBasti]

URL: https://github.com/freeipa/freeipa/pull/617
Title: #617: Allow renaming of sudo and HBAC rules

MartinBasti commented:
"""
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?
"""

See the full comment at https://github.com/freeipa/freeipa/pull/617#issuecomment-289021080

[abbra]

URL: https://github.com/freeipa/freeipa/pull/617
Title: #617: Allow renaming of sudo and HBAC rules

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

See the full comment at https://github.com/freeipa/freeipa/pull/617#issuecomment-289022375

[MartinBasti]

URL: https://github.com/freeipa/freeipa/pull/617
Title: #617: Allow renaming of sudo and HBAC rules

MartinBasti commented:
"""
Please provide tests, LGTM otherwise
"""

See the full comment at https://github.com/freeipa/freeipa/pull/617#issuecomment-289027561

[stlaz]

URL: https://github.com/freeipa/freeipa/pull/617
Title: #617: Allow renaming of sudo and HBAC rules

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

See the full comment at https://github.com/freeipa/freeipa/pull/617#issuecomment-289038857

[stlaz]

URL: https://github.com/freeipa/freeipa/pull/617
Author: stlaz
Title: #617: Allow renaming of sudo and HBAC rules
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/617/head:pr617
git checkout pr617

[MartinBasti]

URL: https://github.com/freeipa/freeipa/pull/617
Title: #617: Allow renaming of sudo and HBAC rules

MartinBasti commented:
"""
```
************* 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
"""

See the full comment at https://github.com/freeipa/freeipa/pull/617#issuecomment-289053965

[stlaz]

URL: https://github.com/freeipa/freeipa/pull/617
Author: stlaz
Title: #617: Allow renaming of sudo and HBAC rules
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/617/head:pr617
git checkout pr617

[stlaz]

URL: https://github.com/freeipa/freeipa/pull/617
Title: #617: Allow renaming of sudo and HBAC rules

stlaz commented:
"""
*sigh* there was a rogue space.

Split into three separate commits.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/617#issuecomment-289366812

[stlaz]

URL: https://github.com/freeipa/freeipa/pull/617
Title: #617: Allow renaming of sudo and HBAC rules

stlaz commented:
"""
*sigh* there was a rogue space.

Split into three separate commits.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/617#issuecomment-289366812

[MartinBasti]

URL: https://github.com/freeipa/freeipa/pull/617
Title: #617: Allow renaming of sudo and HBAC rules

MartinBasti commented:
"""
Please update release notes (changelog)
"""

See the full comment at https://github.com/freeipa/freeipa/pull/617#issuecomment-289423071

[MartinBasti]

URL: https://github.com/freeipa/freeipa/pull/617
Title: #617: Allow renaming of sudo and HBAC rules

Label: +ack

[stlaz]

URL: https://github.com/freeipa/freeipa/pull/617
Title: #617: Allow renaming of sudo and HBAC rules

stlaz commented:
"""
Changelogs were updated.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/617#issuecomment-289440947

[pvomacka]

URL: https://github.com/freeipa/freeipa/pull/617
Author: stlaz
Title: #617: Allow renaming of sudo and HBAC rules
Action: closed

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/617/head:pr617
git checkout pr617

[pvomacka]

URL: https://github.com/freeipa/freeipa/pull/617
Title: #617: Allow renaming of sudo and HBAC rules

Label: +pushed

[pvomacka]

URL: https://github.com/freeipa/freeipa/pull/617
Title: #617: Allow renaming of sudo and HBAC rules

pvomacka commented:
"""
ipa-4-5:

* 28db6cd40100c6301121e3f82c074624fe53729c Reworked the renaming mechanism
* 85f2a19f88eef94ff080a42246658f572b5275f4 Allow renaming of the HBAC rule objects
* 7d3229bfb88f0fdc559245c8741563faba716106 Allow renaming of the sudorule objects
master:

* 8e4408e6784f929b4c3d861f0dd509335238e951 Reworked the renaming mechanism
* 55424c8677ba7de464c820afd31260aa4a7678d0 Allow renaming of the HBAC rule objects
* 8c1409155e9a9a978d3d763045a84d1eac585dfd Allow renaming of the sudorule objects
"""

See the full comment at https://github.com/freeipa/freeipa/pull/617#issuecomment-289518952