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

Add ability to call custom GET/POST command via synadm script #11

Closed
MurzNN opened this issue Jan 13, 2021 · 21 comments · Fixed by #129
Closed

Add ability to call custom GET/POST command via synadm script #11

MurzNN opened this issue Jan 13, 2021 · 21 comments · Fixed by #129
Labels
feature Feature request, not a core feature

Comments

@MurzNN
Copy link

MurzNN commented Jan 13, 2021

Will be good to use same synadm tool not only for pre-defined API calls, but for custom too, for not to create custom scripts for each call.
For example, allow something like this:

synadm custom-api-call --type POST --path "/_synapse/admin/v1/media/ru-matrix.org/delete?before_ts=1602745639000" --body "{}"

where make default argument value for type as GET, body = {}.

This way will be much more comfortable, than composing full curl command in cli manually and remember your token each time.

Especially this is useful at now, when some of popular API calls are not implemented in build-in commands.

@MurzNN
Copy link
Author

MurzNN commented Jan 13, 2021

Also will be good to show response text in prettified JSON, instead of plaintext like in raw curl command.

@JOJ0
Copy link
Owner

JOJ0 commented Jan 14, 2021

Did you check the latest upstream version (master branch)? Have you seen the --output/-o option? Actually pretty printed json is supported since day one :-) It was called --json and now is called --output pprint.
Just issue synadm config and you can configure your prefererred default output format.

@JOJ0
Copy link
Owner

JOJ0 commented Jan 14, 2021

Will be good to use same synadm tool not only for pre-defined API calls, but for custom too, for not to create custom scripts for each call.
For example, allow something like this:

synadm custom-api-call --type POST --path "/_synapse/admin/v1/media/ru-matrix.org/delete?before_ts=1602745639000" --body "{}"

where make default argument value for type as GET, body = {}.

This way will be much more comfortable, than composing full curl command in cli manually and remember your token each time.

Especially this is useful at now, when some of popular API calls are not implemented in build-in commands.

As for this suggestion: Yes nice idea and I will think about it but actually what do you think about my preferred idea: You seem to have a specific idea of what admin API calls you are missing. It is pretty easy to implement a missing one. So please file a pull-request with the one you are missing most and I will happily try to assist with reviewing your code :-)

The goal of synadm is to at some point support everything the Synapse admin APIs offer. So implementing one available API after the other is the first priority. Any help with that is greatly appreciated!

The second priority is supporting regular matrix calls too. One of the next features will be to let the admin user login by password prompt or token environment variable, so if a user prefers it that way, the token has not to be saved in the configuration. User login is a regular matrix client API and to support it there needs to be some foundational work done first.

At the moment top priority is getting the latest bugs fixed that were introduced by latest PR. So please go ahead and try out the latest version and open issues when you find anything that fails or is not working as before! That would help a lot! Thanks and keep those suggestions and issue reports coming! Appreciated :-)

@amstan
Copy link

amstan commented Feb 5, 2021

+1 on this. I was chatting about this earlier on #synapse:matrix.org as well.

I propose raw for the subcommand name, it's shorter. Instead of custom-api-call

It would be a great stopgap when the official cli ui is not implemented for some new feature.

Bonus points if you can make the matrix api documentation writers to adopt the tool instead of curl (though now i can't find examples of curl usage in any such documentation).

@JOJ0
Copy link
Owner

JOJ0 commented Feb 5, 2021

I will think about it but it seems to be more work and thinking compared to supporting a new regular admin API, which as already mentioned, currently should be my main priority. If any of you would send a PR draft we could work on it together. That would be awesome! :-)

@JOJ0
Copy link
Owner

JOJ0 commented Feb 6, 2021

+1 on this. I was chatting about this earlier on #synapse:matrix.org as well.

I propose raw for the subcommand name, it's shorter. Instead of custom-api-call

It would be a great stopgap when the official cli ui is not implemented for some new feature.

Bonus points if you can make the matrix api documentation writers to adopt the tool instead of curl (though now i can't find examples of curl usage in any such documentation).

@amstan I like synadm raw as the option name but somehow we'd need to distinguish between "admin API" command and "matrix client API" command.

The first part of the URI is in the configuration already, currently only for the admin api but as mentioned synadm will make use of the regular matrix client api in the future, so it will be a configuration item and should be used. See here, soon there will be an option called "Matrix Client path" or similar:

$ synadm config
Running configurator...
Synapse admin user name [jo]:
Synapse admin user token [asdf]:
Synapse base URL [http://localhost:8008]:
Synapse admin API path [/_synapse/admin]:
Default output format (yaml, json, human, pprint) [yaml]:
Default http timeout [7]:
Restricting access to config file to user only

You have a good suggestion how the command could be constructed? My only idea would be something like:

synadm raw 'admin_api_url'

synadm raw -m 'matrix_client_api_url'

or better make dedicated commands? (but that's kind of even more ugly ;-))

synadm raw-admin 'matrix_client_api_url'

synadm raw-matrix 'matrix_client_api_url'

As mentioned already: I'd welcome a PR implementing this but won't find the time any time soon to code it myself. Thanks in advance if you'd really consider to submit it.

@JOJ0 JOJ0 added feature Feature request, not a core feature help wanted Extra attention is needed labels Mar 4, 2021
@JOJ0
Copy link
Owner

JOJ0 commented Apr 10, 2021

@amstan still waiting for your pr draft 😅

@JOJ0
Copy link
Owner

JOJ0 commented Jun 21, 2021

Part of this has been implemented in branch do_as_user - regular Matrix API calls can be executed using synadm

Note that this is a convenience feature for admins trying to help users and not trying to replace existing Matrix API CLI tools! Since I recently also implemented a command for "user token fetching API": synadm user login (also in do_as_user branch), this felt useful to implement.

Data can be read in from file or stdin:

synadm matrix raw 'client/r0/user_directory/search' -m post -f search.json
cat search.json | synadm matrix raw 'client/r0/user_directory/search' -m post -f -

or from CLI directly:

synadm matrix raw 'client/r0/user_directory/search' -d '{"search_term": "username", "limit":100}' -m post

A Matrix token can be read in interactivly:

synadm matrix raw 'client/r0/user_directory/search' -m post -f search.json -p

Set on CLI:

synadm matrix raw 'client/r0/user_directory/search' -m post -f search.json -t xxxunsecurexxx

Or even read from an environment variable:

read MTOKEN
synadm matrix raw 'client/r0/user_directory/search' -m post -f search.json

The current help of the new command should describe all the possibilities:

jojo@jum ~/git/synadm (do_as_user) $ synadm matrix raw -h
Usage: synadm matrix raw [OPTIONS] ENDPOINT

  Execute a raw request to the Matrix API.

  The endpoint argument is the part of the URL _after_ the configured base
  URL and Matrix path (see `synadm config`). A simple get request would e.g
  like this: `synadm matrix raw client/versions`

  Use either --token or --prompt to provide a user's token and execute
  Matrix commands on their behalf. Respect the privacy of others! Be
  responsible!

  The precedence rules for token reading are: 1. Interactive input using
  --prompt; 2. Set on CLI via --token string; 3. Read from environment
  variable $MTOKEN; 4. Preconfigured admin token set in synadm's config
  file.

Options:
  -m, --method [get|post|put|delete]
                                  The HTTP method used for the request.
                                  [default: get]

  Data input: [mutually_exclusive]
    -d, --data TEXT               The JSON string sent in the body of post,
                                  put and delete requests - provided as a
                                  string. Make sure to escape it from shell
                                  interpretation by using single quotes. E.g
                                  '{"key1": "value1", "key2": 123}'  [default:
                                  {}]

    -f, --data-file FILENAME      Read JSON data from file. To read from stdin
                                  use "-" as the filename argument.

  Matrix token: [mutually_exclusive]
    -t, --token TEXT              Token used for Matrix authentication instead
                                  of the configured admin user's token. If
                                  --token (and --prompt) option is missing,
                                  the token is read from environment variable
                                  $MTOKEN instead. To make sure a user's token
                                  does not show up in system logs, don't
                                  provided it on the shell directly but set
                                  $MTOKEN by using shell command `read
                                  MTOKEN`.

    -p, --prompt                  Prompt for the token used for Matrix
                                  authentication. This option always overrides
                                  $MTOKEN.  [default: False]

  -h, --help                      Show this message and exit.
jojo@jum ~/git/synadm (do_as_user) $

@amstan and @MurzNN: Please help me get this feature stable and do some testing and playing around with it. Also review of the help text's english and clarity of descriptions is appreciated. That would help a lot. Thanks in advance

@JOJ0
Copy link
Owner

JOJ0 commented Jun 24, 2021

Above mentioned has been released in this version: https://github.com/JOJ0/synadm/releases/tag/v0.30

@JOJ0 JOJ0 removed the help wanted Extra attention is needed label Feb 24, 2022
@Ascurius
Copy link
Collaborator

Ascurius commented Apr 7, 2022

I am closing the issue because the requested feature already has been implemented

@Ascurius Ascurius closed this as completed Apr 7, 2022
@JOJ0
Copy link
Owner

JOJ0 commented Apr 7, 2022

Sorry, please reopen. It only is implemented for matrix calls but not for admin api calls 😀

some details pn the initial ideas here: #11 (comment)

@Ascurius Ascurius reopened this Apr 8, 2022
@JacksonChen666
Copy link
Collaborator

JacksonChen666 commented Mar 2, 2023

Bonus points if you can make the matrix api documentation writers to adopt the tool instead of curl (though now i can't find examples of curl usage in any such documentation).

The admin API is not exactly documentation for the end-sysadmins, it's mainly for developers.
The developers are not going to develop their programs for the admin API around the curl program, so the curl commands are unlikely going to further help the developers. The hints of GET, POST, etc. is well enough for developers writing programs and curl users writing commands.


I like synadm raw as the option name but somehow we'd need to distinguish between "admin API" command and "matrix client API" command.

My answer to that is don't touch the "path" part of the URL, it gets annoying just trying to figure out how the code behaves if it touches any variables.

Why? The synadm matrix raw command accepts a URL path, then changes it to /_matrix/ plus whatever the user gave. That makes it awkward to work with because the documentation includes the full URL path already and you're probably expecting the command to accept a full path (which you probably copied as is from the documentation).

You have a good suggestion how the command could be constructed? My only idea would be something like:

synadm raw 'admin_api_url'

synadm raw -m 'matrix_client_api_url'

or better make dedicated commands? (but that's kind of even more ugly ;-))

synadm raw-admin 'matrix_client_api_url'

synadm raw-matrix 'matrix_client_api_url'

To re-iterate over my answer (differently this time), don't distinguish between the Matrix API calls and the synapse admin API calls and don't manipulate the URL. Single command to call any endpoint, that's it. The raw command is for when you know what you're doing (just like using curl).
Even though my idea basically creates a glorified curl --header "Authorization: Bearer <token>", it removes the constraints of what endpoints you can call (instead of only either synapse admin API or Matrix API, you can do anything).

just my two cents.

@JOJ0
Copy link
Owner

JOJ0 commented Mar 16, 2023

To re-iterate over my answer (differently this time), don't distinguish between the Matrix API calls and the synapse admin API calls and don't manipulate the URL. Single command to call any endpoint, that's it. The raw command is for when you know what you're doing (just like using curl). Even though my idea basically creates a glorified curl --header "Authorization: Bearer <token>", it removes the constraints of what endpoints you can call (instead of only either synapse admin API or Matrix API, you can do anything).

I absolutely agree that it doesn't help a synadm user at all to have to copy and paste an endpoint URL and then having to remove _synapse or _matrix again. One command for both seems to be the solution to go for. At the end of the day synadm should be a tool that helps admins, even rather advanced ones that would use such a command. Copy/paste directly from the docs as they state the endpoints would indeed be much more conformtable than how synadm matrix raw works right now!

Some concerns anyway:

  • Currently one of the two things is implemented already as synadm matrix raw which indicates the use of a Matrix API.
  • Logically speaking it would probably be expected to be able to use synadm raw for a regular synadm command which is to the Synapse Admin API
  • If we now combine the command into one, where should it live? Within the main context or the "matrix" subcommands context? I'm not sure. Maybe in both?
  • What I'm also not sure is that if we combine commands, we always send, as you just mentioned, autorization information to the Matrix API which is not of use there.
  • Also note that synadm matrix has some separate handling of what token to be used available:

@optgroup.group(
"Matrix token",
cls=MutuallyExclusiveOptionGroup,
help="")
@optgroup.option(
"--token", "-t", type=str, envvar='MTOKEN', show_default=True,
help="""Token used for Matrix authentication instead of the configured
admin user's token. If --token (and --prompt) option is missing, the token
is read from environment variable $MTOKEN instead. To make sure a user's
token does not show up in system logs, don't provide it on the shell
directly but set $MTOKEN with shell command `read MTOKEN`.""")
@optgroup.option(
"--prompt", "-p", is_flag=True, show_default=True,
help="""Prompt for the token used for Matrix authentication. This option
always overrides $MTOKEN.""")

I didn't think through entirely whether your idea would break this ^ possibility or if it could happily co-exist!!

Appologiez if some thought might occur random, I just wanted to finally answer to this and hopefully find a concept together that works out and doesn't break existing concepts too much. That said, this command still is a very advanced one and we could probably change some concepts without bothering such advanced users too much if we improve it. At least I think that this command probably isn't used in scripts too often, but we can never know....

@JOJ0
Copy link
Owner

JOJ0 commented Mar 16, 2023

I just jecallled that somewhere @Ascurius and me came to the conclusion that --token should move to the main command anyway, to finally have a way of submitting token without safing it on-disk (Reading from shell environment and such things se have implemented here should be included....)

I just can't find anywhere the discussion .... 🤷

Update: Found it: #62 (comment), Damnit that is too obvious of a name for that issue LOL

@JacksonChen666
Copy link
Collaborator

JacksonChen666 commented Mar 16, 2023

Re: Tokens

keeping secrets a secret on the shell is... probably unexpectedly difficult. There's shell history and process arguments, and to hide those without using standard input to accept tokens is quite complicated.

Look at this: https://unix.stackexchange.com/questions/439497/is-there-a-way-to-pass-sensitive-data-in-bash-using-a-prompt-for-any-command
(thorough reading is highly recommended, including comments)

So, when implementing one-off token usage, there really should be a way to accept standard input as a method to pass tokens (and also don't echo the token). Just something to remember when implementing.

@JacksonChen666
Copy link
Collaborator

Some concerns anyway:

  • Logically speaking it would probably be expected to be able to use synadm raw for a regular synadm command which is to the Synapse Admin API

?

  • If we now combine the command into one, where should it live? Within the main context or the "matrix" subcommands context? I'm not sure. Maybe in both?

root context since you're able to do anything (matrix or synapse admin) with it.

  • What I'm also not sure is that if we combine commands, we always send, as you just mentioned, autorization information to the Matrix API which is not of use there.

i'm not sure if I understand, but then I came up with another concern: what if the API the user is calling disallows authorization? I guess just a flag to not give the authorization header should be enough.

@JOJ0
Copy link
Owner

JOJ0 commented Mar 17, 2023

Re: Tokens

keeping secrets a secret on the shell is... probably unexpectedly difficult. There's shell history and process arguments, and to hide those without using standard input to accept tokens is quite complicated.

Look at this: https://unix.stackexchange.com/questions/439497/is-there-a-way-to-pass-sensitive-data-in-bash-using-a-prompt-for-any-command (thorough reading is highly recommended, including comments)

Too lazy for reading now but I will once we start a reimplementation. Thanks for the article. We had disussed quite some of these security related topics in #synadm back then before I implemented matrix raw

So, when implementing one-off token usage, there really should be a way to accept standard input as a method to pass tokens (and also don't echo the token). Just something to remember when implementing.

Exactly, we should provide that!

We have that for matrix raw already :-) https://synadm.readthedocs.io/en/latest/synadm.cli.matrix.html#cmdoption-synadm-matrix-raw-p

We have a way of interactive prompt as well as using a shell env var called MTOKEN, the latter probably being not a good idea to use. We can only provide the tools and if we are kind provide a word of warning that using shell variables with secrets is tricky.

@JOJ0
Copy link
Owner

JOJ0 commented Mar 17, 2023

Some concerns anyway:

  • Logically speaking it would probably be expected to be able to use synadm raw for a regular synadm command which is to the Synapse Admin API

?

  • If we now combine the command into one, where should it live? Within the main context or the "matrix" subcommands context? I'm not sure. Maybe in both?

root context since you're able to do anything (matrix or synapse admin) with it.

Sorry for speking complicated ;-) Yes I actually ment: An existing user that knows about synadm matrix raw would now expect synadm raw to be a regular Synapse Admin API call.

The most logic way would be to move this raw command to the root context. Yes, but could break workflows or scripts of users...

  • What I'm also not sure is that if we combine commands, we always send, as you just mentioned, autorization information to the Matrix API which is not of use there.

i'm not sure if I understand, but then I came up with another concern: what if the API the user is calling disallows authorization? I guess just a flag to not give the authorization header should be enough.

Maybe my statement is bollocks but I think I was thinking something like this: Sometimes I would send a call to a Matrix API where I don't want to send my admin token, but now that I read the docs of matrix raw again I learned that actually I can freely choose which token to send....

but your idea is also a thing/feature we should provide: not sending a token at all! could be useful.

@JacksonChen666
Copy link
Collaborator

synadm custom-api-call --type POST --path "/_synapse/admin/v1/media/ru-matrix.org/delete?before_ts=1602745639000" --body "{}"

This way will be much more comfortable, than composing full curl command in cli manually and remember your token each time.

Actually, I don't think it's that cumbersome to use curl. Here's the equivalent command with curl:

curl --header "Authorization: Bearer $SECRET" 'http://localhost:8080/_synapse/admin/v1/media/ru-matrix.org/delete?before_ts=1602745639000' -XPOST -d '{}'

The differences between these commands are:

And that's it. Pretty much everything else including the request type, request body and URL path is the same. Practically no other differences there.

What if some user demands more curl like options in synadm? Because then, we're just gonna start re-implementing curl. The benefits (tiny, microscopic when compared) don't outweigh the cost (curl has hundreds of options, been developed over decades, and has a man page that's 6229 lines long).

This suggestion now feels quite out of scope to me, because synadm hits synapse admin APIs. It probably shouldn't be the next curl re-implementation, or a HTTP client tool. synadm isn't that, nor should it be that.

@JOJ0
Copy link
Owner

JOJ0 commented Sep 4, 2023

synadm should be a convenient helper to a Synapse admin.

Actually I personally dont care too much if we have this featurr or not. I still think that using that curl command you provide here is supertedious and havin synadm do it, could be just a little easier

I suggest we ask the original submitter whether they're still interested in the feature. @MurzNN what do you say?

@JOJ0
Copy link
Owner

JOJ0 commented Oct 2, 2023

I still think it's very handy and I'd like to have it. I just started an implementation. Also I'll use that opportunity to adress a problem we often faced already: Sharing common things between commands.

@JOJ0 JOJ0 closed this as completed in #129 Oct 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature request, not a core feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants