Rewriting the user modify command #126

JacksonChen666 · 2023-09-04T20:03:02Z

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:

Placeholders (see checklist and Rewriting the user modify command #126 (comment))
Agreed placeholders (agreed)
Implementation
TODOs (deprecating, check synadm user reactivate with --batch)
Testing everything new (including deprecation warning)
Make README up to date with these changes

JOJ0 · 2023-09-06T14:25:09Z

Thanks for getting this started!

JacksonChen666 · 2023-09-10T21:03:48Z

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

JOJ0

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

JOJ0 · 2023-09-18T05:18:40Z

synadm/api.py

@@ -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


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

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.

JOJ0 · 2023-09-18T05:23:49Z

synadm/cli/user.py

@@ -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


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

JOJ0 · 2023-09-18T05:24:55Z

synadm/cli/user.py

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


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

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.

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

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!

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.

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?

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

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.

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

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

JacksonChen666 · 2023-09-19T11:12:36Z

JacksonChen666 · 2023-09-19T11:36:02Z

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 · 2023-09-25T07:51:24Z

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 · 2023-10-02T06:03:26Z

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 · 2023-10-02T06:26:49Z

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 · 2023-10-02T06:43:21Z

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:

Placeholders (see checklist and Rewriting the user modify command #126 (comment))

Agreed placeholders

Implementation

Make README up to date with these changes

JOJ0 · 2023-10-02T06:48:36Z

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 · 2023-10-02T06:56:28Z

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:

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

JacksonChen666 · 2023-10-02T07:02:57Z

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 · 2023-10-02T07:38:37Z

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.

to not duplicate the same query code

JacksonChen666 · 2023-10-12T13:31:14Z

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 · 2023-10-13T10:25:58Z

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 · 2023-11-16T11:15:03Z

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.

JacksonChen666 requested a review from JOJ0 September 4, 2023 20:03

JacksonChen666 requested review from JOJ0 and removed request for JOJ0 September 10, 2023 21:00

JOJ0 reviewed Sep 18, 2023

View reviewed changes

JacksonChen666 requested a review from JOJ0 September 25, 2023 10:21

JacksonChen666 self-assigned this Oct 6, 2023

JacksonChen666 force-pushed the user-modify-rewrite branch from f05f3b7 to 87ace0f Compare October 12, 2023 11:09

JacksonChen666 added 7 commits October 12, 2023 15:20

add user modify api wrapper

e14cf47

to not duplicate the same query code

refactor all to use the user mod wrapper API

a765775

implement setting user display name

6316036

implement setting profile pictures

6bebbd4

implement user locking and unlocking

e2839bc

implement admin grant and revoke

b8d7cd0

implement user reactivate

ad40728

JacksonChen666 force-pushed the user-modify-rewrite branch from 87ace0f to dc615e4 Compare October 12, 2023 13:24

JacksonChen666 added 2 commits October 12, 2023 15:27

implement setting user type

9aa5fbe

crap: some comment and TODOs

70fdb89

JacksonChen666 force-pushed the user-modify-rewrite branch from dc615e4 to 70fdb89 Compare October 12, 2023 15:07

JacksonChen666 mentioned this pull request Nov 12, 2023

User modify API requires a redesign #100

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewriting the user modify command #126

Rewriting the user modify command #126

JacksonChen666 commented Sep 4, 2023 •

edited

JOJ0 commented Sep 6, 2023

JacksonChen666 commented Sep 10, 2023 •

edited

JOJ0 left a comment

JOJ0 Sep 18, 2023

JacksonChen666 Sep 19, 2023

JOJ0 Sep 18, 2023

JOJ0 Sep 18, 2023

JacksonChen666 Sep 19, 2023 •

edited

JacksonChen666 Sep 25, 2023

JOJ0 Sep 28, 2023 •

edited

JacksonChen666 Sep 28, 2023

JOJ0 Oct 2, 2023 •

edited

JOJ0 Oct 13, 2023 •

edited

JacksonChen666 Oct 13, 2023

JOJ0 Oct 13, 2023

JOJ0 Oct 14, 2023 •

edited

JacksonChen666 commented Sep 19, 2023 •

edited

JacksonChen666 commented Sep 19, 2023 •

edited

JacksonChen666 commented Sep 25, 2023

JOJ0 commented Oct 2, 2023 •

edited

JOJ0 commented Oct 2, 2023 •

edited

JacksonChen666 commented Oct 2, 2023 •

edited

JOJ0 commented Oct 2, 2023

JacksonChen666 commented Oct 2, 2023 •

edited

JacksonChen666 commented Oct 2, 2023

JOJ0 commented Oct 2, 2023

JacksonChen666 commented Oct 12, 2023

JOJ0 commented Oct 13, 2023

JOJ0 commented Nov 16, 2023 •

edited

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

Rewriting the user modify command #126

Are you sure you want to change the base?

Rewriting the user modify command #126

Conversation

JacksonChen666 commented Sep 4, 2023 • edited

JOJ0 commented Sep 6, 2023

JacksonChen666 commented Sep 10, 2023 • edited

JOJ0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JacksonChen666 Sep 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JOJ0 Sep 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JOJ0 Oct 2, 2023 • edited

Choose a reason for hiding this comment

JOJ0 Oct 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JOJ0 Oct 14, 2023 • edited

Choose a reason for hiding this comment

JacksonChen666 commented Sep 19, 2023 • edited

JacksonChen666 commented Sep 19, 2023 • edited

JacksonChen666 commented Sep 25, 2023

JOJ0 commented Oct 2, 2023 • edited

JOJ0 commented Oct 2, 2023 • edited

JacksonChen666 commented Oct 2, 2023 • edited

JOJ0 commented Oct 2, 2023

JacksonChen666 commented Oct 2, 2023 • edited

JacksonChen666 commented Oct 2, 2023

JOJ0 commented Oct 2, 2023

JacksonChen666 commented Oct 12, 2023

JOJ0 commented Oct 13, 2023

JOJ0 commented Nov 16, 2023 • edited

JacksonChen666 commented Sep 4, 2023 •

edited

JacksonChen666 commented Sep 10, 2023 •

edited

JacksonChen666 Sep 19, 2023 •

edited

JOJ0 Sep 28, 2023 •

edited

JOJ0 Oct 2, 2023 •

edited

JOJ0 Oct 13, 2023 •

edited

JOJ0 Oct 14, 2023 •

edited

JacksonChen666 commented Sep 19, 2023 •

edited

JacksonChen666 commented Sep 19, 2023 •

edited

JOJ0 commented Oct 2, 2023 •

edited

JOJ0 commented Oct 2, 2023 •

edited

JacksonChen666 commented Oct 2, 2023 •

edited

JacksonChen666 commented Oct 2, 2023 •

edited

JOJ0 commented Nov 16, 2023 •

edited