/synapse

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Administration functionalities #1784

Merged
merged 1 commit into from Feb 6, 2017

Conversation

Reviewers

@richvdh richvdh

@erikjohnston erikjohnston

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@morteza-araby @matrixbot @erikjohnston @richvdh @jcgruenhage
@morteza-araby
Contributor

morteza-araby commented Jan 9, 2017

1- Get Request to get list of users for administrator
2- Post request to reset password for users
3 - Get Request to get a paginated users With start and limit.
4 - Search users from database and return an array of found users or empty
@matrixbot

matrixbot commented Jan 9, 2017

Can one of the admins verify this patch?
@matrixbot

matrixbot commented Jan 9, 2017

Can one of the admins verify this patch?
@matrixbot

matrixbot commented Jan 9, 2017

Can one of the admins verify this patch?
@erikjohnston
Owner

erikjohnston commented Jan 19, 2017

@matrixbot ok to test
@erikjohnston
Owner

erikjohnston commented Jan 19, 2017

There's a bunch of PEP8 violations:

synapse/handlers/admin.py:22:1: F401 'operator.attrgetter' imported but unused
synapse/rest/client/v1/admin.py:40:9: F841 local variable 'auth_user' is assigned to but never used
synapse/rest/client/v1/admin.py:57:1: E302 expected 2 blank lines, found 1
synapse/rest/client/v1/admin.py:159:1: E302 expected 2 blank lines, found 1
synapse/rest/client/v1/admin.py:190:1: E302 expected 2 blank lines, found 1
synapse/rest/client/v1/admin.py:204:9: F841 local variable 'auth_user' is assigned to but never used
synapse/rest/client/v1/admin.py:253:1: E302 expected 2 blank lines, found 1
synapse/rest/client/v1/admin.py:290:1: E302 expected 2 blank lines, found 1
synapse/storage/_base.py:931:37: E128 continuation line under-indented for visual indent
synapse/storage/_base.py:951:91: E501 line too long (92 > 90 characters)
synapse/storage/_base.py:953:91: E501 line too long (93 > 90 characters)
synapse/storage/_base.py:973:91: E501 line too long (92 > 90 characters)
synapse/storage/_base.py:1039:1: E303 too many blank lines (3)

You should be able to test locally by installing flake8 and then running flake8 synapse/
@erikjohnston

erikjohnston reviewed Jan 19, 2017
View changes

This broadly looks good. If you can just add some docstring to:

  • the functions in the handler and storage
  • the classes in the rest layer explaining what they do
scripts-dev/add_admin.py
+ url = 'http://localhost:8008/_matrix/client/api/v1/register'
+ post_data = {'user': 'admin',
+ 'password': 'admin123',
+ 'type': 'm.login.password'}
@erikjohnston

erikjohnston Jan 19, 2017

Owner

I'm a bit hesitant to add example scripts like this, as they can't directly be run. I believe the register_new_user script actually has an option to specify the new user is an admin or not.

@morteza-araby

morteza-araby Jan 21, 2017

Contributor

Hi,
Made a new commit.
1- Fix all error from flake8
2- add docstring to all the new methods
3 - Used _simple_select_list_paginate as private method which is more generic to use for other list search as well and used get_user_list_paginate for calling from rest api

synapse/handlers/admin.py
+ def get_users_paginate(self, user, start, limit):
+ ret = yield self.store.get_users_paginate(user, start, limit)
+
+ defer.returnValue(ret)
@erikjohnston

erikjohnston Jan 19, 2017

Owner

These functions can simply be rewritten as:

def get_users_paginate(self, user, start, limit):
    return self.store.get_users_paginate(user, start, limit)
synapse/rest/client/v1/admin.py
+ raise SynapseError(400, "Missing 'limit' arg")
+ if not start:
+ raise SynapseError(400, "Missing 'start' arg")
+ logger.info("limit: %s, start: %s", limit, start)
@erikjohnston

erikjohnston Jan 19, 2017

Owner

This should probably be debug level logging.

synapse/rest/client/v1/admin.py
+ raise SynapseError(400, "Missing 'limit' arg")
+ if not start:
+ raise SynapseError(400, "Missing 'start' arg")
+ logger.info("limit: %s, start: %s", limit, start)
@erikjohnston

erikjohnston Jan 19, 2017

Owner

This should probably be debug level logging.

synapse/rest/client/v1/admin.py
+ if not term:
+ raise SynapseError(400, "Missing 'term' arg")
+
+ logger.info("term: %s ", term)
@erikjohnston

erikjohnston Jan 19, 2017

Owner

This should probably be debug level logging.

synapse/storage/__init__.py
@@ -284,6 +284,48 @@ def get_user_ip_and_agents(self, user):
desc="get_user_ip_and_agents",
)
+ def get_users(self, user):
@erikjohnston

erikjohnston Jan 19, 2017

Owner

Why does this take a user param?

@erikjohnston
Owner

erikjohnston commented Jan 19, 2017

(The sytest dendron failure is probably spurious, ignore it)
@morteza-araby
Contributor

morteza-araby commented Jan 21, 2017

@morteza-araby morteza-araby changed the base branch from master to develop Feb 2, 2017

@morteza-araby
admin,storage: added more administrator functionalities 
administrators can now:
 - Set displayname of users
 - Update user avatars
 - Search for users by user_id
 - Browse all users in a paginated API
 - Reset user passwords
 - Deactivate users

Helpers for doing paginated queries has also been added to storage

Signed-off-by: Morteza Araby <morteza.araby@ericsson.com>

Some checks were not successful

1 failing and 3 successful checks
@matrixbot
Sytest Dendron (Merged PR) — Build finished.
Details
@matrixbot
Sytest Postgres (Merged PR) — Build finished.
Details
@matrixbot
Sytest SQLite (Merged PR) — Build finished.
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
2849d3f

@erikjohnston erikjohnston merged commit af6da6d into matrix-org:develop Feb 6, 2017
3 of 4 checks passed

3 of 4 checks passed

Sytest Dendron (Merged PR) Build finished.
Details
Sytest Postgres (Merged PR) Build finished.
Details
Sytest SQLite (Merged PR) Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

erikjohnston added a commit that referenced this pull request Mar 13, 2017

@erikjohnston
Merge tag 'v0.19.3-rc1' into release-v0.19.3 
Changes in synapse v0.19.3-rc1 (2017-03-08)
===========================================

Features:

* Add some administration functionalities. Thanks to morteza-araby! (PR #1784)

Changes:

* Reduce database table sizes (PR #1873, #1916, #1923, #1963)
* Update contrib/ to not use syutil. Thanks to andrewshadura! (PR #1907)
* Don't fetch current state when sending an event in common case (PR #1955)

Bug fixes:

* Fix synapse_port_db failure. Thanks to Pneumaticat! (PR #1904)
* Fix caching to not cache error responses (PR #1913)
* Fix APIs to make kick & ban reasons work (PR #1917)
* Fix bugs in the /keys/changes api (PR #1921)
* Fix bug where users couldn't forget rooms they were banned from (PR #1922)
* Fix issue with long language values in pushers API (PR #1925)
* Fix a race in transaction queue (PR #1930)
* Fix dynamic thumbnailing to preserve aspect ratio. Thanks to jkolo! (PR
  #1945)
* Fix device list update to not constantly resync (PR #1964)
* Fix potential for huge memory usage when getting device that have
  changed (PR #1969)
8b0f2af
@richvdh
Member

richvdh commented Mar 22, 2017

It'd be nice if someone could write something in https://github.com/matrix-org/synapse/tree/master/docs/admin_api about this.
@jcgruenhage

jcgruenhage commented Mar 22, 2017

Also, it would be great if the swagger ui supported this.
@richvdh
Member

richvdh commented Mar 22, 2017

The swagger spec is limited to the client-server API. These APIs are synapse-specific.

Of course, you could give them a swagger UI, but it wouldn't be appropriate to include them in the current one.
@jcgruenhage

jcgruenhage commented Mar 22, 2017

It would probably be more appropriate to write a little synapse admin client instead
(Or maybe even a web interface for those :D)
synapse/rest/client/v1/admin.py
@@ -25,6 +26,34 @@
logger = logging.getLogger(__name__)
+class UsersRestServlet(ClientV1RestServlet):
+ PATTERNS = client_path_patterns("/admin/users/(?P<user_id>[^/]*)")
@richvdh

richvdh Apr 20, 2017

Member

is my understanding correct? this takes a user_id, and then ignores it? @morteza-araby

@morteza-araby

morteza-araby Apr 21, 2017

Contributor

you are right, I must have missed that. The UsersRest Api, has not been used at my client side implementation and I think that's why I've missed all of your comments here. Probably should have wrote some test cases.

@richvdh

richvdh Oct 10, 2017

Member

This is now tracked as #2522

synapse/rest/client/v1/admin.py
+ # raise AuthError(403, "You are not a server admin")
+
+ if not self.hs.is_mine(target_user):
+ raise SynapseError(400, "Can only users a local user")
@richvdh

richvdh Apr 20, 2017

Member

this error makes no sense

@morteza-araby

morteza-araby Apr 21, 2017

Contributor

you are right.

synapse/rest/client/v1/admin.py
+ if not new_password:
+ raise SynapseError(400, "Missing 'new_password' arg")
+
+ logger.info("new_password: %r", new_password)
@richvdh

richvdh Apr 20, 2017

Member

we really shouldn't log passwords at info.

@morteza-araby

morteza-araby Apr 21, 2017

Contributor

should be removed.

@richvdh

richvdh Oct 10, 2017

Member

yes it should...

richvdh added a commit that referenced this pull request Apr 20, 2017

@richvdh
Document some of the admin APIs 
I haven't (yet) documented all of the user-list APIs introduced in
#1784 because the API shape seems
very odd, given the functionality.

All checks have passed

4 successful checks
@matrixbot
Sytest Dendron (Commit) — Build #1978 origin/rav/document_admin_apis succeeded in 8 min 41 sec
Details
@matrixbot
Sytest Postgres (Commit) — Build #2808 origin/rav/document_admin_apis succeeded in 7 min 55 sec
Details
@matrixbot
Sytest SQLite (Commit) — Build #2877 origin/rav/document_admin_apis succeeded in 5 min 50 sec
Details
continuous-integration/travis-ci/push — The Travis CI build passed
Details
8bbf154

@richvdh richvdh referenced this pull request Apr 20, 2017

Merged

Document some of the admin APIs #2143

richvdh added a commit that referenced this pull request Apr 21, 2017

@richvdh
Document some of the admin APIs (#2143) 
I haven't (yet) documented all of the user-list APIs introduced in
#1784 because the API shape seems
very odd, given the functionality.

All checks have passed

4 successful checks
@matrixbot
Sytest Dendron (Commit) — Build #1986 origin/develop succeeded in 9 min 19 sec
Details
@matrixbot
Sytest Postgres (Commit) — Build #2816 origin/develop succeeded in 6 min 56 sec
Details
@matrixbot
Sytest SQLite (Commit) — Build #2885 origin/develop succeeded in 5 min 9 sec
Details
continuous-integration/travis-ci/push — The Travis CI build passed
Details
cea7839

@richvdh richvdh referenced this pull request Oct 10, 2017

Open

<user id> should be removed from /admin/users/<user id> #2522

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment