Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Admin API for creating new users#3415

Merged
hawkowl merged 5 commits into
developfrom
hawkowl/register-user-improv
Jul 20, 2018
Merged

Admin API for creating new users#3415
hawkowl merged 5 commits into
developfrom
hawkowl/register-user-improv

Conversation

@hawkowl
Copy link
Copy Markdown
Contributor

@hawkowl hawkowl commented Jun 20, 2018

This has an accompanying sytest change: matrix-org/sytest#453

@hawkowl hawkowl requested a review from a team June 22, 2018 09:06
@richvdh richvdh self-assigned this Jun 22, 2018
Copy link
Copy Markdown
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum. Given this is making an API which is incompatible to both the existing APIs, I think we should take the opportunity to move this to a completely separate endpoint.

It fits badly under /r0/register, since (a) it skips user-interactive auth, and (b) it's synapse specific.

In short, I'm afraid I would prefer making (and documenting) yet a third version, and deprecating the existing APIs, with a view to deleting them in short order.

@richvdh richvdh assigned hawkowl and unassigned richvdh Jun 22, 2018
@hawkowl hawkowl changed the title Update Shared Secret registration in v2 and make the create script point at it [WIP] Admin API for creating new users Jul 17, 2018
@hawkowl hawkowl changed the title [WIP] Admin API for creating new users Admin API for creating new users Jul 18, 2018
@hawkowl hawkowl requested a review from a team July 18, 2018 12:39
@hawkowl
Copy link
Copy Markdown
Contributor Author

hawkowl commented Jul 18, 2018

This moves the disparate and theoretically-insecure APIs from v1 and r0 into an admin path, including nonce use to prevent replay attacks.

This has an accompanying sytest change: matrix-org/sytest#453

Copy link
Copy Markdown
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally looks great.

Could we have a doc in docs/admin-api to document the new API?

Comment thread synapse/rest/client/v1/register.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we deprecate this for now, rather than remove it, please?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reverted

Comment thread tests/utils.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused as to what's going on here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Importing the admin APIs loads the media storage providers code, which tries to iterate over this (and you can't iterate over a mock)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sigh. ok. thanks.

Comment thread synapse/rest/client/v1/admin.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have a comment to explain what this does?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment thread synapse/rest/client/v1/admin.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do we know it's being reused?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the comment to be a bit more accurate

Comment thread synapse/rest/client/v1/admin.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we have a comment for this, please? what does it map to and from?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment thread synapse/secrets.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, but can we define what it does?

@hawkowl hawkowl force-pushed the hawkowl/register-user-improv branch from 3334d48 to 31f89bb Compare July 19, 2018 06:25
@hawkowl hawkowl requested a review from a team July 19, 2018 07:12
Copy link
Copy Markdown
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm modulo nits

Comment thread docs/admin_api/register_api.rst Outdated
Shared-Secret Registration
==========================

This API allows for the creation of users in an administrative and non-interactive way. This is generally used for bootstrapping a Synapse instance with administrator accounts.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we wrap at 80 chars please?

Comment thread docs/admin_api/register_api.rst Outdated
"device_id": "device_id_here"
}

The MAC is the hex digest output of the HMAC-SHA1 algorithm, with the key being the shared secret and the content being the nonce, user, password, and either the string "admin" or "notadmin", each separated by NULLs. For an example of generation in Python::
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NULs

Comment thread synapse/rest/client/v1/admin.py Outdated
"""
Attributes:
NONCE_TIMEOUT (int): Seconds until a generated nonce won't be accepted
nonces (dict): The nonces that we will accept. A dict of nonce to the
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so it's dict[str, int] ?

Comment thread tests/utils.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sigh. ok. thanks.

@hawkowl hawkowl merged commit e1a237e into develop Jul 20, 2018
@hawkowl hawkowl deleted the hawkowl/register-user-improv branch July 20, 2018 12:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants