-
Notifications
You must be signed in to change notification settings - Fork 152
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
UX and api design for common functionality between "account" and "self" #668
Comments
I don't think we need the /whoami, because the user's account token (UAT) is transparent, and already contains the uuid, so we can just introspect the session locally and get the uuid from there :) Otherwise, I think is the right way to do it. The main thing to be careful of is that I want to split "person" vs "account". IE a person can self-write, but an account can not. Consider a service account on a database server, we don't want it to be able to change mail etc as that could be a security compromise during an exploit scenario. But a person, we do want them to have extra ability to modify themself since they are a person!
This already exists actually, it's internal to the server and isn't exposed. So for now I'd probably want to keep it that way because it's pretty contextual and the filter syntax/lang may end up being an internal-only element.
Yes, the layout has evolved :) I'm certainly open to better and more natural suggestions!
Consistency is very important, so I completely agree here, let's keep "self" as a subset of "account", and that subset should be laid out identically. I also agree that we should make it better, so I'd love to hear your ideas :)
Yes, for a CLI that's not possible. That's something more realistic in the webui though, and something I need to really consider and think about more for that kind of dynamic highlighting.
Existing apis - I think it was a mistake to include a self api in the past. We can access the uuid via the UAT, so we can use that.
It should be the same structure, consistency is a very important concept in human interaction psychology, and I'd like to provide that natural and consistent behaviour.
There's a lot of lessons I have learnt since I initially created some of this cli, and a lot of ways we can improve this. So I'm really open to suggestions. I really like the idea here of the generic, and how you use it, since it means we can really control what is and isn't shared nicely and it prevents a lot of duplication. I think this is great and I'd be happy to go ahead with it. As mentioned, check the UAT since it contains the uuid already, you don't need to make a call out to the server #60 Thanks so much for your excellent ideas and thoughts! |
Cheering along the current ideas - one API endpoint for a given action regardless of target UUID makes a lot more sense than having to support multiple routes 😄 I'd argue against the |
From a CLI -> credential perspective, maybe a slightly more hierarchical tree like this?
|
@yaleman Well, given how credential is being reworked atm it's more likely to be:
But I think we can focus on that a bit in the current credentual update session pr I'll be posting soon. |
Yep, agreed :) I liked the suggested "traits" what contain a "get_target" method, it's nice and simple and I think it would let us already remove the self routes. |
I'd probably move "trusted devices" up a layer in the tree, since while they are technically credentials if you squint, I'm not sure that's as ergonomic. And the children of "primary" up under credential - having it nest so deep is a bit... challenging from a discovery perspective. |
You're completely right, that's a good point :) |
I like and use this software and want to pay it forward. @Firstyear pointed me in the direction of this ticket so I am going to take a crack at it. I am writing down my thoughts here for the sake of discussion but would prefer that it gets done to @Firstyear preferences. These are some initial thoughts from reading discussion. I still don't know the CLI or code that well. Points of discussionUpdate CLI StructureSooooooooo, what if we move self to its own binary and mirror the commands there? This way self-help user don't have to deal with the contextual overload of seeing all the admin commands. Imagine opening up a new cli and see 3 commands vs 17. In the gui, do you place the admin panel with the user settings or make them separate pages? It would make it far less scary for self-help end-users. Intuitively, this will also give us more design room with global stuff in the admin CLI if we do not have to worry about the self serve users. We could do awesome things, I think? [Generally, I feel that admin user will benefit for increased flexibility at the cost of complexity. Like making the admin CLI more tuned for batch operations (acting admin account, operation, <accounts/groups/etc>[]) so bash loops are unneeded for batch operations.] On the other hand, now there are two binaries... but we we could keep the self namespace in the admin cli as well. So each user only needs one. Making the self help cli a strict subset of the admin cli. I suspect we should still be able increase code reuse overall as we do this. Update API StructureRemove the self API in favor of hitting whoiam and the accounts API. Parsing the ID out of the local context not worth the code (although if server and client can share the same machinery here...). I think this is clear cut from the discussion unless I misread. Migrate StructOps to ClapI see that StructOps is getting sunsetted in favor of the Clap builder API. The release happened this past December. It will be in maintenance mode so no reason to switch unless needed. https://github.com/clap-rs/clap/blob/master/CHANGELOG.md#300---2021-12-31 Hiding Interface DynamicallyThe issue mentions hiding the interface dynamically. Hiding and other options of modifying visibility built into the Clap package functionality (see Migrate StructOps to Clap). However, I have been burned when GUIs hide things from me rather then greying them out with an explanation. I think it is bad UX. It makes things confusing. In terms of CLI "greying" we could however override the help output. Particularly, if it has configurable colored output we could literally grey it out with a tailored explanation. :) We would probably need to wait for this and would probably need us to switch to the builder API but I would be interested it pushing the derive boundary to its fullest: Overall, I think the work required would out weigh the benefit. Moving self-help users to there own CLI also mitigates the need (See Update CLI Structure)). Person/Accounts Mental MapThere are some holes here for me. So I am writing down my mental map. self = i.person person = (human, account(with self-write priv)) self command: self acting on self account command: self acting on accounts What makes sense to is that Person is very distinct from account. And each account must tied to a set of people. Additional authorization options for accounts might be that approval is required from n people in the set or that an additional password is required in addition to being authorized as that person in a set (this is required so things like the default admin accounts can work within this context and not be special exceptions). For example: this would make easy for one person to have many accounts on a social media site for anonymity. But for the social media site to set like limits per person. |
I'd assume that non-technical folks will likely be using the Web-UI for provisioning, and CLI-lovers will be able to parse the incremental complexity of "self" vs other tasks? It's an interesting idea though, as the project continues and UI gets more complex. From a development/maintenance perspective I'd avoid adding another CLI tool to maintain, too. UI-greying/hiding is messy and adds complexity, I'd agree it isn't needed, even if it was possible. As StructOpt's going into maintenance mode, the move to pure-clap and builders should definitely be considered at some point. My understanding is that all people are accounts, but all accounts aren't people, it's a flag to set 'person' status. I'm not sure where the multiple-person-auth fits in - they're like "service" accounts for automation or break-glass processes etc. |
That's possible, but see below/above - the cli is intended for power users, so I think it's more a "shortcut" in this case for an admin to just tweak their own account. So we likely don't need this kind of split (yet?) - I'm very open to be wrong!
Well, we could do the trait-style split inside of the current kanidm binary, and then if we decided we want the kanidm-self binary, it would be a pretty easy and straight forward jump to use that? So I think that would be the first focus, and then most of the work would already be done to get a self binary available if we decided to include that :)
Yep, that's clear cut :)
Yep, this has been on my radar for a while, but I've been focused on "user-facing" bits rather than updating that library, so it would be awesome if you could do that!
Well, the main thing here, as @yaleman pointed out is the CLI is mainly for administrators and "power users". The Web-UI is finally getting fleshed out so for most people with accounts, they'll have the more constrained and focused web page they can use. So I think the need for "dynamic hiding" isn't really needed :)
This is prettttyyyyy close to correct. Self is actually Otherwise, you seem to be correct, the self command is "acting on the account I am currently authenticated as". So if I were to do:
I'd want to see "myself". The same way that So I think you get it :) |
My search of "update self" lead me here. After a whole year, I want to ask:
|
No api change is needed, it's just a change in the cli to automatically populate the target uuid with the uuid from the current session.
Yes, they already can by using the current kanidm person commands with their own username as the target. There is an access control group you can add persons (or groups) to to enable the "self write email" privilege. |
Background
This issue is to try and figure out the right way to model commands that modify accounts, and may also be self-service.
For a motivating example, let's look at the following existing pair of commands:
Let's also consider a hypothetical pair of commands:
There are several other attributes which may be self-service, and all of them have a similar potential split.
I think this shows that the
self
andaccount
namespace should have some overlap, and the rest of this issue is to discuss options for that.Current technical solution / server api
The current solution used for
set_password
is effectively to duplicate the code:account credential set_password
kanidm/kanidm_tools/src/cli/account.rs
Lines 67 to 78 in 4862b91
self set_password
kanidm/kanidm_tools/src/cli/lib.rs
Lines 61 to 71 in 4862b91
Notably, this includes two backend APIs on the server too:
(self):
/_credential/primary/set_password
(account credential):
/v1/account/:id/_credential/primary
Alternative API
One alternative to the pair of server-side APIs would be to instead just have the account version of it.
The
self set_password
command, instead of calling/_credential/primary/set_password
, could instead:/whoami
and get their own uuid/v1/account/<uuid>/_credential/primary
to set the credentialAlternatively, we could have some reserved uuid or other identifier that we fill in to
:id
which indicatesself
, removing the whoami call. The server obviously already can figure out the authenticated account.Command UX
For the actual UX of the command, we have additional choices.
I think the biggest one is "do we mirror the 'account' subcommand, or do we make a bespoke self UX?"
I think there's a fair argument for making the account and self commands have a similar subcommand structure (with
self
being a subset of account). Having similar CLI UX will especially be helpful for an administrator who's used to using the 'account' commands, but wants to recommend a user run a correspondingself
command.If we followed the mirroring approach, I think it would currently look like so:
I think that this command structure is somewhat inconsistent (i.e. the credential grouping feels a little haphazard, the
person set
command has different ux compared to everything else, etc)....But I don't think that's an argument not to mirror the structure, but is rather an argument to improve both the account and person structure to be laid out better, but still identically.
Hiding unused portions
One thing which would be nice for cleaning up the UX, but may not be viable, is excluding portions from 'self' that don't make sense for any logged in account.
For example, if a given account doesn't have the
posixaccount
class, none of theposix
commands will do anything. This is similar for person.Unfortunately, this seems like it's probably not technically feasible, especially when shipping static auto-completions etc.
Initial decision points
The above raised two questions which I think are worth considering:
a. If "use account APIs", should they internally call a
whoami
/get_account_id
api to fill in their account uuid (and possibly cache the uuid locally)? Or should they fill in some sigil/special uuid that indicates "currently authed user uuid here"?My current instincts are "1. No dedicated APIs, use the account ones, preferably with a 'placeholder' account id" and "2. Same structure".
A final aside, this issue is too high level to actually talk about how we model 2 in structopt, but I did poke at that a bit.
It's possible to make an optional final arg (the account ID) in clap by making a generic structopt struct.
See what I mean here (full diff here, I'm quite aware the code's not well structured, I was just experimenting to see that it is possible to do that at all)
The text was updated successfully, but these errors were encountered: