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

Rewriting the user modify command #126

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

JacksonChen666
Copy link
Collaborator

@JacksonChen666 JacksonChen666 commented Sep 4, 2023

Currently a placeholder. Only about the structure of commands (and maybe the parameters) until agreed.

Initial idea: #100 (comment)

Resolves #100

Placeholder/existing commands checklist

(Commits history will 100% get rewritten after the code is finished)


Progress checklist:

@JOJ0
Copy link
Owner

JOJ0 commented Sep 6, 2023

Thanks for getting this started!

@JacksonChen666 JacksonChen666 requested review from JOJ0 and removed request for JOJ0 September 10, 2023 21:00
@JacksonChen666
Copy link
Collaborator Author

JacksonChen666 commented Sep 10, 2023

@JOJ0 Please review the structure of the commands and the API and request any changes.

Copy link
Owner

@JOJ0 JOJ0 left a comment

Choose a reason for hiding this comment

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

Conclusive thoughts: If there are verbs in a command they are self-descriptive. Eg. user deactivate

If there are not they are not 100% obvious what they do (display or modify). Eg. user display-name

Not sure if I'm nitpicking here too much or if the priority should be "shorter" names. Eg. set-display-name is rather tedious to type and might be a bad choice...

@@ -600,6 +600,7 @@ def user_modify(self, user_id, password, display_name, threepid,

Threepid should be passed as a tuple in a tuple
"""
# TODO: deprecate
Copy link
Owner

Choose a reason for hiding this comment

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

I agree we should deprecate synadm user modify within this PR, or actually I'd like to achieve this: As soon as everything from user modify has its own command or the user should be warned - best with a literal log.warning.

But I'm not sure anymore wether we'd like to keep some parts of user modify as-is and not invent new commands. Can't recall what was the outcome of that discussion, please help me remember! :-)

If we would keep parts of modify as-is and do not plan to change, then probably it would be better that exactly when these options are used, a log.warning is spit out. Not sure though what's best....

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But I'm not sure anymore wether we'd like to keep some parts of user modify as-is and not invent new commands. Can't recall what was the outcome of that discussion, please help me remember! :-)

If we would keep parts of modify as-is and do not plan to change, then probably it would be better that exactly when these options are used, a log.warning is spit out. Not sure though what's best....

I think deprecating by warning the user is the best option, and we could remove the modify command in the future.

We also do not add any new functions to the modify command to discourage use of it.

@@ -500,6 +500,80 @@ def modify(ctx, helper, user_id, password, password_prompt, display_name,
click.echo("Abort.")


# for these placeholders, function similarly to the modify command, but in
# it's own command.
# TODO: find 3pid modify command
Copy link
Owner

Choose a reason for hiding this comment

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

We have a 3pid command already, but it's for displaying only: https://synadm.readthedocs.io/en/latest/synadm.cli.user.html#synadm-user-3pid

That I guess is what you are thinking about? Where should we put / and how should we name the modify command for 3pid modify?

Currently 3pid modify is included with the original user modify command: https://synadm.readthedocs.io/en/latest/synadm.cli.user.html#cmdoption-synadm-user-modify-t

@click.pass_obj
def reactivate(helper, user_id):
# TODO
# TODO: does this require a password? merge into password? user
Copy link
Owner

Choose a reason for hiding this comment

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

It definitely does require a password.

Look for this phrase "Note: the password field must also be set if both of the following are true:" around here: https://matrix-org.github.io/synapse/latest/admin_api/user_admin_api.html#create-or-modify-account

Copy link
Collaborator Author

@JacksonChen666 JacksonChen666 Sep 19, 2023

Choose a reason for hiding this comment

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

It only requires a password if synapse is storing a password, so I'm thinking of a required flag of either --password $PASSWORD (must have argument) or --no-password (no argument), but I'm not sure how to do that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

--password $PASSWORD (must have argument)

Actually that's a bad idea and we should ask for the password on stdin instead of on the command line arguments.

Copy link
Owner

@JOJ0 JOJ0 Sep 28, 2023

Choose a reason for hiding this comment

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

I basically agree but on the other hand: why not borrow the ideas we have over at the user password command?

I think we have both: stdin and cli arg

I still think that securitywise it's an admin's choice whether they want to eg. modify 10 testusers on a playground server - scripted! Or they'd like to securely modify one real user's account on the cli.

It's an old unix philosophy: Know what you are doing if you are root!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So:

  • --prompt (default, mutually exclusive of --no-prompt)
  • --no-prompt (mutually exclusive of --prompt, leave errors up to synapse)
  • --password (likely insecure but useful for automation in testing environments, takes priority over stdin options)

Those arguments will probably also apply to the password command, if it doesn't have such (and it doesn't seem like the password command has such options).

Tangent: We do not securely prompt for a token in synadm config. We really should do that (no echo and iTerm enables secure keyboard entry on password prompt), but I'm not sure how to do that.

Copy link
Owner

@JOJ0 JOJ0 Oct 2, 2023

Choose a reason for hiding this comment

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

Regarding reusing of an option or even a couple of options, I think I found a nice way we could try. Creating a decorator that applies a group of options. Have a look at the very bottom of this post: https://stackoverflow.com/a/66743881/10207149

What do you think?

Copy link
Owner

@JOJ0 JOJ0 Oct 13, 2023

Choose a reason for hiding this comment

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

  • --prompt (default, mutually exclusive of --no-prompt)
  • --no-prompt (mutually exclusive of --prompt, leave errors up to synapse)
  • --password (likely insecure but useful for automation in testing environments, takes priority over stdin options)

Exactly, something like that!

Those arguments will probably also apply to the password command, if it doesn't have such (and it doesn't seem like the password command has such options).

Oh my bad. When I talked about "borrowing ideas of user password command" I probably ment the user modify command - We have --password and --password-prompt there.

As we had it so often already with Click/synadm, copying boilerplate code is the quick way but it would be nice if we could find a way to prepare these options once and reuse them in several commands. That also applies to any sanity-checking code we'd require on the UI end.

Regarding password options streamlining, I would love if we could make use of the _common module I introduced here: 79d0ea4

We could put all password-related options into that module and reuse them with all password-related commands.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could put all password-related options into that module and reuse them with all password-related commands.

ahem: https://click.palletsprojects.com/en/8.1.x/api/#click.password_option

also, 79d0ea4 doesn't look like it's part of the master branch, so i'll not do that yet.

Copy link
Owner

Choose a reason for hiding this comment

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

Sure, certainly I ment once it's merged ;-)

Copy link
Owner

@JOJ0 JOJ0 Oct 14, 2023

Choose a reason for hiding this comment

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

And yes: I agree we should also make use of that click.password_option

For some reason I did not use it in my intial implementation of user password - there might be a reason for it but it could also be that I just didn't read the docs carfully enough (it's been around since Click 6.x at least or even since always ;-). I used a confirmation prompt thing though :-)

$ synadm user password testuser3
Password:
Repeat for confirmation:
{}

synadm/synadm/cli/user.py

Lines 286 to 288 in be77cef

@click.option(
"--password", "-p", prompt=True, hide_input=True,
confirmation_prompt=True, help="New password.")

We do also have use-cases were this option might not be enough....for example when handling with tokens there is more to it than either by prompt or by passing a cli arg, for example passing by environment variable as well... probably off-topic here....just mentioning..

synadm/cli/user.py Outdated Show resolved Hide resolved
@JacksonChen666
Copy link
Collaborator Author

JacksonChen666 commented Sep 19, 2023

checklist of what's been planned/already done so far:

  • password
  • display name
  • avatar URL
  • changing 3pid
  • external_ids (not part of synadm user modify?)
  • admin grant
  • admin revoke
  • deactivation
  • reactivation
  • locking
  • unlocking
  • user types

@JacksonChen666
Copy link
Collaborator Author

JacksonChen666 commented Sep 19, 2023

Conclusive thoughts: If there are verbs in a command they are self-descriptive. Eg. user deactivate

If there are not they are not 100% obvious what they do (display or modify). Eg. user display-name

Not sure if I'm nitpicking here too much or if the priority should be "shorter" names. Eg. set-display-name is rather tedious to type and might be a bad choice...

It should be with a set if it's going to set it. I didn't have plans to make it retrieve the display name and display it. Maybe a --set flag which can set a display name could work, while it could be handy to also see the display name (get by default, which will use the user query API).

@JacksonChen666
Copy link
Collaborator Author

checklist of what's been planned/already done so far:
- [ ] changing 3pid
- [ ] external_ids (not part of synadm user modify?)

I'm gonna leave those behind for later, keeping the synadm user modify command, maybe implementing modifying those later and deprecating when replacements have been implemented.

@JOJ0
Copy link
Owner

JOJ0 commented Oct 2, 2023

checklist of what's been planned/already done so far:

  • password
  • display name
  • avatar URL
    ...

Nice summary! Why not move it to the initial description of this PR? Easier to find and also for future reference what's still missing/postponed (as I just read in your subsequent post you plan on doing)

@JOJ0
Copy link
Owner

JOJ0 commented Oct 2, 2023

Conclusive thoughts: If there are verbs in a command they are self-descriptive. Eg. user deactivate
If there are not they are not 100% obvious what they do (display or modify). Eg. user display-name
Not sure if I'm nitpicking here too much or if the priority should be "shorter" names. Eg. set-display-name is rather tedious to type and might be a bad choice...

It should be with a set if it's going to set it. I didn't have plans to make it retrieve the display name and display it. Maybe a --set flag which can set a display name could work, while it could be handy to also see the display name (get by default, which will use the user query API).

Good question what's best....

I just had a look on your work so far in reality/--help:

  admin-grant
  admin-revoke
   ...
  set-display-name     Set the display name of a user.
  set-profile-picture  Set a profile picture for a user by the MXC URI.
  set-user-type
  ...

It seems there is no one-way of doing it right - verb in front, verb on the end of the command. Too long, too short command names....I'm not sure where to go actually....

One thing I'm wondering though: --help as the go-to move for most people (I hope! ;-)) - If we would find a way to be able to manually sort how the options are presented it would be less of a headache for the naming....

let me check, I think I researched this already ages ago and by default Click doesn't have it.....

pallets/click#672

pallets/click#513

@JacksonChen666
Copy link
Collaborator Author

JacksonChen666 commented Oct 2, 2023

checklist of what's been planned/already done so far:
Nice summary! Why not move it to the initial description of this PR? Easier to find and also for future reference what's still missing/postponed (as I just read in your subsequent post you plan on doing)

See my PR description:

Resolves #100

Placeholder/existing commands checklist

Progress checklist:

@JOJ0
Copy link
Owner

JOJ0 commented Oct 2, 2023

Ah ok, not sure how you ment that. You will move the checklist up into the description once it's finished?

Anyway, all good, work away! :-)

@JacksonChen666
Copy link
Collaborator Author

JacksonChen666 commented Oct 2, 2023

Good question what's best....

I think at this point, we have a mostly clear structure although with not so clear command names.

That should allow me to start implementing the commands while we continue deciding on the command names until we find some answer. We can easily change the name later (although less so, the structure, if necessary).

I just had a look on your work so far in reality/--help:

  admin-grant
  admin-revoke
   ...
  set-display-name     Set the display name of a user.
  set-profile-picture  Set a profile picture for a user by the MXC URI.
  set-user-type
  ...

It seems there is no one-way of doing it right - verb in front, verb on the end of the command. Too long, too short command names....I'm not sure where to go actually....

One thing I'm wondering though: --help as the go-to move for most people (I hope! ;-)) - If we would find a way to be able to manually sort how the options are presented it would be less of a headache for the naming....

I have these ideas for command names for setting stuff:

  1. set-thingy-thing (current, slightly messes with ordering)
  2. thingy-thing set and thingy-thing get (subcommand, not consistent with the rest)
  3. thingy-thing --set and thingy-thing --get (command with arg to either set or get)
  4. thingy-thing (not as ideal as discussed)
  5. set-tt or stt or tt (aliases, not sure if possible, e.g. pfp or set-pfp for set-profile-picture)

@JacksonChen666
Copy link
Collaborator Author

Ah ok, not sure how you ment that. You will move the checklist up into the description once it's finished?

No, not really. The checklist in the PR description shows up in PR listing as how much has been done. Including the placeholders would add to that number, which isn't really desirable since not all commands have been included, which means not 100% of the tasks done.

Although on whether to include checklist or not in the PR description, I'm not sure which side to err on.

@JOJ0
Copy link
Owner

JOJ0 commented Oct 2, 2023

Please go ahead with implementation and just use what you suggested / we have so far! Yes we could think out for ages on how commands should be named :-) Let's see when implementation is finished whether we'd like to change them.

@JacksonChen666
Copy link
Collaborator Author

Just realized, user modify API can also create users. Not sure how we'll deal with that, we might have to keep the user modify command around and possibly also update it with new stuff.

@JOJ0
Copy link
Owner

JOJ0 commented Oct 13, 2023

Just realized, user modify API can also create users. Not sure how we'll deal with that, we might have to keep the user modify command around and possibly also update it with new stuff.

Oh, I'm sorry I didn't mention that, I was just assuming you are aware of that. I also didn't realise/think hard enough that it could be a problem with all the "redesign"..... :-/ Sorry my bad!

@JOJ0
Copy link
Owner

JOJ0 commented Nov 16, 2023

Just realized, user modify API can also create users. Not sure how we'll deal with that, we might have to keep the user modify command around and possibly also update it with new stuff.

Oh, I'm sorry I didn't mention that, I was just assuming you are aware of that. I also didn't realise/think hard enough that it could be a problem with all the "redesign"..... :-/ Sorry my bad!

To wrap this up and partly spice with some new ideas. I was thinking about all this a little:

The current api.user_modify() becomes the user_create() function - we need to keep it anyway as you mentioned. It only is used for create, that should spare us at least a little headache because some things are just not relevant when creating in contrast to modifying). For tri-state options, we could even think if enums could help? Or we keep the current None, True, False business....maybe that's enough. Don't kill me yet, I didn't think through each and every thing we want to set 😅 )

Modifying is not possible anymore with this new method api.user_create()! The newly designed user set... whatever` commands take care of that, each with their own api methods, just as you designed and drafted it already.

Maybe we can reuse _ at least some_ of the cli-options by putting them into the cli.common module? Not sure! Often the wording in --help will be different, which makes reusing not possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User modify API requires a redesign
2 participants