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

create support user #4141

Merged
merged 67 commits into from
Dec 14, 2018
Merged

create support user #4141

merged 67 commits into from
Dec 14, 2018

Conversation

neilisfragile
Copy link
Contributor

Creates a support user and excludes it from mau counting and the user dir.

I'm not very happy with the user dir exclusions since it means finding 6 places where the index is added to/updated and filtering, thoughts on better impls much appreciated

@neilisfragile neilisfragile changed the title Neilj/create support user create support user Nov 6, 2018
@rubo77
Copy link
Contributor

rubo77 commented Nov 6, 2018

What will the support user be for?

@neilisfragile
Copy link
Contributor Author

@rubo77 it's an off by default feature to aid trouble shooting.

@rubo77
Copy link
Contributor

rubo77 commented Nov 6, 2018

what is "an off"? (sorry, I am not native english)

So what is the user doing if activated? what is this change for? is it connected with another PR?

@neilisfragile
Copy link
Contributor Author

@rubo77 This feature is useful if you are supporting someone's homeserver and want to log in as a user to trouble shoot without polluting the user directory or monthly active user counts. The specific driver is for Modular though this could be used in any context where one party is providing support to another.

by 'an off by default' - I mean that it is not expected that most synapse admins will want this behaviour and by default it will not be enabled.

@rubo77
Copy link
Contributor

rubo77 commented Nov 6, 2018

This seems a bit dangerous: as an admin You could very comfortable spy on others with this feature.

What about encrypted rooms? Is it impossible to join those as support user?

@neilisfragile
Copy link
Contributor Author

To be clear, the user is still visible in any room it joins - so in that regard it is no different from a regular user signing up and joining rooms - I don't see an opportunity for spying. The difference is that the support user does not appear in search results and does not contribute to monthly active user limits.

@richvdh richvdh mentioned this pull request Dec 4, 2018
synapse/handlers/register.py Show resolved Hide resolved
# the room is never created, though this seems unlikely and
# recoverable from given the support user being involved in the first
# place.
if (self.hs.config.autocreate_auto_join_rooms and not is_support):
Copy link
Member

Choose a reason for hiding this comment

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

nit: parens are redundant here

synapse/rest/client/v1/admin.py Show resolved Hide resolved
synapse/storage/monthly_active_users.py Show resolved Hide resolved
@@ -184,6 +185,9 @@ def register(self, user_id, token=None, password_hash=None,
appservice_id (str): The ID of the appservice registering the user.
create_profile_with_displayname (unicode): Optionally create a profile for
the user, setting their displayname to the given value
admin (boolean): is an admin user?
user_type (synapse.api.constants import UserTypes): type of user
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
user_type (synapse.api.constants import UserTypes): type of user
user_type (synapse.api.constants.UserTypes|None): type of user

@@ -247,6 +253,7 @@ def _register(
"is_guest": 1 if make_guest else 0,
"appservice_id": appservice_id,
"admin": 1 if admin else 0,
"user_type": user_type
Copy link
Member

Choose a reason for hiding this comment

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

trailing comma would be good here as well

* limitations under the License.
*/

/* Record the user_type of the user, initially this column is used to identify
Copy link
Member

Choose a reason for hiding this comment

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

This is kinda stating the obvious and will be unhelpful if we ever add more user types. How about:

The type of the user: NULL for a regular user, or one of the constants in synapse.api.constants.UserTypes

@@ -43,10 +43,6 @@ def setUp(self):
self.addCleanup,
expire_access_token=True,
)
self.macaroon_generator = Mock(
Copy link
Member

Choose a reason for hiding this comment

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

it seems to be a way of checking the access token is correctly passed through by get_or_create_user. So let me rephrase: is it necessary to remove these and weaken the result _token tests below?

Copy link
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.

nearly there!

@@ -65,6 +66,8 @@ def request_registration(
mac.update(password.encode('utf8'))
mac.update(b"\x00")
mac.update(b"admin" if admin else b"notadmin")
mac.update(b"\x00")
Copy link
Member

Choose a reason for hiding this comment

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

as per my comments in #synapse-dev, it would be nice to preserve backwards-compatibility for users of this API who just want to make a regular user.

synapse/handlers/register.py Outdated Show resolved Hide resolved
synapse/rest/client/v1/admin.py Outdated Show resolved Hide resolved
synapse/storage/registration.py Outdated Show resolved Hide resolved
neilisfragile and others added 5 commits December 14, 2018 15:30
Co-Authored-By: neilisfragile <neil@matrix.org>
Co-Authored-By: neilisfragile <neil@matrix.org>
Co-Authored-By: neilisfragile <neil@matrix.org>
@@ -60,6 +60,7 @@ each separated by NULs. For an example of generation in Python::
mac.update(b"\x00")
mac.update(b"admin" if admin else b"notadmin")
mac.update(b"\x00")
Copy link
Member

Choose a reason for hiding this comment

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

this needs to be conditional :/

Copy link
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

@neilisfragile neilisfragile merged commit d2f7c4e into develop Dec 14, 2018
@neilisfragile neilisfragile deleted the neilj/create_support_user branch December 14, 2018 18:21
@richvdh richvdh mentioned this pull request Dec 20, 2018
richvdh added a commit that referenced this pull request Jan 8, 2019
Synapse 0.34.1rc1 (2019-01-08)
==============================

Features
--------

- Special-case a support user for use in verifying behaviour of a given server. The support user does not appear in user directory or monthly active user counts. ([\#4141](#4141), [\#4344](#4344))
- Support for serving .well-known files ([\#4262](#4262))
- Rework SAML2 authentication ([\#4265](#4265), [\#4267](#4267))
- SAML2 authentication: Initialise user display name from SAML2 data ([\#4272](#4272))
- Synapse can now have its conditional/extra dependencies installed by pip. This functionality can be used by using `pip install matrix-synapse[feature]`, where feature is a comma separated list with the possible values `email.enable_notifs`, `matrix-synapse-ldap3`, `postgres`, `resources.consent`, `saml2`, `url_preview`, and `test`. If you want to install all optional dependencies, you can use "all" instead. ([\#4298](#4298), [\#4325](#4325), [\#4327](#4327))
- Add routes for reading account data. ([\#4303](#4303))
- Add opt-in support for v2 rooms ([\#4307](#4307))
- Add a script to generate a clean config file ([\#4315](#4315))
- Return server data in /login response ([\#4319](#4319))

Bugfixes
--------

- Fix contains_url check to be consistent with other instances in code-base and check that value is an instance of string. ([\#3405](#3405))
- Fix CAS login when username is not valid in an MXID ([\#4264](#4264))
- Send CORS headers for /media/config ([\#4279](#4279))
- Add 'sandbox' to CSP for media reprository ([\#4284](#4284))
- Make the new landing page prettier. ([\#4294](#4294))
- Fix deleting E2E room keys when using old SQLite versions. ([\#4295](#4295))
- The metric synapse_admin_mau:current previously did not update when config.mau_stats_only was set to True ([\#4305](#4305))
- Fixed per-room account data filters ([\#4309](#4309))
- Fix indentation in default config ([\#4313](#4313))
- Fix synapse:latest docker upload ([\#4316](#4316))
- Fix test_metric.py compatibility with prometheus_client 0.5. Contributed by Maarten de Vries <maarten@de-vri.es>. ([\#4317](#4317))
- Avoid packaging _trial_temp directory in -py3 debian packages ([\#4326](#4326))
- Check jinja version for consent resource ([\#4327](#4327))
- fix NPE in /messages by checking if all events were filtered out ([\#4330](#4330))
- Fix `python -m synapse.config` on Python 3. ([\#4356](#4356))

Deprecations and Removals
-------------------------

- Remove the deprecated v1/register API on Python 2. It was never ported to Python 3. ([\#4334](#4334))

Internal Changes
----------------

- Getting URL previews of IP addresses no longer fails on Python 3. ([\#4215](#4215))
- drop undocumented dependency on dateutil ([\#4266](#4266))
- Update the example systemd config to use a virtualenv ([\#4273](#4273))
- Update link to kernel DCO guide ([\#4274](#4274))
- Make isort tox check print diff when it fails ([\#4283](#4283))
- Log room_id in Unknown room errors ([\#4297](#4297))
- Documentation improvements for coturn setup. Contributed by Krithin Sitaram. ([\#4333](#4333))
- Update pull request template to use absolute links ([\#4341](#4341))
- Update README to not lie about required restart when updating TLS certificates ([\#4343](#4343))
- Update debian packaging for compatibility with transitional package ([\#4349](#4349))
- Fix command hint to generate a config file when trying to start without a config file ([\#4353](#4353))
- Add better logging for unexpected errors while sending transactions ([\#4358](#4358))
erikjohnston added a commit that referenced this pull request Feb 18, 2019
When guest_access changes from allowed to forbidden all local guest
users should be kicked from the room. This did not happen when
revocation was received from federation on a worker.

Presumably broken in #4141
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.

None yet

5 participants