Add support for registration token management #42

govynnus · 2021-08-21T11:20:54Z

This adds support for managing registration tokens using the admin API introduced by matrix-org/synapse#10142 which implements MSC3231 - Token Authenticated Registration.

That synapse PR has been merged but isn't part of a release yet, so you probably won't want this until it is.
I'm happy to change names of commands/options/anything if you want.

JOJ0 · 2021-08-26T10:26:31Z

Hi @govynnus, thank you so much for the submission. Probably the most complete PR ever submitted to this repo ;-)) Thanks for adding docstrings and even extending the Sphinx .rst files! Great!

Overall this looks coded very thorougly but before I can give you feedback I should read a little about what exactly this new Synapse feature does. The "latest docs" link does not work yet (https://matrix-org.github.io/synapse/latest/usage/administration/admin_api/registration_tokens.html). You have a dev-version link of the docs for me, that's up and readably already? Thanks!

govynnus · 2021-08-27T12:46:55Z

Hi @JOJ0 :-)
Ah yes, the latest link won't work until the code becomes part of a release, but https://matrix-org.github.io/synapse/develop/usage/administration/admin_api/registration_tokens.html should work. Shall I change it to the develop link?

I did mean to add a convenient date format, but forgot. Handy that you already have some examples for me to have a look at :)
I'll probably have a look at that next week now.

JOJ0 · 2021-08-27T14:22:42Z

no leave it as the final/released docs link. so we don't have to change it later. I think that's fine. Just wanted to find it to read a little about it myself :-)

perfect regarding date format convenience. we are not in a hurry with getting this merged, all good.
thanks again! appreciate your efforts!

anoadragon453 · 2021-09-06T17:18:52Z

(https://matrix-org.github.io/synapse/develop/usage/administration/admin_api/registration_tokens.html now resolves by the way :)

govynnus · 2021-09-08T10:26:40Z

@JOJ0 It would be nice to display the expiry time in a human friendly format too (in list and details), but I'm not sure whether to do that in api.py or cli/regtok.py. I think it would just be adding another field to each dictionary.

JOJ0 · 2021-09-12T18:27:09Z

Thanks for your patience. Been busy with music projects and work lately.

I think it would just be adding another field to each dictionary.

You mean adding a new field in the results dict we get from the api response, ie additionally to the "expiry_time" field (eg as in the result shown here: https://matrix-org.github.io/synapse/develop/usage/administration/admin_api/registration_tokens.html#list-all-tokens)?

Another option would be to just replace the value in expire_time with a human readable format. But in that case it should be configurable via another --option. Not sure if that's a good idea though. What do you think?

And regarding wheather to put it in api.py or cli frontend code, I think since the helper methods are in api.py and possibly api.py could be used standalone theoretically I feel it's ok that the api method can be asked te return human readable or unix timestamp.

govynnus · 2021-09-12T18:41:54Z

Ooh, music projects :-)

You mean adding a new field in the results dict we get from the api response, ie additionally to the "expiry_time" field

Yeah. I think the timestamp format should at least be available of it is wanted. Maybe by default show human readable only and have an option that shows the timestamp instead? Because in normal use the timestamp will probably just be clutter.

And regarding wheather to put it in api.py or cli frontend code, I think since the helper methods are in api.py and possibly api.py could be used standalone theoretically I feel it's ok that the api method can be asked te return human readable or unix timestamp.

Ok, thanks.

JOJ0 · 2021-09-13T05:26:45Z

You mean adding a new field in the results dict we get from the api response, ie additionally to the "expiry_time" field

Yeah. I think the timestamp format should at least be available of it is wanted. Maybe by default show human readable only and have an option that shows the timestamp instead? Because in normal use the timestamp will probably just be clutter.

So you do mean "replacing" the value of expiry_time and not "adding a new additional field" ("expiry_date" or something)? And then have an option switch to control weather the value is a timestamp or an actual date? Did I understand correctly?

govynnus · 2021-09-13T07:17:23Z

Well to start with I did mean adding a new additional field, but now I think it might be better to replace it and have an option switch.

govynnus · 2021-09-13T10:55:32Z

Does f0b6e00 look alright?

Registration token support has been released in Synapse 1.42.0, so the link to the docs works now.

JOJ0 · 2021-09-13T16:38:25Z

Hi, thanks for letting me know that the feature is released now. Upgraded my Synapse and did some testing.
There is some issues with the datetime conversion. Have a look, yaml and human output are fine, in pprint it is literally showing the datetime object (the repr I guess?):

(synadm) jojo@jum ~/git/govynnus_synadm (regtok) $ synadm -o yaml regtok list
registration_tokens:
- completed: 0
  expiry_time: 2021-09-30 00:00:00
  pending: 0
  token: lqvrdFcFXDTcQk3G
  uses_allowed: null

(synadm) jojo@jum ~/git/govynnus_synadm (regtok) $ synadm -o pprint regtok list
{'registration_tokens': [{'completed': 0,
                          'expiry_time': datetime.datetime(2021, 9, 30, 0, 0),
                          'pending': 0,
                          'token': 'lqvrdFcFXDTcQk3G',
                          'uses_allowed': None}]}
(synadm) jojo@jum ~/git/govynnus_synadm (regtok) $
(synadm) jojo@jum ~/git/govynnus_synadm (regtok) $ synadm -o human regtok list
token             uses_allowed      pending    completed  expiry_time
----------------  --------------  ---------  -----------  -------------------
lqvrdFcFXDTcQk3G                          0            0  2021-09-30 00:00:00
(synadm) jojo@jum ~/git/govynnus_synadm (regtok) $

And json output fails because the output renderer can't handle it:

(synadm) jojo@jum ~/git/govynnus_synadm (regtok) $ synadm -o json regtok list
Traceback (most recent call last):
  File "/home/jojo/.venvs/synadm/bin/synadm", line 11, in <module>
    load_entry_point('synadm', 'console_scripts', 'synadm')()
  File "/home/jojo/.venvs/synadm/lib/python3.6/site-packages/click-7.1.2-py3.6.egg/click/core.py", line 829, in __call__
    return self.main(*args, **kwargs)
  File "/home/jojo/.venvs/synadm/lib/python3.6/site-packages/click-7.1.2-py3.6.egg/click/core.py", line 782, in main
    rv = self.invoke(ctx)
  File "/home/jojo/.venvs/synadm/lib/python3.6/site-packages/click-7.1.2-py3.6.egg/click/core.py", line 1259, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/jojo/.venvs/synadm/lib/python3.6/site-packages/click-7.1.2-py3.6.egg/click/core.py", line 1259, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/jojo/.venvs/synadm/lib/python3.6/site-packages/click-7.1.2-py3.6.egg/click/core.py", line 1066, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/jojo/.venvs/synadm/lib/python3.6/site-packages/click-7.1.2-py3.6.egg/click/core.py", line 610, in invoke
    return callback(*args, **kwargs)
  File "/home/jojo/.venvs/synadm/lib/python3.6/site-packages/click-7.1.2-py3.6.egg/click/decorators.py", line 33, in new_func
    return f(get_current_context().obj, *args, **kwargs)
  File "/home/jojo/git/govynnus_synadm/synadm/cli/regtok.py", line 59, in regtok_list_cmd
    helper.output(regtoks)
  File "/home/jojo/git/govynnus_synadm/synadm/cli/__init__.py", line 176, in output
    click.echo(self.formatter(data))
  File "/usr/lib/python3.6/json/__init__.py", line 231, in dumps
    return _default_encoder.encode(obj)
  File "/usr/lib/python3.6/json/encoder.py", line 199, in encode
    chunks = self.iterencode(o, _one_shot=True)
  File "/usr/lib/python3.6/json/encoder.py", line 257, in iterencode
    return _iterencode(o, 0)
  File "/usr/lib/python3.6/json/encoder.py", line 180, in default
    o.__class__.__name__)
TypeError: Object of type 'datetime' is not JSON serializable
(synadm) jojo@jum ~/git/govynnus_synadm (regtok) $

Not sure what's the best way to fix those issues. Let's think about it...

json and pprint output formats don't automatically convert a python datetime object to a string.

govynnus · 2021-09-13T16:59:34Z

Ah sorry, I only tested with the human output format. Does converting to a string manually as in 323c33d look alright?

synadm/cli/regtok.py

JOJ0 · 2021-09-15T05:44:56Z

Ah sorry, I only tested with the human output format. Does converting to a string manually as in 323c33d look alright?

No worries and yes perfect, that probably is the best solution, have a string right away. Also thanks for renaming the variable to readable_expiry, I think that simple change adds to code readability a lot! :-)

Otherwise that all looks good and I think we are ready to merge. I found some little things about the show_default flags and posted inline.

Something else: I am wondering if there are places in synadm where I should adapt to your nice idea of having an option to choose for output format "--datetime/--timestamp". I used the timestamp translation stuff in media and quarantine API's a lot, but I think the list commands over there don't spit out timestamps. So, if you come across anywhere else where timestamps are produced, give me a hint and I will also use your idea there to streamline user experience. Thanks!

synadm/cli/regtok.py

govynnus · 2021-09-15T10:10:24Z

I've removed show_default where the default is already hardcoded in the help text. I think I must have been copy/pasting 🙃

If I do see any timestamps being output I'll let you know.

JOJ0 · 2021-09-15T18:10:36Z

Ah one last thing, you used -t option twice in regtok new command ;-) For --expiry-ts and --token.
I suggest you use -n for --token,
since -t is just fitting too well for --expiry-ts.
Or maybe you have another suggestion? Always welcome!

JOJ0 · 2021-09-16T05:21:43Z

Found one more. This error should be catched/prevented. The output mode doesn't matter:

(synadm) jojo@jum ~/git/govynnus_synadm (regtok) $ synadm -o pprint regtok details nonexistenttoken
WARNING Synapse returned status code 404
Traceback (most recent call last):
  File "/home/jojo/.venvs/synadm/bin/synadm", line 11, in <module>
    load_entry_point('synadm', 'console_scripts', 'synadm')()
  File "/home/jojo/.venvs/synadm/lib/python3.6/site-packages/click-7.1.2-py3.6.egg/click/core.py", line 829, in __call__
    return self.main(*args, **kwargs)
  File "/home/jojo/.venvs/synadm/lib/python3.6/site-packages/click-7.1.2-py3.6.egg/click/core.py", line 782, in main
    rv = self.invoke(ctx)
  File "/home/jojo/.venvs/synadm/lib/python3.6/site-packages/click-7.1.2-py3.6.egg/click/core.py", line 1259, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/jojo/.venvs/synadm/lib/python3.6/site-packages/click-7.1.2-py3.6.egg/click/core.py", line 1259, in invoke
    return _process_result(sub_ctx.command.invoke(sub_ctx))
  File "/home/jojo/.venvs/synadm/lib/python3.6/site-packages/click-7.1.2-py3.6.egg/click/core.py", line 1066, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/jojo/.venvs/synadm/lib/python3.6/site-packages/click-7.1.2-py3.6.egg/click/core.py", line 610, in invoke
    return callback(*args, **kwargs)
  File "/home/jojo/.venvs/synadm/lib/python3.6/site-packages/click-7.1.2-py3.6.egg/click/decorators.py", line 33, in new_func
    return f(get_current_context().obj, *args, **kwargs)
  File "/home/jojo/git/govynnus_synadm/synadm/cli/regtok.py", line 72, in regtok_details_cmd
    regtok = helper.api.regtok_details(token, datetime)
  File "/home/jojo/git/govynnus_synadm/synadm/api.py", line 705, in regtok_details
    if result["expiry_time"] is not None:
KeyError: 'expiry_time'
(synadm) jojo@jum ~/git/govynnus_synadm (regtok) $

I guess you could quickly fix that KeyError with a result.get("expiry_time")?

JOJ0 · 2021-09-21T06:11:24Z

Hi, I'll merge this now and quickly fix these little things in the master. Thank you so much for this very complete PR, your thoroughness and good communication! May I invite you to #synadm:peek-a-boo.at, we often share opinions about new features and how they could/should be designed. I am sure your presence and thoughts would add some to the community. All the best. Jojo

as mentioned in PR #42

JOJ0 · 2021-09-21T07:40:08Z

Oh, how nice, I spotted your summary page about the feature. Great contribution to Matrix in general. Thanks for that!!! https://calcuode.com/matrix-gsoc/2021-08-22_final-report.html
You could mention synadm as supporting it entirely now if you want. Thanks again for the PR!

govynnus · 2021-09-21T09:39:13Z

Hi, sorry I didn't get round to this, been busy moving to university the past ~week. I am planning to write another blog post now that the synapse part is released and synadm support exists :-)

Thanks for your help with this and thanks for synadm, probably see you around in #synadm:peek-a-boo.at

Add support for registration token management

c140222

regtok: human-friendly expiry time input

fcd2f1d

regtok: Display expiry time as datetime by default

f0b6e00

regtok: Convert datetime to string for output

323c33d

json and pprint output formats don't automatically convert a python datetime object to a string.