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

Admin api implementation #247

Closed
wants to merge 2 commits into from
Closed

Conversation

slipeer
Copy link

@slipeer slipeer commented Jul 12, 2018

Implemented admin api calls:

  • purge_hostory
  • purge_hostory_status
  • media_in_room
  • whois
  • deactivate
  • reset_password
  • quarantine_media
  • shutdown_room

Based on https://github.com/matrix-org/synapse/tree/master/docs/admin_api docs
And synapse sources: https://github.com/matrix-org/synapse/blob/master/synapse/rest/client/v1/admin.py

@slipeer
Copy link
Author

slipeer commented Jul 12, 2018

Travis fails due to #246

@krombel
Copy link

krombel commented Jul 12, 2018

Just to note some feedback from #synapse-dev:matrix.org here:
@richvdh here:

I wouldn't want that to be in the matrix-python-sdk
since they are synapse-specific

@ara4n here

how much of the admin API in synapse is meant to make it into the spec?

i've kinda assumed that we'd eventually formalise the generic ones into the spec, but for now they're certainly meant to be impl-specific. but the python-sdk could always load them in in a module of some kind

@Half-Shot
Copy link
Collaborator

Agreed with @krombel here. They are however quite useful so perhaps a module like matrix_client.synapse where we just have a client.py that inherits the existing client class. Probably would also need an api.py that does that same thing.

@non-Jedi
Copy link
Collaborator

non-Jedi commented Jul 13, 2018

The problem is that extending functionality by inheriting from classes pushes
the software in a much less modular direction. For example, if I or someone else
ever gets around to writing an async version of MatrixClient (by subclassing
MatrixClient), a subclass implementing the synpase admin api wouldn't benefit
at all from that effort.

I definitely think this stuff is useful to have in the python sdk (and we won't
hesitate to add admin features for other HS as they become workable), but it
needs to be separated from the base spec matrix stuff somehow (at both
MatrixHttpApi level and MatrixClient level).

There are architectures that would make this trivial (and one in particular I'd
really like to move the sdk towards if I ever get a bit of time), but the
current architecture isn't one of them.

As a temporary solution to move us towards architectures that are easily
extendable for adding things like admin apis or other proprietary extensions to
the matrix protocol, how does the following sound?

  • synapse module where all the synapse-specific code lives
  • new AbstractMatrixApi class that MatrixHttpApi inherits from.
  • HTTP methods for synapse admin api in subclass of AbstractMatrixApi within
    synapse module.
  • New kwarg on MatrixClient: admin=None
    • If admin="synapse", we instantiate both an instance of MatrixHttpApi and
      the admin api class.
    • If higher-level abstractions are ever written for the admin api methods,
      they would live as subclasses of Room and User within the synapse module.

Since this change doesn't currently touch MatrixClient, this is what I would
propose as the minimal acceptable change (following model laid out above).

  • Create new api classes such that:
    MatrixHttpApi <: AbstractHttpApi <: AbstractMatrixApi
    • In this model, the _send method would get moved to AbstractHttpApi while
      the rest of MatrixHttpApi is unchanged
  • Subtype AbstractHttpApi with new admin api methods as SynapseApi or
    something, and put that code in a new synapse namespace.

Everyone cool with this?

@slipeer
Copy link
Author

slipeer commented Jul 13, 2018

@non-Jedi ok. Next week i will move admin api to subclass.
but now the dawn in the mountains awaits me ;)

@slipeer slipeer force-pushed the admin_api branch 2 times, most recently from a1b1f70 to 342c9e9 Compare July 16, 2018 14:16
Media list admin API implementation
Admin whois api
Admin deactivate api
Admin api: quarantine_media, reset password
Admin api: shutdown_room
@slipeer slipeer closed this Jul 16, 2018
@slipeer slipeer deleted the admin_api branch July 16, 2018 15:28
@non-Jedi
Copy link
Collaborator

I'd still love to have this even if you don't want to do work of putting it in its own module.

@slipeer any chance you'd signoff on this one as well as the others so we can use this code when someone gets some time to make the changes I outlined?

@slipeer
Copy link
Author

slipeer commented Jul 18, 2018

@non-Jedi I'm working on this module (https://github.com/slipeer/matrix-python-sdk/tree/admin_api). Need a little more hours in a day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants