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

Allow tracking puppeted users for MAU #11561

Merged
merged 6 commits into from
Jan 12, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/11561.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add `track_puppeted_user_ips` config flag to track puppeted user IP addresses. This also includes them in monthly active user counts.
6 changes: 6 additions & 0 deletions docs/sample_config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1502,6 +1502,12 @@ 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.
# Implicitly enables MAU tracking for puppeted users.
#track_puppeted_user_ips: false
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 missing a # line
  • I'm struggling to follow the description. What does the first sentence mean? What does "If this is enabled" mean (just uncommenting the example setting, ie setting track_puppeted_user_ips: false?)
  • convention is that the example should be the opposite to the default, which doesn't seem to be the case here.

More details at https://matrix-org.github.io/synapse/develop/code_style.html#configuration-file-format



# A list of application service config files to use
#
Expand Down
13 changes: 13 additions & 0 deletions synapse/api/auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ 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

Expand Down Expand Up @@ -246,6 +247,18 @@ async def _wrapped_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._track_puppeted_user_ips
):
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,
)
squahtx marked this conversation as resolved.
Show resolved Hide resolved

if is_guest and not allow_guest:
raise AuthError(
Expand Down
10 changes: 10 additions & 0 deletions synapse/config/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -59,6 +60,12 @@ 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

track_appservice_user_ips has a note about mau tracking ("Implicitly enables MAU tracking for application service users."). Shall we add a similar note here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought I added that /o\ I was going to at least, I swear 😅

Added now!

# Implicitly enables MAU tracking for puppeted users.
#track_puppeted_user_ips: false
""" % {
"formatted_default_state_types": formatted_default_state_types
}
Expand Down Expand Up @@ -136,5 +143,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",
},
},
}
33 changes: 33 additions & 0 deletions tests/api/test_auth.py
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,39 @@ def test_get_user_by_req_appservice_valid_token_invalid_device_id(self):
self.assertEquals(failure.value.code, 400)
self.assertEquals(failure.value.errcode, Codes.EXCLUSIVE)

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._track_puppeted_user_ips = 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")
Expand Down