Allow admin to register a new user by awesome-manuel · Pull Request #5742 · matrix-org/synapse

awesome-manuel · 2019-07-23T10:21:34Z

This implements #5741.

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file
Pull request includes a sign off

richvdh · 2019-07-23T12:29:56Z

hrm, what do other ppl on the dev team think about this? In particular:

is removing shared-secret support from /client/r0/register going to break things for people? It's worth noting that the docs don't say anything about it being available on the /client/r0/register endpoint.
if we add an admin API for registering users as an admin user rather than via a shared secret, should it go on a different endpoint to the existing api or is it ok to share?

@awesome-manuel: note that the admin api endpoints are documented at https://github.com/matrix-org/synapse/blob/master/docs/admin_api/ so as part of this change, that documentation will need updating.

awesome-manuel · 2019-07-23T13:52:08Z

@richvdh also note that the checks in _synapse/admin/v1/register are more strict than in /client/r0/register, e.g. no space allowed in username or password and max length must be lower than 512 characters.

richvdh

@awesome-manuel: thanks for this. I think our position on this is:

removing support for shared-secret registration from /client/r0/register is fine, but needs calling out as a specific point in the changelog (so it needs a .removal file in changelog.d). Ideally it would be a separate PR, but I won't insist on that.
We'd rather support for registration-as-admin ended up on a separate REST endpoint in /admin to shared-secret-registration.

awesome-manuel · 2019-07-30T10:17:31Z

@richvdh any suggestion how this new endpoint should be called?

richvdh · 2019-07-30T15:28:30Z

I suggest something like create_user. The existing register name is poor, but that is something to fix another time.

awesome-manuel · 2019-08-19T14:18:11Z

@richvdh see #5877

updated

awesome-manuel · 2019-08-27T13:55:46Z

I suggest something like create_user. The existing register name is poor, but that is something to fix another time.

What do you think of a POST on _synapse/admin/v1/users? Seems to be more REST-like.

richvdh · 2019-08-27T14:41:22Z

What do you think of a POST on _synapse/admin/v1/users? Seems to be more REST-like.

Sure, that could work.

This parameter was used to test if the homeserver belongs to this user_id. Now the requesting user is tested instead. Not sure if this is required, since the request must be an admin anyway.

Signed-off-by: Manuel Stahl <manuel.stahl@awesome-technologies.de>

awesome-manuel · 2019-08-28T13:26:34Z

Partly fixes #2522, but should be done in a separate PR.

richvdh

Looks like a great improvement, thanks! A few comments though...

richvdh · 2019-10-03T12:50:02Z

synapse/rest/admin/__init__.py


 class UsersRestServlet(RestServlet):
-    PATTERNS = historical_admin_path_patterns("/users/(?P<user_id>[^/]*)")
+    PATTERNS = historical_admin_path_patterns("/users")


Hrm. Can you make this continue to accept (but ignore) a /user_id, for compatibility with applications which have hacked around the existing broken API?

richvdh · 2019-10-03T12:50:52Z

synapse/rest/admin/__init__.py


-        if not self.hs.is_mine(target_user):
-            raise SynapseError(400, "Can only users a local user")
+        if not self.hs.is_mine(requester.user):


This is redundant. The requester is a local user by definition.

richvdh · 2019-10-10T11:49:22Z

synapse/rest/admin/__init__.py

+
+        body = parse_json_object_from_request(request)
+
+        if "username" not in body:


Consider assert_params_in_dict here (https://github.com/matrix-org/synapse/blob/master/synapse/http/servlet.py#L258)

richvdh · 2019-10-10T11:52:17Z

docs/admin_api/user_admin_api.rst

@@ -1,3 +1,60 @@
+Create Account


could you document the response format?

richvdh · 2019-10-10T11:52:34Z

synapse/rest/admin/__init__.py

+    }
+
+    returns:
+        200 OK with empty object if success otherwise an error.


shouldn't be an empty object afaict. (Indeed, your test even asserts that.)

richvdh · 2019-10-10T11:55:26Z

synapse/rest/admin/__init__.py

+            user_type=user_type,
+        )
+
+        result = yield register._create_registration_details(user_id, body)


this will issue an access token for the user, which I'm not sure is the right thing to do. (even if you do want to end up with an access token for the new user, I think that might be better served via a different API per #6054).

In short, I don't think you need to call _create_registration_details here, which is good since it's meant to be a private method and instantiating a RegisterRestServlet here is ugly.

richvdh · 2019-10-10T11:56:58Z

synapse/rest/admin/__init__.py

+
+        register = RegisterRestServlet(self.hs)
+
+        user_id = yield register.registration_handler.register_user(


there are better ways of getting hold of the registration_handler than via the RegisterRestServlet. Just stick
self.registration_handler = hs.get_registration_handler() in the constructor.

richvdh requested a review from a team July 23, 2019 12:30

awesome-manuel force-pushed the admin_registration branch 3 times, most recently from 0bc34d5 to fb56022 Compare July 25, 2019 09:24

richvdh previously requested changes Jul 30, 2019

View reviewed changes

neilisfragile mentioned this pull request Jul 31, 2019

Allow admin users to register new user account #5741

Closed

awesome-manuel mentioned this pull request Aug 19, 2019

Remove shared secret registration from _matrix/client/r0/register #5877

Merged

3 tasks

awesome-manuel force-pushed the admin_registration branch 2 times, most recently from 1c332b2 to a7e735f Compare August 19, 2019 15:09

awesome-manuel force-pushed the admin_registration branch 3 times, most recently from ec46859 to 5e9bb20 Compare August 27, 2019 13:52

awesome-manuel force-pushed the admin_registration branch from 5e9bb20 to d8df620 Compare August 27, 2019 14:54

awesome-manuel added 2 commits August 27, 2019 17:58

Remove <user_id> parameter from admin API /users/@user:dom

9cf9724

This parameter was used to test if the homeserver belongs to this user_id. Now the requesting user is tested instead. Not sure if this is required, since the request must be an admin anyway.

Allow admin users to register new users without a shared secret

356bf57

Signed-off-by: Manuel Stahl <manuel.stahl@awesome-technologies.de>

awesome-manuel force-pushed the admin_registration branch from d8df620 to 356bf57 Compare August 27, 2019 16:03

richvdh self-assigned this Oct 3, 2019

richvdh suggested changes Oct 10, 2019

View reviewed changes

awesome-manuel closed this Dec 3, 2019

awesome-manuel deleted the admin_registration branch December 3, 2019 13:54

awesome-manuel restored the admin_registration branch December 3, 2019 13:54

awesome-manuel mentioned this pull request Dec 9, 2019

Allow admin users to create or modify users without a shared secret #6495

Merged

4 tasks

dklimpel mentioned this pull request Jan 23, 2020

Admin API to add an email address to user accounts #6769

Closed


		body = parse_json_object_from_request(request)

		if "username" not in body:


		register = RegisterRestServlet(self.hs)

		user_id = yield register.registration_handler.register_user(

Uh oh!

Conversation

awesome-manuel commented Jul 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Checklist

Uh oh!

richvdh commented Jul 23, 2019

Uh oh!

awesome-manuel commented Jul 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

awesome-manuel commented Jul 30, 2019

Uh oh!

richvdh commented Jul 30, 2019

Uh oh!

awesome-manuel commented Aug 19, 2019

Uh oh!

awesome-manuel commented Aug 27, 2019

Uh oh!

richvdh commented Aug 27, 2019

Uh oh!

awesome-manuel commented Aug 28, 2019

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

richvdh Oct 3, 2019

Choose a reason for hiding this comment

Uh oh!

richvdh Oct 3, 2019

Choose a reason for hiding this comment

Uh oh!

richvdh Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

richvdh Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

richvdh Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

richvdh Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

richvdh Oct 10, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

awesome-manuel commented Jul 23, 2019 •

edited

Loading

awesome-manuel commented Jul 23, 2019 •

edited

Loading