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

Fixing adding authenticator indicators to host #761

Closed
wants to merge 1 commit into from

Conversation

felipevolpone
Copy link
Member

@felipevolpone felipevolpone commented May 3, 2017

The check for krbprincipalaux in the entries is now made case-insensitively.

https://pagure.io/freeipa/issue/6911
https://bugzilla.redhat.com/show_bug.cgi?id=1441593#c2

@stlaz
Copy link
Contributor

stlaz commented May 4, 2017

************* Module ipaserver.plugins.host

ipaserver/plugins/host.py:887: [C0303(trailing-whitespace), ] Trailing whitespace)

+ wrong author in the commit

@@ -884,7 +884,8 @@ def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, **options):
msg = 'Principal name already set, it is unchangeable.'
raise errors.ACIError(info=msg)
obj_classes = entry_attrs_old['objectclass']
if 'krbprincipalaux' not in obj_classes:
if 'krbprincipalaux' not in set(item.lower() for item in
Copy link
Contributor

@stlaz stlaz May 4, 2017

Choose a reason for hiding this comment

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

I do not see any reason to construct a set from the result.

@pvoborni
Copy link
Member

pvoborni commented May 4, 2017

I'd fix it on all places in host-mod:

  885                  raise errors.ACIError(info=msg)
  886              obj_classes = entry_attrs_old['objectclass']
  887:             if 'krbprincipalaux' not in obj_classes:
  888                  obj_classes.append('krbprincipalaux')
  889                  entry_attrs['objectclass'] = obj_classes
  ...
  921                  _entry_attrs = ldap.get_entry(dn, ['objectclass'])
  922                  obj_classes = _entry_attrs['objectclass']
  923:             if 'ieee802device' not in obj_classes:
  924                  obj_classes.append('ieee802device')
  925                  entry_attrs['objectclass'] = obj_classes
  ...
  941                  _entry_attrs = ldap.get_entry(dn, ['objectclass'])
  942                  obj_classes = entry_attrs['objectclass'] = _entry_attrs['objectclass']
  943:             if 'ipasshhost' not in obj_classes:
  944                  obj_classes.append('ipasshhost')

so that the plugin would be consistent. Rest of framework can be fixed other time.

@@ -920,7 +920,7 @@ def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, **options):
else:
_entry_attrs = ldap.get_entry(dn, ['objectclass'])
obj_classes = _entry_attrs['objectclass']
if 'ieee802device' not in obj_classes:
if 'ieee802device' not in [item.lower() for item in obj_classes]:
Copy link
Contributor

Choose a reason for hiding this comment

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

My point was that this should be a generator expression.

Copy link
Member Author

Choose a reason for hiding this comment

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

By generator expression, you mean this?

(item.lower() for item in obj_classes)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

@felipevolpone
Copy link
Member Author

Done

@@ -949,14 +950,16 @@ def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, **options):
if 'objectclass' not in entry_attrs:
entry_attrs_old = ldap.get_entry(dn, ['objectclass'])
entry_attrs['objectclass'] = entry_attrs_old['objectclass']
if 'krbticketpolicyaux' not in entry_attrs['objectclass']:
if 'krbticketpolicyaux' not in (item.lower() for item in
obj_classes):
Copy link
Contributor

Choose a reason for hiding this comment

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

obj_classes are possibly undefined here, looks like a copy-paste error to me.

entry_attrs['objectclass'].append('krbticketpolicyaux')

if 'krbprincipalauthind' in entry_attrs:
if 'objectclass' not in entry_attrs:
entry_attrs_old = ldap.get_entry(dn, ['objectclass'])
entry_attrs['objectclass'] = entry_attrs_old['objectclass']
if 'krbprincipalaux' not in entry_attrs['objectclass']:
if 'krbprincipalaux' not in (item.lower() for item in
obj_classes):
Copy link
Contributor

Choose a reason for hiding this comment

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

obj_classes are possibly undefined here, looks like a copy-paste error to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

That exactly what happened. Thank you for pointing me that :)
It's fixed now. If you think it is, I can squash my commits into a single one.

@stlaz
Copy link
Contributor

stlaz commented May 10, 2017

Yes, that seems to have fixed that. Please do squash them now, I guess we can ACK this ;)

The check for krbprincipalaux in the entries is now made
case-insensitively.

https://pagure.io/freeipa/issue/6911
@felipevolpone
Copy link
Member Author

Cool :)) thanks!

@stlaz stlaz added the ack Pull Request approved, can be merged label May 12, 2017
@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label May 16, 2017
@MartinBasti
Copy link
Contributor

master:

  • d51af28 Fixing adding authenticator indicators to host

ipa-4-5:

  • 81ae5f4 Fixing adding authenticator indicators to host

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
4 participants