From 9e90cc178e64a5d29373bc09582df0fe2b4b189d Mon Sep 17 00:00:00 2001 From: Jason Robinson Date: Fri, 10 Dec 2021 20:03:20 +0200 Subject: [PATCH 1/5] Allow tracking puppeted users for MAU Currently when puppeting another user, the user doing the puppeting is tracked for client ip's and MAU (if configured). When tracking MAU is important, it becomes necessary to be possible to also track the client IP's and MAU of puppeted users. As an example a client that manages user creation and creation of tokens via the Synapse admin API, passing those tokens for the client to use. This PR adds optional configuration to enable tracking of puppeted users into monthly active users. The default behaviour stays the same. Signed-off-by: Jason Robinson --- changelog.d/11561.feature | 1 + docs/sample_config.yaml | 5 +++++ synapse/api/auth.py | 13 +++++++++++++ synapse/config/server.py | 6 ++++++ tests/api/test_auth.py | 33 +++++++++++++++++++++++++++++++++ 5 files changed, 58 insertions(+) create mode 100644 changelog.d/11561.feature diff --git a/changelog.d/11561.feature b/changelog.d/11561.feature new file mode 100644 index 000000000000..6b81a8ed6176 --- /dev/null +++ b/changelog.d/11561.feature @@ -0,0 +1 @@ +Add config flag `mau_track_puppeted_users` to allow tracking puppeted users in terms of monthly active users. diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index 6696ed5d1ef9..c3152c2b84a4 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -439,6 +439,11 @@ manhole_settings: # - medium: 'email' # address: 'reserved_user@example.com' +# If enabled, puppeted users can also be tracked. By default when +# puppeting another user, the user who has created the access token +# for puppeting is tracked. If this is enabled, both requests are tracked. +#mau_track_puppeted_users: false + # Used by phonehome stats to group together related servers. #server_context: context diff --git a/synapse/api/auth.py b/synapse/api/auth.py index 44883c6663ff..af5e63e339f7 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -73,6 +73,7 @@ def __init__(self, hs: "HomeServer"): self._track_appservice_user_ips = hs.config.appservice.track_appservice_user_ips self._macaroon_secret_key = hs.config.key.macaroon_secret_key self._force_tracing_for_users = hs.config.tracing.force_tracing_for_users + self._mau_track_puppeted_users = hs.config.server.mau_track_puppeted_users async def check_user_in_room( self, @@ -208,6 +209,18 @@ async def get_user_by_req( user_agent=user_agent, device_id=device_id, ) + # Track also the puppeted user client IP if enabled and the user is puppeting + if ( + user_info.user_id != user_info.token_owner + and self._mau_track_puppeted_users + ): + await self.store.insert_client_ip( + user_id=user_info.user_id, + access_token=access_token, + ip=ip_addr, + user_agent=user_agent, + device_id=device_id, + ) if is_guest and not allow_guest: raise AuthError( diff --git a/synapse/config/server.py b/synapse/config/server.py index 1de2dea9b024..254a2c121093 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -418,6 +418,7 @@ def read_config(self, config, **kwargs): self.mau_trial_days = config.get("mau_trial_days", 0) self.mau_limit_alerting = config.get("mau_limit_alerting", True) + self.mau_track_puppeted_users = config.get("mau_track_puppeted_users", False) # How long to keep redacted events in the database in unredacted form # before redacting them. @@ -1128,6 +1129,11 @@ def generate_config_section( # - medium: 'email' # address: 'reserved_user@example.com' + # If enabled, puppeted users can also be tracked. By default when + # puppeting another user, the user who has created the access token + # for puppeting is tracked. If this is enabled, both requests are tracked. + #mau_track_puppeted_users: false + # Used by phonehome stats to group together related servers. #server_context: context diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py index 3aa9ba3c43ac..c603eb004fab 100644 --- a/tests/api/test_auth.py +++ b/tests/api/test_auth.py @@ -210,6 +210,39 @@ def test_get_user_by_req_appservice_valid_token_bad_user_id(self): request.requestHeaders.getRawHeaders = mock_getRawHeaders() self.get_failure(self.auth.get_user_by_req(request), AuthError) + def test_get_user_by_req__puppeted_token__not_tracking_puppeted_mau(self): + self.store.get_user_by_access_token = simple_async_mock( + TokenLookupResult( + user_id="@baldrick:matrix.org", + device_id="device", + token_owner="@admin:matrix.org", + ) + ) + self.store.insert_client_ip = simple_async_mock(None) + request = Mock(args={}) + request.getClientIP.return_value = "127.0.0.1" + request.args[b"access_token"] = [self.test_token] + request.requestHeaders.getRawHeaders = mock_getRawHeaders() + self.get_success(self.auth.get_user_by_req(request)) + self.store.insert_client_ip.assert_called_once() + + def test_get_user_by_req__puppeted_token__tracking_puppeted_mau(self): + self.auth._mau_track_puppeted_users = True + self.store.get_user_by_access_token = simple_async_mock( + TokenLookupResult( + user_id="@baldrick:matrix.org", + device_id="device", + token_owner="@admin:matrix.org", + ) + ) + self.store.insert_client_ip = simple_async_mock(None) + request = Mock(args={}) + request.getClientIP.return_value = "127.0.0.1" + request.args[b"access_token"] = [self.test_token] + request.requestHeaders.getRawHeaders = mock_getRawHeaders() + self.get_success(self.auth.get_user_by_req(request)) + self.assertEquals(self.store.insert_client_ip.call_count, 2) + def test_get_user_from_macaroon(self): self.store.get_user_by_access_token = simple_async_mock( TokenLookupResult(user_id="@baldrick:matrix.org", device_id="device") From 6f1a4e36f06a95f9bb65a3db9f33bc9cb1637926 Mon Sep 17 00:00:00 2001 From: Jason Robinson Date: Wed, 22 Dec 2021 11:52:48 +0200 Subject: [PATCH 2/5] Apply suggestions from code review Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com> --- changelog.d/11561.feature | 2 +- docs/sample_config.yaml | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/changelog.d/11561.feature b/changelog.d/11561.feature index 6b81a8ed6176..4b9f48040209 100644 --- a/changelog.d/11561.feature +++ b/changelog.d/11561.feature @@ -1 +1 @@ -Add config flag `mau_track_puppeted_users` to allow tracking puppeted users in terms of monthly active users. +Add `mau_track_puppeted_users` config flag to include puppeted users in the monthly active users count. diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index c3152c2b84a4..a1cc7ba36047 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -439,9 +439,9 @@ manhole_settings: # - medium: 'email' # address: 'reserved_user@example.com' -# If enabled, puppeted users can also be tracked. By default when -# puppeting another user, the user who has created the access token -# for puppeting is tracked. If this is enabled, both requests are tracked. +# If enabled, puppeted users will be counted in monthly active users. +# By default, when puppeting another user, only the puppeteer is counted. +# If this is enabled, both users are counted. #mau_track_puppeted_users: false # Used by phonehome stats to group together related servers. From 9e6a74b01822f75c5a98a9f32b395a50db5e1a2a Mon Sep 17 00:00:00 2001 From: Jason Robinson Date: Wed, 5 Jan 2022 11:17:09 +0200 Subject: [PATCH 3/5] Use `track_puppeted_user_ips` instead Also move it from server to api config, which seems more relevant, given the code it is used is in api auth. Signed-off-by: Jason Robinson --- changelog.d/11561.feature | 2 +- docs/sample_config.yaml | 10 +++++----- synapse/api/auth.py | 4 ++-- synapse/config/api.py | 9 +++++++++ synapse/config/server.py | 6 ------ tests/api/test_auth.py | 2 +- 6 files changed, 18 insertions(+), 15 deletions(-) diff --git a/changelog.d/11561.feature b/changelog.d/11561.feature index 4b9f48040209..19dada883bb2 100644 --- a/changelog.d/11561.feature +++ b/changelog.d/11561.feature @@ -1 +1 @@ -Add `mau_track_puppeted_users` config flag to include puppeted users in the monthly active users count. +Add `track_puppeted_user_ips` config flag to track puppeted user IP addresses. This also includes them in monthly active user counts. diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index a1cc7ba36047..a909c734c88e 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -439,11 +439,6 @@ manhole_settings: # - medium: 'email' # address: 'reserved_user@example.com' -# If enabled, puppeted users will be counted in monthly active users. -# By default, when puppeting another user, only the puppeteer is counted. -# If this is enabled, both users are counted. -#mau_track_puppeted_users: false - # Used by phonehome stats to group together related servers. #server_context: context @@ -1507,6 +1502,11 @@ room_prejoin_state: #additional_event_types: # - org.example.custom.event.type +# If enabled, puppeted user IP's can also be tracked. By default when +# puppeting another user, the user who has created the access token +# for puppeting is tracked. If this is enabled, both requests are tracked. +#track_puppeted_user_ips: false + # A list of application service config files to use # diff --git a/synapse/api/auth.py b/synapse/api/auth.py index 7784c718f697..683241201ca6 100644 --- a/synapse/api/auth.py +++ b/synapse/api/auth.py @@ -71,9 +71,9 @@ def __init__(self, hs: "HomeServer"): self._auth_blocking = AuthBlocking(self.hs) self._track_appservice_user_ips = hs.config.appservice.track_appservice_user_ips + self._track_puppeted_user_ips = hs.config.api.track_puppeted_user_ips self._macaroon_secret_key = hs.config.key.macaroon_secret_key self._force_tracing_for_users = hs.config.tracing.force_tracing_for_users - self._mau_track_puppeted_users = hs.config.server.mau_track_puppeted_users async def check_user_in_room( self, @@ -250,7 +250,7 @@ async def _wrapped_get_user_by_req( # Track also the puppeted user client IP if enabled and the user is puppeting if ( user_info.user_id != user_info.token_owner - and self._mau_track_puppeted_users + and self._track_puppeted_user_ips ): await self.store.insert_client_ip( user_id=user_info.user_id, diff --git a/synapse/config/api.py b/synapse/config/api.py index b18044f9822a..e1e7eb374782 100644 --- a/synapse/config/api.py +++ b/synapse/config/api.py @@ -29,6 +29,7 @@ class ApiConfig(Config): def read_config(self, config: JsonDict, **kwargs): validate_config(_MAIN_SCHEMA, config, ()) self.room_prejoin_state = list(self._get_prejoin_state_types(config)) + self.track_puppeted_user_ips = config.get("track_puppeted_user_ips", False) def generate_config_section(cls, **kwargs) -> str: formatted_default_state_types = "\n".join( @@ -59,6 +60,11 @@ def generate_config_section(cls, **kwargs) -> str: # #additional_event_types: # - org.example.custom.event.type + + # If enabled, puppeted user IP's can also be tracked. By default when + # puppeting another user, the user who has created the access token + # for puppeting is tracked. If this is enabled, both requests are tracked. + #track_puppeted_user_ips: false """ % { "formatted_default_state_types": formatted_default_state_types } @@ -136,5 +142,8 @@ def _get_prejoin_state_types(self, config: JsonDict) -> Iterable[str]: "properties": { "room_prejoin_state": _ROOM_PREJOIN_STATE_CONFIG_SCHEMA, "room_invite_state_types": _ROOM_INVITE_STATE_TYPES_SCHEMA, + "track_puppeted_user_ips": { + "type": "boolean", + }, }, } diff --git a/synapse/config/server.py b/synapse/config/server.py index 254a2c121093..1de2dea9b024 100644 --- a/synapse/config/server.py +++ b/synapse/config/server.py @@ -418,7 +418,6 @@ def read_config(self, config, **kwargs): self.mau_trial_days = config.get("mau_trial_days", 0) self.mau_limit_alerting = config.get("mau_limit_alerting", True) - self.mau_track_puppeted_users = config.get("mau_track_puppeted_users", False) # How long to keep redacted events in the database in unredacted form # before redacting them. @@ -1129,11 +1128,6 @@ def generate_config_section( # - medium: 'email' # address: 'reserved_user@example.com' - # If enabled, puppeted users can also be tracked. By default when - # puppeting another user, the user who has created the access token - # for puppeting is tracked. If this is enabled, both requests are tracked. - #mau_track_puppeted_users: false - # Used by phonehome stats to group together related servers. #server_context: context diff --git a/tests/api/test_auth.py b/tests/api/test_auth.py index d8117a205685..4b53b6d40b31 100644 --- a/tests/api/test_auth.py +++ b/tests/api/test_auth.py @@ -291,7 +291,7 @@ def test_get_user_by_req__puppeted_token__not_tracking_puppeted_mau(self): self.store.insert_client_ip.assert_called_once() def test_get_user_by_req__puppeted_token__tracking_puppeted_mau(self): - self.auth._mau_track_puppeted_users = True + self.auth._track_puppeted_user_ips = True self.store.get_user_by_access_token = simple_async_mock( TokenLookupResult( user_id="@baldrick:matrix.org", From c8c32367b5f5de839e5e79ff08c4f0c297d7c220 Mon Sep 17 00:00:00 2001 From: Jason Robinson Date: Wed, 5 Jan 2022 11:22:05 +0200 Subject: [PATCH 4/5] Remove whitespace causing linting issue Signed-off-by: Jason Robinson --- synapse/config/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/config/api.py b/synapse/config/api.py index e1e7eb374782..2c1cc9df12b8 100644 --- a/synapse/config/api.py +++ b/synapse/config/api.py @@ -60,7 +60,7 @@ def generate_config_section(cls, **kwargs) -> str: # #additional_event_types: # - org.example.custom.event.type - + # If enabled, puppeted user IP's can also be tracked. By default when # puppeting another user, the user who has created the access token # for puppeting is tracked. If this is enabled, both requests are tracked. From 0950d5673d777e8bac55b1fdc00a24480f7b0801 Mon Sep 17 00:00:00 2001 From: Jason Robinson Date: Wed, 5 Jan 2022 16:40:33 +0200 Subject: [PATCH 5/5] Add note about MAU tracking for puppeted users Signed-off-by: Jason Robinson --- docs/sample_config.yaml | 1 + synapse/config/api.py | 1 + 2 files changed, 2 insertions(+) diff --git a/docs/sample_config.yaml b/docs/sample_config.yaml index a909c734c88e..b3dbc10bbf34 100644 --- a/docs/sample_config.yaml +++ b/docs/sample_config.yaml @@ -1505,6 +1505,7 @@ room_prejoin_state: # If enabled, puppeted user IP's can also be tracked. By default when # puppeting another user, the user who has created the access token # for puppeting is tracked. If this is enabled, both requests are tracked. +# Implicitly enables MAU tracking for puppeted users. #track_puppeted_user_ips: false diff --git a/synapse/config/api.py b/synapse/config/api.py index 2c1cc9df12b8..d3ef2edb71db 100644 --- a/synapse/config/api.py +++ b/synapse/config/api.py @@ -64,6 +64,7 @@ def generate_config_section(cls, **kwargs) -> str: # If enabled, puppeted user IP's can also be tracked. By default when # puppeting another user, the user who has created the access token # for puppeting is tracked. If this is enabled, both requests are tracked. + # Implicitly enables MAU tracking for puppeted users. #track_puppeted_user_ips: false """ % { "formatted_default_state_types": formatted_default_state_types