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

add whoami command #535

Closed
wants to merge 1 commit into from
Closed

add whoami command #535

wants to merge 1 commit into from

Conversation

abbra
Copy link
Contributor

@abbra abbra commented Mar 2, 2017

ipa whoami command allows to query details about currently
authenticated identity. The command returns following information:

  • object class name
  • function to call to get actual details about the object
  • arguments to pass to the function
  • options to pass to the function

There are five types of objects that could bind to IPA using their
credentials. ipa whoami call expects one of the following:

  • users
  • staged users
  • hosts
  • Kerberos services
  • ID user override from the default trust view

The latter category of objects is automatically mapped by SASL GSSAPI
mapping rule in 389-ds for users from trusted Active Directory forests.

The command is expected to be used by Web UI to define proper view for
the authenticated identity.

Below is an example of how communication looks like for an Active
Directory user which has ID override in 'Default Trust View'.

$ ipa -vv whoami
ipa: INFO: trying https://ipa.example.com/ipa/session/json
ipa: INFO: Forwarding 'whoami/1' to json server 'https://ipa.example.com/ipa/session/json'
ipa: INFO: Request: {
    "id": 0,
    "method": "whoami/1",
    "params": [
        [],
        {
            "version": "2.220"
        }
    ]
}
ipa: INFO: Response: {
    "error": null,
    "id": 0,
    "principal": "Administrator@AD.DOMAIN",
    "result": {
        "arguments": [
            "default trust view",
            "administrator@ad.domain"
        ],
        "details": "idoverrideuser_show/1",
        "object": "idoverrideuser",
        "options": []
    },
    "version": "<IPA VERSION>"
}

Fixes https://pagure.io/freeipa/issue/6643

@abbra
Copy link
Contributor Author

abbra commented Mar 2, 2017

@stlaz stlaz self-assigned this Mar 7, 2017
@stlaz
Copy link
Contributor

stlaz commented Mar 8, 2017

I believe that in CLI ipa whoami should actually output the output of the command it found with the arguments and options it found since in WebUI this is eventually done as well.
I can go ahead and try to implement it if we can agree on such behavior.

@abbra
Copy link
Contributor Author

abbra commented Mar 8, 2017

Uhm, no, I don't want that. It makes the command behaving differently depending on a context and that would be broken. For client-side plugin that would also be an abuse of interface, I'd say.

@stlaz
Copy link
Contributor

stlaz commented Mar 8, 2017

Ok. It just doesn't seem right to have a command which shows something that's not immediately useful to the user. I am not sure whether we should have it enabled for CLI.

@abbra
Copy link
Contributor Author

abbra commented Mar 8, 2017

We can disable it for CLI, that's not a problem.

__doc__ = _("""
Return a description of currently authenticated identity

Who am I command returns information on who to get
Copy link
Contributor

Choose a reason for hiding this comment

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

Major nitpick: I believe you meant "...returns information on how to get..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.


output_params = (
Str('object', label=_('Object class name')),
Str('details', label= _('Function to get details')),
Copy link
Contributor

Choose a reason for hiding this comment

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

Please call the param "command" instead of "details", it's much more descriptive. "details" does not mean anything.

object that handles the container where this DN belongs to. Then report
details about this object.
"""
exceptions = dict(
Copy link
Contributor

Choose a reason for hiding this comment

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

For the future: dict() should generally be avoided for CPython and {} should be used instead to init dictionaries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

o_func = None
o_args = []
o_opts = []
for o in api.Object():
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you don't need to init api.Object() here but that does not matter much so feel free to leave it there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no need to init it because api is fully initialized already at the point when any plugin command is run. api.Object() returns an iterator, it is not a class constructor.

# We found exact container this DN belongs to
o_name = unicode(o.name)
o_args = [unicode(entry.single_value.get(o.primary_key.name))]
o_opts = []
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no reason to have options in the output since you never add anything there, please, remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On further thinking I removed it.

@stlaz
Copy link
Contributor

stlaz commented Mar 9, 2017

Please, disable the command for CLI since by itself it does not bring any valuable information to the user, in my opinion it can only confuse them. It'll be easier to turn it on if there's a demand for it rather than turning it off if it confuses some people.
Also, please rename the details part of output to command as that's what it really is, details is confusing.
Please, also remove the options from the output since it's not currently used. I hope it's safe to assume that new output parameters can be added at any time.
You don't need to bother much with the other comments I left, there're more important things to do now than fiddling with docs and minor performance issues.

@abbra abbra force-pushed the whoami branch 2 times, most recently from 4a8a7f8 to 8813bd2 Compare March 9, 2017 10:08
@abbra
Copy link
Contributor Author

abbra commented Mar 9, 2017

Updated.

@stlaz
Copy link
Contributor

stlaz commented Mar 9, 2017

@abbra Thank you for the changes, the patch seems fine now. I tested the user/service/host scenarios and it worked fine. I couldn't test idviews since trusts are broken now but I assume it should work fine as well.
If you could only apply the following patch https://transfer.sh/IA7Ic/0001-improve-one-more-dict.patch which improves the last dict() behavior then I'll bless this patch :)
We may want to add some tests later so I will propose to leave the ticket open.

Whoami command allows to query details about currently
authenticated identity. The command returns following information:

  * object class name
  * function to call to get actual details about the object
  * arguments to pass to the function

There are five types of objects that could bind to IPA using their
credentials. `ipa whoami` call expects one of the following:

  * users
  * staged users
  * hosts
  * Kerberos services
  * ID user override from the default trust view

The latter category of objects is automatically mapped by SASL GSSAPI
mapping rule in 389-ds for users from trusted Active Directory forests.

The command is expected to be used by Web UI to define proper view for
the authenticated identity. It is not visible in the command line
interface is `ipa` command.

Below is an example of how communication looks like for a host
principal:

   # kinit -k
   # ipa console
   (Custom IPA interactive Python console)
   >>> api.Command.whoami()
   {u'command': u'host_show/1', u'object': u'host', u'arguments': (u'ipa.example.com',)}
   >>>

Fixes https://pagure.io/freeipa/issue/6643
@abbra
Copy link
Contributor Author

abbra commented Mar 9, 2017

Done. I've also updated the design page to reflect the changes.

@stlaz
Copy link
Contributor

stlaz commented Mar 9, 2017

Thank you, ACK. Please don't close the ticket, we still need tests.

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

master:

@MartinBasti MartinBasti closed this Mar 9, 2017
@MartinBasti
Copy link
Contributor

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