Add ability for access tokens to belong to one user but grant access to another user. #8616

erikjohnston · 2020-10-21T16:20:22Z

~~Based on #8615, #8614.~~ This is towards #6054.

We do it this way round so that only the "owner" can delete the access token (i.e. /logout/all by the "owner" also deletes that token, but /logout/all by the "target user" doesn't).

A future PR will add an API for creating such a token.

When the target user and authenticated entity are different the Processed request log line will be logged with a: {@admin:server as @bob:server} .... I'm not convinced by that format (especially since it adds spaces in there, making it harder to use cut -d ' ' to chop off the start of log lines). Suggestions welcome.

clokep

I'm fairly certain this looks OK, but it might make sense for another team member to take a quick look, particularly at the wording / naming of things since we originally had the discussion about that. 👍

synapse/types.py

synapse/storage/databases/main/registration.py

erikjohnston · 2020-10-22T15:51:18Z

When the target user and authenticated entity are different the Processed request log line will be logged with a: {@admin:server as @bob:server} .... I'm not convinced by that format (especially since it adds spaces in there, making it harder to use cut -d ' ' to chop off the start of log lines). Suggestions welcome.

I wonder if a format like {@admin:server,@bob:server} would be better, though less intuitive.

michaelkaye · 2020-10-23T09:31:30Z

@erikjohnston Does that affect which user ends up in the token? I know there are some tools that use the data in the token for load balancing by user ID, rate limiting or abuse management; will this violate any (unspecced but assumed) expectations about token ownership?

erikjohnston · 2020-10-23T09:35:20Z

@erikjohnston Does that affect which user ends up in the token? I know there are some tools that use the data in the token for load balancing by user ID, rate limiting or abuse management; will this violate any (unspecced but assumed) expectations about token ownership?

Good point. The owner will be in the token, I think that should be find on all those counts as you want those things to apply to the owner rather than the target user?

Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>

richvdh

seems generally sensible to me. A few thoughts.

richvdh · 2020-10-26T15:07:31Z

synapse/http/site.py

        self.authenticated_entity = None
+        self.target_user = None


should probably have some comments to explain what these mean.

can we just replace both with a requester property?

Hmm, annoyingly this is also used by the federation code which doesn't have a Requester object.

I made SynapseRequest.requester be either a Requester or str, which felt the cleanest.

richvdh · 2020-10-26T15:10:42Z

synapse/http/site.py

+        if self.target_user:
+            authenticated_entity = "{} as {}".format(
+                authenticated_entity, self.target_user,
+            )


we're using the as format even if the two user ids are the same?

Woops, that is a hold over from when target_user was None if they were the same. The intention is to only use the format if the two users are different.

Also, I might change the format to be {<auth_entity>,<target_user>} to make parsing the logs easier (as we use spaces as separators up to the message)

richvdh · 2020-10-26T15:19:07Z

synapse/handlers/auth.py

            await self.hs.get_pusherpool().remove_pushers_by_access_token(
-                str(user_info["user"]), (user_info["token_id"],)
+                user_info.user_id, (user_info.token_id,)


any idea what the str here was for?

The user_info["user"] object was a UserID object.

richvdh · 2020-10-26T15:22:12Z

synapse/storage/databases/main/registration.py

+            user, or a server admin who is logged in as the user.
+    """
+
+    user_id = attr.ib(type=str)


it feels confusing that Requests have an authenticated_user and a target_user, Requester has authenticated_entity and user, and this has token_owner and a user_id. I keep forgetting which is which, particularly the ones called just "user".

Agreed. I can certainly change the user_id to be target_user, I'm not sure about using authenticated_entity here since I'm not sure we've actually authenticated.

Or something, I'll try and make it better

Ok, so now SynapseRequest takes a Requester object and I changed the opentracing tag from target_user to user_id. This should mean that everything uses user/user_id to mean the same thing, so the only difference is token_owner vs authenticated_entity.

I'm not terribly happy with Requester.user, but changing it means touching a lot of places, which I'm a bit in two minds about doing.

synapse/types.py

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

erikjohnston · 2020-10-27T10:33:44Z

I guess we also probably want to use target_user_id rather than target_user when they're strings rather than UserID objects

erikjohnston · 2020-10-27T12:21:08Z

I've checked the logs and we appear to be logging requesters correctly (i.e. there is no change), both for clients and servers.

clokep

A couple of thoughts. I'm not sure any of them are really blocking this though.

clokep · 2020-10-28T15:19:31Z

synapse/api/auth.py

+                    user_id, app_service=app_service
+                )
+
+                request.requester = user_id


This implies some potential clean-up if each request has a requester on it... (I think we frequently pass the SynapseRequest and Requester into handlers). Nothing to really do here though. Just noting.

synapse/api/auth.py

We require application services to be configured with a sender, so we may as well require it when creating an ApplicationService object.

clokep · 2020-10-29T13:21:00Z

@erikjohnston Looks like tests are failing?

erikjohnston · 2020-10-29T13:37:03Z

Gah, python3.5

@dklimpel

Synapse 1.23.0 (2020-11-18) =========================== This release changes the way structured logging is configured. See the [upgrade notes](UPGRADE.rst#upgrading-to-v1230) for details. **Note**: We are aware of a trivially exploitable denial of service vulnerability in versions of Synapse prior to 1.20.0. Complete details will be disclosed on Monday, November 23rd. If you have not upgraded recently, please do so. Bugfixes -------- - Fix a dependency versioning bug in the Dockerfile that prevented Synapse from starting. ([\#8767](matrix-org/synapse#8767)) Synapse 1.23.0rc1 (2020-11-13) ============================== Features -------- - Add a push rule that highlights when a jitsi conference is created in a room. ([\#8286](matrix-org/synapse#8286)) - Add an admin api to delete a single file or files that were not used for a defined time from server. Contributed by @dklimpel. ([\#8519](matrix-org/synapse#8519)) - Split admin API for reported events (`GET /_synapse/admin/v1/event_reports`) into detail and list endpoints. This is a breaking change to #8217 which was introduced in Synapse v1.21.0. Those who already use this API should check their scripts. Contributed by @dklimpel. ([\#8539](matrix-org/synapse#8539)) - Support generating structured logs via the standard logging configuration. ([\#8607](matrix-org/synapse#8607), [\#8685](matrix-org/synapse#8685)) - Add an admin API to allow server admins to list users' pushers. Contributed by @dklimpel. ([\#8610](matrix-org/synapse#8610), [\#8689](matrix-org/synapse#8689)) - Add an admin API `GET /_synapse/admin/v1/users/<user_id>/media` to get information about uploaded media. Contributed by @dklimpel. ([\#8647](matrix-org/synapse#8647)) - Add an admin API for local user media statistics. Contributed by @dklimpel. ([\#8700](matrix-org/synapse#8700)) - Add `displayname` to Shared-Secret Registration for admins. ([\#8722](matrix-org/synapse#8722)) Bugfixes -------- - Fix fetching of E2E cross signing keys over federation when only one of the master key and device signing key is cached already. ([\#8455](matrix-org/synapse#8455)) - Fix a bug where Synapse would blindly forward bad responses from federation to clients when retrieving profile information. ([\#8580](matrix-org/synapse#8580)) - Fix a bug where the account validity endpoint would silently fail if the user ID did not have an expiration time. It now returns a 400 error. ([\#8620](matrix-org/synapse#8620)) - Fix email notifications for invites without local state. ([\#8627](matrix-org/synapse#8627)) - Fix handling of invalid group IDs to return a 400 rather than log an exception and return a 500. ([\#8628](matrix-org/synapse#8628)) - Fix handling of User-Agent headers that are invalid UTF-8, which caused user agents of users to not get correctly recorded. ([\#8632](matrix-org/synapse#8632)) - Fix a bug in the `joined_rooms` admin API if the user has never joined any rooms. The bug was introduced, along with the API, in v1.21.0. ([\#8643](matrix-org/synapse#8643)) - Fix exception during handling multiple concurrent requests for remote media when using multiple media repositories. ([\#8682](matrix-org/synapse#8682)) - Fix bug that prevented Synapse from recovering after losing connection to the database. ([\#8726](matrix-org/synapse#8726)) - Fix bug where the `/_synapse/admin/v1/send_server_notice` API could send notices to non-notice rooms. ([\#8728](matrix-org/synapse#8728)) - Fix PostgreSQL port script fails when DB has no backfilled events. Broke in v1.21.0. ([\#8729](matrix-org/synapse#8729)) - Fix PostgreSQL port script to correctly handle foreign key constraints. Broke in v1.21.0. ([\#8730](matrix-org/synapse#8730)) - Fix PostgreSQL port script so that it can be run again after a failure. Broke in v1.21.0. ([\#8755](matrix-org/synapse#8755)) Improved Documentation ---------------------- - Instructions for Azure AD in the OpenID Connect documentation. Contributed by peterk. ([\#8582](matrix-org/synapse#8582)) - Improve the sample configuration for single sign-on providers. ([\#8635](matrix-org/synapse#8635)) - Fix the filepath of Dex's example config and the link to Dex's Getting Started guide in the OpenID Connect docs. ([\#8657](matrix-org/synapse#8657)) - Note support for Python 3.9. ([\#8665](matrix-org/synapse#8665)) - Minor updates to docs on running tests. ([\#8666](matrix-org/synapse#8666)) - Interlink prometheus/grafana documentation. ([\#8667](matrix-org/synapse#8667)) - Notes on SSO logins and media_repository worker. ([\#8701](matrix-org/synapse#8701)) - Document experimental support for running multiple event persisters. ([\#8706](matrix-org/synapse#8706)) - Add information regarding the various sources of, and expected contributions to, Synapse's documentation to `CONTRIBUTING.md`. ([\#8714](matrix-org/synapse#8714)) - Migrate documentation `docs/admin_api/event_reports` to markdown. ([\#8742](matrix-org/synapse#8742)) - Add some helpful hints to the README for new Synapse developers. Contributed by @chagai95. ([\#8746](matrix-org/synapse#8746)) Internal Changes ---------------- - Optimise `/createRoom` with multiple invited users. ([\#8559](matrix-org/synapse#8559)) - Implement and use an `@lru_cache` decorator. ([\#8595](matrix-org/synapse#8595)) - Don't instansiate Requester directly. ([\#8614](matrix-org/synapse#8614)) - Type hints for `RegistrationStore`. ([\#8615](matrix-org/synapse#8615)) - Change schema to support access tokens belonging to one user but granting access to another. ([\#8616](matrix-org/synapse#8616)) - Remove unused OPTIONS handlers. ([\#8621](matrix-org/synapse#8621)) - Run `mypy` as part of the lint.sh script. ([\#8633](matrix-org/synapse#8633)) - Correct Synapse's PyPI package name in the OpenID Connect installation instructions. ([\#8634](matrix-org/synapse#8634)) - Catch exceptions during initialization of `password_providers`. Contributed by Nicolai Søborg. ([\#8636](matrix-org/synapse#8636)) - Fix typos and spelling errors in the code. ([\#8639](matrix-org/synapse#8639)) - Reduce number of OpenTracing spans started. ([\#8640](matrix-org/synapse#8640), [\#8668](matrix-org/synapse#8668), [\#8670](matrix-org/synapse#8670)) - Add field `total` to device list in admin API. ([\#8644](matrix-org/synapse#8644)) - Add more type hints to the application services code. ([\#8655](matrix-org/synapse#8655), [\#8693](matrix-org/synapse#8693)) - Tell Black to format code for Python 3.5. ([\#8664](matrix-org/synapse#8664)) - Don't pull event from DB when handling replication traffic. ([\#8669](matrix-org/synapse#8669)) - Abstract some invite-related code in preparation for landing knocking. ([\#8671](matrix-org/synapse#8671), [\#8688](matrix-org/synapse#8688)) - Clarify representation of events in logfiles. ([\#8679](matrix-org/synapse#8679)) - Don't require `hiredis` package to be installed to run unit tests. ([\#8680](matrix-org/synapse#8680)) - Fix typing info on cache call signature to accept `on_invalidate`. ([\#8684](matrix-org/synapse#8684)) - Fail tests if they do not await coroutines. ([\#8690](matrix-org/synapse#8690)) - Improve start time by adding an index to `e2e_cross_signing_keys.stream_id`. ([\#8694](matrix-org/synapse#8694)) - Re-organize the structured logging code to separate the TCP transport handling from the JSON formatting. ([\#8697](matrix-org/synapse#8697)) - Use Python 3.8 in Docker images by default. ([\#8698](matrix-org/synapse#8698)) - Remove the "draft" status of the Room Details Admin API. ([\#8702](matrix-org/synapse#8702)) - Improve the error returned when a non-string displayname or avatar_url is used when updating a user's profile. ([\#8705](matrix-org/synapse#8705)) - Block attempts by clients to send server ACLs, or redactions of server ACLs, that would result in the local server being blocked from the room. ([\#8708](matrix-org/synapse#8708)) - Add metrics the allow the local sysadmin to track 3PID `/requestToken` requests. ([\#8712](matrix-org/synapse#8712)) - Consolidate duplicated lists of purged tables that are checked in tests. ([\#8713](matrix-org/synapse#8713)) - Add some `mdui:UIInfo` element examples for `saml2_config` in the homeserver config. ([\#8718](matrix-org/synapse#8718)) - Improve the error message returned when a remote server incorrectly sets the `Content-Type` header in response to a JSON request. ([\#8719](matrix-org/synapse#8719)) - Speed up repeated state resolutions on the same room by caching event ID to auth event ID lookups. ([\#8752](matrix-org/synapse#8752)) Synapse 1.22.1 (2020-10-30) =========================== Bugfixes -------- - Fix a bug where an appservice may not be forwarded events for a room it was recently invited to. Broke in v1.22.0. ([\#8676](matrix-org/synapse#8676)) - Fix `Object of type frozendict is not JSON serializable` exceptions when using third-party event rules. Broke in v1.22.0. ([\#8678](matrix-org/synapse#8678))

erikjohnston mentioned this pull request Oct 21, 2020

Add admin API for logging in as a user #8617

Merged

erikjohnston added 3 commits October 22, 2020 11:57

Make get_user_by_access_token return a proper type

bc422e1

Add concept of authenticated_entity vs target_user

17a3d94

Newsfile

028f04b

erikjohnston force-pushed the erikj/puppet branch from 03625d3 to 028f04b Compare October 22, 2020 10:57

erikjohnston requested a review from a team October 22, 2020 11:01

clokep reviewed Oct 22, 2020

View reviewed changes

synapse/types.py Outdated Show resolved Hide resolved

synapse/storage/databases/main/registration.py Show resolved Hide resolved

clokep requested a review from a team October 22, 2020 11:39

erikjohnston and others added 2 commits October 23, 2020 10:52

Update synapse/types.py

9b72dab

Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>

Update docstring

66ef6cd

richvdh reviewed Oct 26, 2020

View reviewed changes

Update docstring

c87bf0d

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>

erikjohnston added 4 commits October 27, 2020 11:34

Pass requester to SynapseRequest

6a06304

Change log format when puppetting user to use ',' rather than ' as '

3286c21

Add types to create_requester

acf1314

Change opentracing to use user_id not target_user

6f736b8

erikjohnston force-pushed the erikj/puppet branch from e78bf5d to 6f736b8 Compare October 27, 2020 11:34

Fix mypy

9d919da

erikjohnston requested a review from a team October 27, 2020 12:21

clokep reviewed Oct 28, 2020

View reviewed changes

Review comments

49bb079

erikjohnston requested a review from clokep October 28, 2020 16:02

Fix tests by mandating AS have sender.

492e15e

We require application services to be configured with a sender, so we may as well require it when creating an ApplicationService object.

py3.5 syntax

05c7f7b

clokep approved these changes Oct 29, 2020

View reviewed changes

erikjohnston merged commit f21e24f into develop Oct 29, 2020

erikjohnston deleted the erikj/puppet branch October 29, 2020 15:58

erikjohnston mentioned this pull request Oct 29, 2020

Fix unit tests #8689

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability for access tokens to belong to one user but grant access to another user. #8616

Add ability for access tokens to belong to one user but grant access to another user. #8616

erikjohnston commented Oct 21, 2020 •

edited

Loading

clokep left a comment •

edited

Loading

erikjohnston commented Oct 22, 2020

michaelkaye commented Oct 23, 2020

erikjohnston commented Oct 23, 2020

richvdh left a comment

richvdh Oct 26, 2020

richvdh Oct 26, 2020

erikjohnston Oct 27, 2020

erikjohnston Oct 27, 2020

richvdh Oct 26, 2020

erikjohnston Oct 27, 2020

richvdh Oct 26, 2020

erikjohnston Oct 27, 2020

richvdh Oct 26, 2020

erikjohnston Oct 27, 2020

erikjohnston Oct 27, 2020

erikjohnston Oct 27, 2020

erikjohnston commented Oct 27, 2020

erikjohnston commented Oct 27, 2020

clokep left a comment

clokep Oct 28, 2020

clokep commented Oct 29, 2020

erikjohnston commented Oct 29, 2020

Add ability for access tokens to belong to one user but grant access to another user. #8616

Add ability for access tokens to belong to one user but grant access to another user. #8616

Conversation

erikjohnston commented Oct 21, 2020 • edited Loading

clokep left a comment • edited Loading

Choose a reason for hiding this comment

erikjohnston commented Oct 22, 2020

michaelkaye commented Oct 23, 2020

erikjohnston commented Oct 23, 2020

richvdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikjohnston commented Oct 27, 2020

erikjohnston commented Oct 27, 2020

clokep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clokep commented Oct 29, 2020

erikjohnston commented Oct 29, 2020

erikjohnston commented Oct 21, 2020 •

edited

Loading

clokep left a comment •

edited

Loading