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

UX and api design for common functionality between "account" and "self" #668

Open
euank opened this issue Apr 4, 2022 · 12 comments · May be fixed by #797
Open

UX and api design for common functionality between "account" and "self" #668

euank opened this issue Apr 4, 2022 · 12 comments · May be fixed by #797
Labels
api REST API things

Comments

@euank
Copy link
Contributor

euank commented Apr 4, 2022

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:

$ kanidm account credential set_password <account>
$ kanidm self set_password

Let's also consider a hypothetical pair of commands:

$ kanidm account person set --legalname "Name" <account>
$ kanidm self <set legal name subcommand; design TBD

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 and account 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

let client = acsopt.copt.to_client();
let password = match password_prompt(
format!("Enter new password for {}: ", acsopt.aopts.account_id).as_str(),
) {
Some(v) => v,
None => {
println!("Passwords do not match");
return;
}
};
if let Err(e) = client.idm_account_primary_credential_set_password(

self set_password

let client = copt.to_client();
let password = match rpassword::prompt_password_stderr("Enter new password: ") {
Ok(p) => p,
Err(e) => {
error!("Error -> {:?}", e);
return;
}
};
if let Err(e) = client.idm_account_set_password(password) {

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:

  1. Call /whoami and get their own uuid
  2. Call /v1/account/<uuid>/_credential/primary to set the credential

Alternatively, we could have some reserved uuid or other identifier that we fill in to :id which indicates self, 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 corresponding self command.

If we followed the mirroring approach, I think it would currently look like so:

$ kanidm self
             ├ credential
             │           ├ generate_backup_codes
             │           ├ register_totp
             │           ├ register_webauthn
             │           ├ remove_backup_codes
             │           ├ remove_totp
             │           ├ reset_credential
             │           ├ remove_webauthn
             │           ├ set_password
             │           └ status
             │
             ├ person
             │       └ set --legalname --mail
             │
             ├ posix
             │      ├ set_password
             │      └ show
             │
             ├ radius
             │       ├ generate_secret
             │       ├ delete_secret
             │       └ show_secret
             │
             └ ssh
                  ├ add_publickey
                  ├ delete_publickey
                  └ list_publickeys

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 the posix 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:

  1. Should the server API include dedicated APIs for "self" commands, or should those commands typically lean on existing account APIs
    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"?
  2. Should the command structure for account vs person differ? Why/why not?

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)

@Firstyear
Copy link
Member

The self set_password command, instead of calling /_credential/primary/set_password, could instead:

  1. Call /whoami and get their own uuid
  2. Call /v1/account/<uuid>/_credential/primary to set the credential

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!

Alternatively, we could have some reserved uuid or other identifier that we fill in to :id which indicates self, removing the whoami call. The server obviously already can figure out the authenticated account.

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.

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 corresponding self command.

If we followed the mirroring approach, I think it would currently look like so:

$ kanidm self
             ├ credential
             │           ├ generate_backup_codes
             │           ├ register_totp
             │           ├ register_webauthn
             │           ├ remove_backup_codes
             │           ├ remove_totp
             │           ├ reset_credential
             │           ├ remove_webauthn
             │           ├ set_password
             │           └ status
             │
             ├ person
             │       └ set --legalname --mail
             │
             ├ posix
             │      ├ set_password
             │      └ show
             │
             ├ radius
             │       ├ generate_secret
             │       ├ delete_secret
             │       └ show_secret
             │
             └ ssh
                  ├ add_publickey
                  ├ delete_publickey
                  └ list_publickeys

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)....

Yes, the layout has evolved :)

I'm certainly open to better and more natural suggestions!

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.

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 :)

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 the posix 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.

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.

Initial decision points

The above raised two questions which I think are worth considering:

  1. Should the server API include dedicated APIs for "self" commands, or should those commands typically lean on existing account APIs
    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"?

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.

  1. Should the command structure for account vs person differ? Why/why not?

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.

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)

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!

@yaleman
Copy link
Member

yaleman commented Apr 9, 2022

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 self mapping shortcut, just because it's more code to maintain and code accumulates bugs. As noted by @Firstyear the user knows their own UUID from sign-in and even if they didn't, a call to /whoami isn't the worst thing if it needed to be done, self actions shouldn't happen that often :)

@yaleman
Copy link
Member

yaleman commented Apr 9, 2022

From a CLI -> credential perspective, maybe a slightly more hierarchical tree like this?

$ kanidm self
     ├ credential
     |       ├ totp
     |       |     ├ generate_backup_codes
     │       |     ├ remove_backup_codes <+- this can probably go, since generate should reset them?
     │       |     ├ register
     │       |     └ remove
     │       ├ webauthn
     │       |     ├ register
     │       |     └ remove
     │       ├ reset_all_credentials
     │       ├ set_password
     │       └ status

@Firstyear
Copy link
Member

@yaleman Well, given how credential is being reworked atm it's more likely to be:

kanidm self
    |- credential
    |    |- primary
    |    |    |- password
    |    |    |- totp
    |    |    |- security key
    |    |
    |    |- trusted devices
    |    |    |- list
    |    |    |- enroll
    |    |    |- remove

But I think we can focus on that a bit in the current credentual update session pr I'll be posting soon.

@Firstyear
Copy link
Member

I'd argue against the self mapping shortcut, just because it's more code to maintain and code accumulates bugs. As noted by @Firstyear the user knows their own UUID from sign-in and even if they didn't, a call to /whoami isn't the worst thing if it needed to be done, self actions shouldn't happen that often :)

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.

@yaleman
Copy link
Member

yaleman commented Apr 11, 2022

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.

@Firstyear
Copy link
Member

You're completely right, that's a good point :)

@MateusAmin
Copy link
Contributor

MateusAmin commented May 27, 2022

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 discussion

Update CLI Structure

Sooooooooo, 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 Structure

Remove 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 Clap

I 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 Dynamically

The 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:

clap-rs/clap#2914

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 Map

There 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.

@yaleman
Copy link
Member

yaleman commented May 28, 2022

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.

@MateusAmin MateusAmin linked a pull request May 29, 2022 that will close this issue
5 tasks
@Firstyear
Copy link
Member

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 discussion

Update CLI Structure

Sooooooooo, 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.

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!

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.

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 :)

I suspect we should still be able increase code reuse overall as we do this.

Update API Structure

Remove 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.

Yep, that's clear cut :)

Migrate StructOps to Clap

I 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

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!

Hiding Interface Dynamically

The 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:

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 :)

clap-rs/clap#2914

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 Map

There 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.

This is prettttyyyyy close to correct.

Self is actually self = i.account. An "account" is "any entity that can authenticate to kanidm". This could be a person or a service account etc. A person is a superset of account, which has other personal identifying elements, and extra privileges such as self-write available. But as you correctly identify, a "person" means that the "person + account" is related to a human (and comes with the need to respect their human-identity etc). Where a pure "account" is likely related to a service such as a webservice or radius server etc, and is "administered" externally.

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:

kanidm login -D william
kanidm self show

I'd want to see "myself".

The same way that kanidm accounts is about "acting on others".

So I think you get it :)

@leoleoasd
Copy link
Contributor

My search of "update self" lead me here. After a whole year, I want to ask:

  1. What's the current status of this issue? The API change is still going to happen right?
  2. Is there any current way for a non-priviledged user to see and update it's own email?

@Firstyear
Copy link
Member

  1. What's the current status of this issue? The API change is still going to happen right?

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.

  1. Is there any current way for a non-priviledged user to see and update it's own email?

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. idm_people_self_write_mail_priv

@yaleman yaleman added the api REST API things label Aug 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api REST API things
Projects
Status: 📋 Backlog
Development

Successfully merging a pull request may close this issue.

5 participants