Make it possible to change user type #90

gergelypolonkai · 2023-02-03T09:23:36Z

The user_type value is not checked (yet?) the code, although the spec allows only types bot and support (and the API will respond with M_UNKNOWN if an invalid value is sent). If it should be checked, let me know and i will update the PR.

Closes JOJ0#89

JOJ0

Regarding your questionaround sanity check: In the end we'd like a user to not having to specify --user-type, it should have a resonable default. Post some example cli calls, that would help me review without testing myself :-) Cheers!

synadm/cli/user.py

JOJ0 · 2023-02-03T10:31:40Z

synadm/api.py

@@ -556,6 +556,7 @@ def user_modify(self, user_id, password, display_name, threepid,
            data.update({"deactivated": True})
        if deactivation == "activate":
            data.update({"deactivated": False})
+        data.update({"user_type": user_type})


What if it's None? What does the API respond then? Valid? Was that the M_UNKNOWN response you were referring to?

Yes, user_type can be null in the JSON in which case the field gets cleared (which is basically the “default” option).

Basically that's ok but the bad thing is that if any other property of a user gets changed, the user_type is always cleared. Do we have this behavior somewhere else already by any chance?

Now that you mention it, it seems the activate/deactivate thing works just like that 😁 iʼll check tomorrow as itʼs bedtime for me now.

I was right; the activate/deactivate feature works just like this. If i don’t add --user-type nor --clear-user-type to the command line, it won’t get updated:

$ synadm user modify --password abcdef @test:matrix.local Current user account settings: {"name": "@test:matrix.local "is_guest": 0, "admin": false, "consent_version": null, "consent_ts": null, "consent_server_notice_sent": null, "appservice_id": null, "creation_ts": 1457796444, "user_type": null, "deactivated": false, "shadow_banned": false, "displayname": "Test Account", "avatar_url": null, "threepids": [], "external_ids": [], "erased": false} User account settings to be modified: Password will be set as provided on command line. Are you sure you want to modify user? (y/N):

I just read the api docs: https://matrix-org.github.io/synapse/develop/admin_api/user_admin_api.html#create-or-modify-account

user_type is optional, I think you just need to make sure it isnt included at all when both of the 2 new options are not given on cli. So nothing gets cleared 🎉 Currently you always include it in the data sent to the endpoint.

@gergelypolonkai I've added the missing condition as suggested in my previous post. I've just edited your feature branch. I hope that's ok.

This change was necessary to prevent resetting user_type back to null, as we see in this output recorded prior to the fix:

(synadm38) ~/git/gergelypolonkai_synadm (user-type-edit) $ synadm user modify --display-name "TESTUSER1" testuser1 Current user account settings: admin: 0 appservice_id: null avatar_url: null consent_server_notice_sent: null consent_version: null creation_ts: 1573653091 deactivated: 0 displayname: Test User 1 external_ids: [] is_guest: 0 name: '@testuser1:peek-a-boo.at' password_hash: $secret shadow_banned: null threepids: [] user_type: bot User account settings to be modified: display_name: TESTUSER1 Are you sure you want to modify user? (y/N): y admin: 0 appservice_id: null avatar_url: null consent_server_notice_sent: null consent_version: null creation_ts: 1573653091 deactivated: 0 displayname: TESTUSER1 external_ids: [] is_guest: 0 name: '@testuser1:peek-a-boo.at' password_hash: $secret shadow_banned: null threepids: [] user_type: null

synadm/cli/user.py

gergelypolonkai · 2023-02-03T12:16:31Z

we'd like a user to not having to specify --user-type

You don’t have to, but you can. Not having a user-type set (which can be achieved by using the --clear-user-type command line option) is the default. According to the spec, a user can have their user type set to null (default), "bot", and "support" (whatever that may mean). Specifying any other type on the API call will result in M_UNKNOWN.

Leave out user_type in the post data entirely when user_type is not passed from the cli, otherwise it would be reset to 'null', which is unexpected behaviour. According to the admin API docs the json text for user_type is optional.

JOJ0 · 2023-02-22T10:01:27Z

We are almost ready to merge. I see a tiny thing in --help: Allowed values are formatted differently. 'bot' has single quotes and 'support' hasn't. Please add that @gergelypolonkai, thank you!

Other than that, LGTM. Final thoughts from @Ascurius and/or @JacksonChen666 would be appreciated! One usability thing I'm wondering about is: Is --clear-user-type really necessary or would something like --user-type clear or user-type null be a good alternative? I'm really not sure about whether that would be a "clean" and expected solution for a unix-shell-util. Give me your thoughts! Thanks!

synadm/cli/user.py

JacksonChen666 · 2023-02-23T16:23:54Z

One usability thing I'm wondering about is: Is --clear-user-type really necessary or would something like --user-type clear or user-type null be a good alternative? I'm really not sure about whether that would be a "clean" and expected solution for a unix-shell-util. Give me your thoughts! Thanks!

I'm not sure either... --user-type clear could make sense but --clear-user-type also makes sense but I don't know which I would prefer (maybe the former?). --user-type null is quite specific and doesn't really say what it is, so maybe not that.

JOJ0 · 2023-02-26T19:19:12Z

Hi @gergelypolonkai, I would like to take over this PR entirely, since we would like to include it in the next synadm release and there is some things left to do. I hope that is ok for you. Thanks for the submission of a useful feature!

Co-authored-by: Jackson Chen <jackson@jacksonchen666.com>

JOJ0 · 2023-02-26T19:31:00Z

One usability thing I'm wondering about is: Is --clear-user-type really necessary or would something like --user-type clear or user-type null be a good alternative? I'm really not sure about whether that would be a "clean" and expected solution for a unix-shell-util. Give me your thoughts! Thanks!

I'm not sure either... --user-type clear could make sense but --clear-user-type also makes sense but I don't know which I would prefer (maybe the former?). --user-type null is quite specific and doesn't really say what it is, so maybe not that.

I think I have made up my mind what seems to feel natural to me:

Remove --clear-user-type
Document --user-type well so it explains that 3 options are currently valid (or even just prevent any other options by Click's choice feature?)
- bot
- support
- default (This is technically setting to 'null', thus a regular user, actually a default matrix user, nothing else)
@JacksonChen666 @Ascurius what do you think?

The term default is not stated as such in the admin api docs (https://matrix-org.github.io/synapse/latest/admin_api/user_admin_api.html#create-or-modify-account) but I still think that it's a very logical and self-explaining thing if we just call this user_type default

JOJ0 · 2023-02-26T19:41:10Z

I think I have made up my mind what seems to feel natural to me:

Remove --clear-user-type

Document --user-type well so it explains that 3 options are currently valid (or even just prevent any other options by Click's choice feature?)

bot

support

default (This is technically setting to 'null', thus a regular user, actually a default matrix user, nothing else)

I just wanted to put these changes into "suggested changes" with this neat feature here but I decided not to. It's quite some changes necessary and it's easier and less errror-prone if I just push that to the branch as one commit. I'll wait with that until you tell me your opinions about this idea.

Thanks in advance!

JacksonChen666

Looks good to me. There are a few things I noticed:

--clear-user-type or --user-type null, I'm not sure.
--user-type will accept any argument, including non-existing user types. Should that be restricted to known valid options?

JOJ0 · 2023-03-04T13:09:21Z

Looks good to me. There are a few things I noticed:

--clear-user-type or --user-type null, I'm not sure.

I really don't like --user-type null, it is not self-explanatory at all. If there is no strong opinions against --user-type default, I will use that one instead. In my opinion it tells everything to a user who did not read the API docs at all. A standard user / a default user will be created / existing user will become that.

--user-type will accept any argument, including non-existing user types. Should that be restricted to known valid options?

I was not sure about the restriction of the possible arguments either but would now say that it's better we leave this to unrestricted for now, since it could happen that new things are added to the list. The API will give us a nice response of M_UKNOWN

JacksonChen666 · 2023-03-04T13:12:59Z

I really don't like --user-type null, it is not self-explanatory at all. If there is no strong opinions against --user-type default, I will use that one instead. In my opinion it tells everything to a user who did not read the API docs at all. A standard user / a default user will be created / existing user will become that.

I would lean towards a default user, Though I'm not sure what a user would think about a "default" user cause that doesn't seem extremely obvious either...

--clear-user-type is kind of awkward. What about --user-type clear?

Remove --clear-user-type option from user modify command and support an argument named 'regular' that submits a value of 'null' which effectively creates a default/regular matrix user.

JacksonChen666

> synadm user modify '@130:localhost:8480' --user-type support
Current user account settings:
{"name": "@130:localhost:8480", "is_guest": 1, "admin": true, "consent_version": null, "consent_ts": null, "consent_server_notice_sent": null, "appservice_id": null, "creation_ts": 1676753770, "user_type": "bot", "deactivated": false, "shadow_banned": false, "displayname": "130", "avatar_url": null, "threepids": [], "external_ids": [], "erased": false}
User account settings to be modified:
user_type: support
Are you sure you want to modify user? (y/N): y
{"name": "@130:localhost:8480", "is_guest": 1, "admin": true, "consent_version": null, "consent_ts": null, "consent_server_notice_sent": null, "appservice_id": null, "creation_ts": 1676753770, "user_type": "support", "deactivated": false, "shadow_banned": false, "displayname": "130", "avatar_url": null, "threepids": [], "external_ids": [], "erased": false}

> synadm user modify '@130:localhost:8480' --user-type regular
Current user account settings:
{"name": "@130:localhost:8480", "is_guest": 1, "admin": true, "consent_version": null, "consent_ts": null, "consent_server_notice_sent": null, "appservice_id": null, "creation_ts": 1676753770, "user_type": "support", "deactivated": false, "shadow_banned": false, "displayname": "130", "avatar_url": null, "threepids": [], "external_ids": [], "erased": false}
User account settings to be modified:
user_type: null
Are you sure you want to modify user? (y/N): y
{"name": "@130:localhost:8480", "is_guest": 1, "admin": true, "consent_version": null, "consent_ts": null, "consent_server_notice_sent": null, "appservice_id": null, "creation_ts": 1676753770, "user_type": "support", "deactivated": false, "shadow_banned": false, "displayname": "130", "avatar_url": null, "threepids": [], "external_ids": [], "erased": false}

This is broken and I don't know why (or it doesn't work (or not for guest accounts)). Also applies to revoking admin privileges (unrelated to this PR).

Also, the "regular" option feels kinda odd.

synadm/cli/user.py

JacksonChen666 · 2023-03-07T09:57:31Z

https://github.com/gergelypolonkai/synadm/blob/dba8132dd74fe4be1129a78e07dd26806a6331d8/synadm/api.py#L536-L561

None is not considered truthy, so using None as "default" would cause the values to never change. This makes for a poorly functional API that is also very poorly defined (the severe lack of docs is included).

Basically, we have a problem that has nothing to do with this PR, but the PR has the same problems.
We can merge this PR without fixing the issue, but we definitely need to fix the issue which is more than just user types (trying to remove admin doesn't work because of a similar issue (False will not pass an implicit if condition)).

JOJ0 · 2023-03-07T10:37:16Z

I had just realized this issue but needed a break...consider my latest commit a draft

JOJ0 · 2023-03-07T10:46:52Z

> synadm user modify '@130:localhost:8480' --user-type support
Current user account settings:
{"name": "@130:localhost:8480", "is_guest": 1, "admin": true, "consent_version": null, "consent_ts": null, "consent_server_notice_sent": null, "appservice_id": null, "creation_ts": 1676753770, "user_type": "bot", "deactivated": false, "shadow_banned": false, "displayname": "130", "avatar_url": null, "threepids": [], "external_ids": [], "erased": false}
User account settings to be modified:
user_type: support
Are you sure you want to modify user? (y/N): y
{"name": "@130:localhost:8480", "is_guest": 1, "admin": true, "consent_version": null, "consent_ts": null, "consent_server_notice_sent": null, "appservice_id": null, "creation_ts": 1676753770, "user_type": "support", "deactivated": false, "shadow_banned": false, "displayname": "130", "avatar_url": null, "threepids": [], "external_ids": [], "erased": false}

> synadm user modify '@130:localhost:8480' --user-type regular
Current user account settings:
{"name": "@130:localhost:8480", "is_guest": 1, "admin": true, "consent_version": null, "consent_ts": null, "consent_server_notice_sent": null, "appservice_id": null, "creation_ts": 1676753770, "user_type": "support", "deactivated": false, "shadow_banned": false, "displayname": "130", "avatar_url": null, "threepids": [], "external_ids": [], "erased": false}
User account settings to be modified:
user_type: null
Are you sure you want to modify user? (y/N): y
{"name": "@130:localhost:8480", "is_guest": 1, "admin": true, "consent_version": null, "consent_ts": null, "consent_server_notice_sent": null, "appservice_id": null, "creation_ts": 1676753770, "user_type": "support", "deactivated": false, "shadow_banned": false, "displayname": "130", "avatar_url": null, "threepids": [], "external_ids": [], "erased": false}

This is broken and I don't know why (or it doesn't work (or not for guest accounts)). Also applies to revoking admin privileges (unrelated to this PR).

Also, the "regular" option feels kinda odd.

Yep, I know it's broken. I didn't ask for a review yet for my latest commit, I was not finished, you are too fast (again ,-))))

Appreciated though! And very nice catch with admin privileges! I was not aware! That sucks!

by simply passing a string. Otherwise setting the user_type to None (which means value 'null' in submitted json) would not be possible.

synadm/api.py

JOJ0 · 2023-03-07T11:12:21Z

Ok, setting user-type now works. I worked around the "poor desing" of user_modify() by just passing a string 'null'.
Well, yes, that is ugly, but it will work. Rerfactoring of that method should be tackled in another PR, I agree. Thanks for opening the issue.

So the only thing that remains that you still think we don't have a good name for creating a regular, a normal, a default matrix user-type. I am running out of ideas and still think putting something like 'null' or 'clear' is not good usability, nor is ist it obvious what the user should expect. They would need to read the admin api docs to understand what that means.

So that's why I still would like to ask you for a suggestion of how you think from a user-perspecitive what should that option be called. Consider these english sentences, which one does make the most self-explanatory sense to you?

A user of the type 'clear'
A user of the type 'default'
A user of the type 'regular'
A user of the type 'null'
A user of the type 'normal'

a oneline-condition.

JOJ0 · 2023-03-07T11:22:44Z

Ok so besides the final decision for option name I think this is good to go now. Please a final review @JacksonChen666 and thanks for all the suggestions!

JOJ0 · 2023-03-07T11:25:06Z

One more thing. We should consider how the help looks like for a final decision. I tried to put all the behaviour possibilities into it. Hope it's easily understood:

    --user-type TEXT              Change the type of the user. Currently
                                  understood by the Admin API are 'bot' and
                                  'support'. Use 'regular' to create a regular
                                  Matrix user (which effectively sets the
                                  user-type to 'null'). If the --user-type
                                  option is omitted when modifying an existing
                                  user, the user-type will not be manipulated.
                                  If the --user-type option is omitted when
                                  creating a new user, a regular user will be
                                  created.

JacksonChen666

@JOJ0 synadm/api.py Line 560 is incorrect. Check my suggestion that fixes, or pull from my branch (git pull https://github.com/JacksonChen666/synadm user-type-edit).

(And no, I didn't mess my suggestion up, I checked.)

After applying my change correctly (either using git pull https://github.com/JacksonChen666/synadm user-type-edit or using GitHub to commit the suggestion), you can assume an approval from me.

synadm/api.py

Co-authored-by: Jackson Chen <jackson@jacksonchen666.com>

JOJ0 · 2023-03-09T09:29:03Z

Whoops, thanks for spotting this embarrassing mistake! :-)

JacksonChen666

(this is a blind-ish approval. I have already reviewed with my fix and my fix has been applied, so it's not so really a blind approval. but it is kind of blind, because i didn't test the changes after the fix was applied)

Make it possible to change user type

bbeee54

Closes JOJ0#89

JOJ0 requested changes Feb 3, 2023

View reviewed changes

JacksonChen666 linked an issue Feb 14, 2023 that may be closed by this pull request

Make it possible to change user type #89

Closed

Prevent resetting user_type on other changes

e30035a

Leave out user_type in the post data entirely when user_type is not passed from the cli, otherwise it would be reset to 'null', which is unexpected behaviour. According to the admin API docs the json text for user_type is optional.

JOJ0 approved these changes Feb 22, 2023

View reviewed changes

JacksonChen666 self-requested a review February 22, 2023 10:20

JacksonChen666 requested changes Feb 23, 2023

View reviewed changes

synadm/cli/user.py Outdated Show resolved Hide resolved

Update synadm/cli/user.py

ab3bcd4

Co-authored-by: Jackson Chen <jackson@jacksonchen666.com>

JacksonChen666 self-requested a review February 27, 2023 10:40

JacksonChen666 approved these changes Mar 4, 2023

View reviewed changes

Change handling of "setting user-type to null"

dba8132

Remove --clear-user-type option from user modify command and support an argument named 'regular' that submits a value of 'null' which effectively creates a default/regular matrix user.

JOJ0 force-pushed the user-type-edit branch from 0ffdbd7 to dba8132 Compare March 7, 2023 08:32

JacksonChen666 requested changes Mar 7, 2023

View reviewed changes

synadm/cli/user.py Show resolved Hide resolved

JacksonChen666 mentioned this pull request Mar 7, 2023

User modify API requires a redesign #100

Open

Workaround poorly designed user_modify api method

891944a

by simply passing a string. Otherwise setting the user_type to None (which means value 'null' in submitted json) would not be possible.

JOJ0 requested a review from JacksonChen666 March 7, 2023 11:05

JacksonChen666 reviewed Mar 7, 2023

View reviewed changes

synadm/api.py Outdated Show resolved Hide resolved

Refactor a condition in user_modify method to

72b01bb

a oneline-condition.

JOJ0 requested a review from JacksonChen666 March 7, 2023 11:22

JOJ0 self-assigned this Mar 8, 2023

JacksonChen666 requested changes Mar 9, 2023

View reviewed changes

synadm/api.py Outdated Show resolved Hide resolved

Update synadm/api.py

188024c

Co-authored-by: Jackson Chen <jackson@jacksonchen666.com>

JacksonChen666 approved these changes Mar 9, 2023

View reviewed changes

JOJ0 merged commit 4542ef7 into JOJ0:master Mar 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make it possible to change user type #90

Make it possible to change user type #90

gergelypolonkai commented Feb 3, 2023

JOJ0 left a comment

JOJ0 Feb 3, 2023

gergelypolonkai Feb 3, 2023

JOJ0 Feb 3, 2023

gergelypolonkai Feb 3, 2023

gergelypolonkai Feb 4, 2023

JOJ0 Feb 6, 2023 •

edited

Loading

JOJ0 Feb 22, 2023 •

edited

Loading

JOJ0 Feb 22, 2023 •

edited

Loading

gergelypolonkai commented Feb 3, 2023

JOJ0 commented Feb 22, 2023 •

edited

Loading

JacksonChen666 commented Feb 23, 2023

JOJ0 commented Feb 26, 2023

JOJ0 commented Feb 26, 2023 •

edited

Loading

JOJ0 commented Feb 26, 2023

JacksonChen666 left a comment

JOJ0 commented Mar 4, 2023

JacksonChen666 commented Mar 4, 2023

JacksonChen666 left a comment

JacksonChen666 commented Mar 7, 2023

JOJ0 commented Mar 7, 2023

JOJ0 commented Mar 7, 2023 •

edited

Loading

JOJ0 commented Mar 7, 2023 •

edited

Loading

JOJ0 commented Mar 7, 2023 •

edited

Loading

JOJ0 commented Mar 7, 2023

JacksonChen666 left a comment

JOJ0 commented Mar 9, 2023

JacksonChen666 left a comment •

edited

Loading

Make it possible to change user type #90

Make it possible to change user type #90

Conversation

gergelypolonkai commented Feb 3, 2023

JOJ0 left a comment

Choose a reason for hiding this comment

JOJ0 Feb 3, 2023

Choose a reason for hiding this comment

gergelypolonkai Feb 3, 2023

Choose a reason for hiding this comment

JOJ0 Feb 3, 2023

Choose a reason for hiding this comment

gergelypolonkai Feb 3, 2023

Choose a reason for hiding this comment

gergelypolonkai Feb 4, 2023

Choose a reason for hiding this comment

JOJ0 Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

JOJ0 Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

JOJ0 Feb 22, 2023 • edited Loading

Choose a reason for hiding this comment

gergelypolonkai commented Feb 3, 2023

JOJ0 commented Feb 22, 2023 • edited Loading

JacksonChen666 commented Feb 23, 2023

JOJ0 commented Feb 26, 2023

JOJ0 commented Feb 26, 2023 • edited Loading

JOJ0 commented Feb 26, 2023

JacksonChen666 left a comment

Choose a reason for hiding this comment

JOJ0 commented Mar 4, 2023

JacksonChen666 commented Mar 4, 2023

JacksonChen666 left a comment

Choose a reason for hiding this comment

JacksonChen666 commented Mar 7, 2023

JOJ0 commented Mar 7, 2023

JOJ0 commented Mar 7, 2023 • edited Loading

JOJ0 commented Mar 7, 2023 • edited Loading

JOJ0 commented Mar 7, 2023 • edited Loading

JOJ0 commented Mar 7, 2023

JacksonChen666 left a comment

Choose a reason for hiding this comment

JOJ0 commented Mar 9, 2023

JacksonChen666 left a comment • edited Loading

Choose a reason for hiding this comment

JOJ0 Feb 6, 2023 •

edited

Loading

JOJ0 Feb 22, 2023 •

edited

Loading

JOJ0 Feb 22, 2023 •

edited

Loading

JOJ0 commented Feb 22, 2023 •

edited

Loading

JOJ0 commented Feb 26, 2023 •

edited

Loading

JOJ0 commented Mar 7, 2023 •

edited

Loading

JOJ0 commented Mar 7, 2023 •

edited

Loading

JOJ0 commented Mar 7, 2023 •

edited

Loading

JacksonChen666 left a comment •

edited

Loading