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.

Sign up for GitHub

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

Jump to bottom

Make it possible to change user type #90

Merged
merged 7 commits into from
Mar 9, 2023
Merged

Make it possible to change user type #90

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bbeee54
Make it possible to change user type
gergelypolonkai Feb 3, 2023
e30035a
Prevent resetting user_type on other changes
JOJ0 Feb 22, 2023
ab3bcd4
Update synadm/cli/user.py
JOJ0 Feb 26, 2023
dba8132
Change handling of "setting user-type to null"
JOJ0 Mar 7, 2023
891944a
Workaround poorly designed user_modify api method
JOJ0 Mar 7, 2023
72b01bb
Refactor a condition in user_modify method to
JOJ0 Mar 7, 2023
188024c
Update synadm/api.py
JOJ0 Mar 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion synadm/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,7 @@ def user_login(self, user_id, expire_days, expire, _expire_ts):
return self.query("post", f"v1/users/{user_id}/login", data=data)

def user_modify(self, user_id, password, display_name, threepid,
avatar_url, admin, deactivation):
avatar_url, admin, deactivation, user_type):
""" Create or update information about a given user

Threepid should be passed as a tuple in a tuple
Expand All @@ -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})
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

@JOJ0 JOJ0 Feb 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Owner

@JOJ0 JOJ0 Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Owner

@JOJ0 JOJ0 Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

return self.query("put", f"v2/users/{user_id}", data=data)

def user_whois(self, user_id):
Expand Down
20 changes: 18 additions & 2 deletions synadm/cli/user.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,10 +358,18 @@ def name_extra(self):
removes their active access tokens, resets their password, kicks them out
of all rooms and deletes third-party identifiers (to prevent the user
requesting a password reset). See also "user deactivate" command.""")
@optgroup.option(
"--user-type", "user_type", type=str,
help="""Change the type of the user. Currently allowed values are 'bot'
and support.""")
JOJ0 marked this conversation as resolved.
Show resolved Hide resolved
@optgroup.option(
"--clear-user-type", "clear_user_type", is_flag=True,
help="""Clear the user_type field of the user.""")
@click.pass_obj
@click.pass_context
def modify(ctx, helper, user_id, password, password_prompt, display_name,
threepid, avatar_url, admin, deactivation):
threepid, avatar_url, admin, deactivation, user_type,
clear_user_type):
""" Create or modify a local user. Provide matrix user ID (@user:server)
as argument.
"""
Expand All @@ -374,6 +382,10 @@ def modify(ctx, helper, user_id, password, password_prompt, display_name,
"Deactivating a user and setting a password doesn't make sense.")
raise SystemExit(1)

if clear_user_type and user_type:
click.echo("Use either --user-type or --clear-user-type, not both.")
raise SystemExit(1)

gergelypolonkai marked this conversation as resolved.
Show resolved Hide resolved
mxid = helper.generate_mxid(user_id)
click.echo("Current user account settings:")
ctx.invoke(user_details_cmd, user_id=mxid)
Expand All @@ -390,6 +402,9 @@ def modify(ctx, helper, user_id, password, password_prompt, display_name,
f"{t_key} is probably not a supported medium "
"type. Threepid medium types according to the "
"current matrix spec are: email, msisdn.")
elif key == "clear_user_type":
if value:
click.echo("user_type: null")
elif value not in [None, {}, []]: # only show non-empty (aka changed)
click.echo(f"{key}: {value}")

Expand All @@ -411,7 +426,8 @@ def modify(ctx, helper, user_id, password, password_prompt, display_name,
if sure:
modified = helper.api.user_modify(
mxid, password, display_name, threepid,
avatar_url, admin, deactivation)
avatar_url, admin, deactivation,
None if clear_user_type else user_type)
gergelypolonkai marked this conversation as resolved.
Show resolved Hide resolved
if modified is None:
click.echo("User could not be modified.")
raise SystemExit(1)
Expand Down