Add script to kick idle bridged users from a room #340

lukebarnard1 · 2017-01-09T15:06:12Z

$ python remove-idle-users.py
--token=someaccesstoken23478586496849569
--alias='#somealias:some.domain'
--homeserver=http://some.domain
--domain=some.domain
--since=50
--template=@irc_$NICK

--token : the access token for the AS
--alias : the alias of the room to kick from
--homeserver : the URL of the HS
--domain : the domain part of the HS URL
--template : the userPattern template (in config.sample.yaml, matrixClients)

python remove-idle-users.py --token=someaccesstoken23478586496849569 --alias='#somealias:some.domain' --homeserver=http://some.domain --domain=some.domain --since=50 --template=@irc_$NICK --token : the access token for the AS --alias : the alias of the room to kick from --homeserver : the URL of the HS --domain : the domain part of the HS URL --template : the userPattern template (in config.sample.yaml, matrixClients)

kegsay · 2017-01-09T15:25:23Z

scripts/remove-idle-users.py

+
+if __name__ == "__main__":
+    parser = argparse.ArgumentParser("Remove idle users from a given Matrix room")
+    parser.add_argument("-t", "--token", help="The AS token", required=True)


Technically any token, not just AS.

kegsay · 2017-01-09T15:25:53Z

scripts/remove-idle-users.py

+    parser.add_argument("-t", "--token", help="The AS token", required=True)
+    parser.add_argument("-a", "--alias", help="The alias of the room eg '#freenode_#matrix-dev:matrix.org'", required=True)
+    parser.add_argument("-u", "--homeserver", help="Base homeserver URL eg 'https://matrix.org'", required=True)
+    parser.add_argument("-d", "--domain", help=" matrix.org'", required=True)


' mismatch. Also missing the sentence!

kegsay · 2017-01-09T15:26:08Z

scripts/remove-idle-users.py

+    parser.add_argument("-a", "--alias", help="The alias of the room eg '#freenode_#matrix-dev:matrix.org'", required=True)
+    parser.add_argument("-u", "--homeserver", help="Base homeserver URL eg 'https://matrix.org'", required=True)
+    parser.add_argument("-d", "--domain", help=" matrix.org'", required=True)
+    parser.add_argument("-s", "--since", type=int, help="Since idle users have been offline for", required=True)


Missing units and example.

kegsay · 2017-01-09T15:27:00Z

scripts/remove-idle-users.py

+
+def main(token, alias, homeserver, homeserver_domain, since, user_template):
+    print("Removing idle users in %s" % alias)
+    token = token


kegsay · 2017-01-09T17:19:18Z

scripts/remove-idle-users.py

+
+def get_room_id(homeserver, alias, token):
+    res = requests.get(homeserver + "/_matrix/client/r0/directory/room/" + urllib.quote(alias) + "?access_token=" + token).json()
+    return res.get("room_id", None)


Nit: Rather than return a nullable room_id, I would just do:

res = requests.get(homeserver + "/_matrix/client/r0/directory/room/" + urllib.quote(alias) + "?access_token=" + token) res.raise_for_status() return res.json()["room_id"]

This will throw on non-2xx and if room_id doesn't exist, both of which you want to do. You can then remove the null guard on :94.

But then you would get a quite useless error when the room_id doesn't get sent back? If this does indicate that the server couldn't resolve it it, would it not be better to throw an error that suggests as such?

Ah but then you don't get to see the error. Fair point, I'll have it raise for status.

kegsay · 2017-01-09T17:21:11Z

scripts/remove-idle-users.py

+    return get_last_active_ago(homeserver, user_id, token) > activity_threshold_ms
+
+def get_idle_users(homeserver, room_id, token, since):
+    res = requests.get(homeserver + "/_matrix/client/r0/rooms/" + urllib.quote(room_id) + "/members?access_token=" + token)


Don't use this API, it'll take far too long. Instead, use /joined_members - https://github.com/matrix-org/gomatrix/blob/master/client.go#L546

This should really be documented... would have saved me some time

It's a new API Erik added not long ago, hence missing docs.

kegsay · 2017-01-09T17:33:37Z

scripts/remove-idle-users.py

+                "user_id": user_id
+            })
+        )
+        res.raise_for_status()


Don't raise for status here. It'll be annoying as all hell if it spends the time working out who to kick then dies on the first failure. I would add the failures to a list and then whine at the end saying "I couldn't kick these guys because $RESPONSE_JSON".

kegsay · 2017-01-09T17:35:40Z

scripts/remove-idle-users.py

+    parser.add_argument("-u", "--homeserver", help="Base homeserver URL eg 'https://matrix.org'", required=True)
+    parser.add_argument("-d", "--domain", help="Domain of the homserver eg 'matrix.org'", required=True)
+    parser.add_argument("-s", "--since", type=int, help="Days since idle users have been offline for eg '30'", required=True)
+    parser.add_argument("-e", "--template", help="User template to determine whether a user should be kicked. E.g. @$SERVER_$NICK", required=True)


I would just do prefix matching to save yourself the hassle from trying to work out what to correctly parse things as. It also makes it clearer to the end-user what they should put as a template value ("@freenode_" for example). Bear in mind people who run this may not know the IRC bridge config file syntax so $SERVER and $NICK may be foreign to them.

Oh fair point. Matching by static prefix string makes sense.

…cesses, add summary of failures

kegsay · 2017-01-10T08:21:24Z

scripts/remove-idle-users.py

+    print("There are %s idle users in %s" % (len(user_ids), room_id))
+    for user_id in user_ids:
+        # Ignore users that do not start with the user_prefix
+        if not user_id.startswith(user_prefix):


We surely want the opposite here. I want to ignore users that start with @freenode_ because they are managed by the bridge.

The continue on the line after makes sure they are skipped

kegsay · 2017-01-10T08:25:59Z

scripts/remove-idle-users.py

+        res = requests.post(
+            homeserver + "/_matrix/client/r0/rooms/" + urllib.quote(room_id) + "/kick?access_token=" + token,
+            data = json.dumps({
+                "reason": reason,


If only it were that easy. Whilst the spec states that reason is a valid key, it looks like synapse doesn't abide by this.

Use this API instead. Will file an issue with Synapse peeps.

matrix-org/synapse#1788

kegsay · 2017-01-10T08:35:36Z

scripts/remove-idle-users.py

+            })
+        )
+        if res.status_code >= 400:
+            failure_responses.append({ "user_id": user_id, "response_json": res.json()})


No guarantee that the server returned JSON in this case. I'm guessing requests will throw if you call .json() in that case? No idea. We should handle it though (I'm thinking 504 Gateway Timeouts from the Apache).

kegsay · 2017-01-10T08:37:30Z

scripts/remove-idle-users.py

+            failure_responses.append({ "user_id": user_id, "response_json": res.json()})
+        else:
+            count += 1
+    print("Kicked %s users in total" % count)


More stats in general would be super useful for me:

How many kicks went through (count)

How many kicks were attempted (count + len(failure_responses))

How many kicks failed (len(failure_responses))

Probably tied together as:

Kicked 44/55 users in total

kegsay · 2017-01-10T08:38:25Z

scripts/remove-idle-users.py

+        return
+    print("Could not kick the following users:")
+    for failure in failure_responses:
+        print("%s : %s - %s" % (failure["user_id"], failure["response_json"]["error"], failure["response_json"]["errcode"]))


No guarantees that error and errcode even exist. It would be a shame to die when logging the failed requests. I would just log the dict.

kegsay · 2017-01-10T08:39:23Z

scripts/remove-idle-users.py

+    parser.add_argument("-u", "--homeserver", help="Base homeserver URL eg 'https://matrix.org'", required=True)
+    parser.add_argument("-d", "--domain", help="Domain of the homserver eg 'matrix.org'", required=True)
+    parser.add_argument("-s", "--since", type=int, help="Days since idle users have been offline for eg '30'", required=True)
+    parser.add_argument("-p", "--prefix", help="User prefix to determine whether a user should be kicked. E.g. @freenode_", required=True)


Technically the prefix isn't required?

It's possible that the bridge does have enough power to kick users that it does not have claim to, so I suppose it could be optional. Although I'm not really sure why an AS token would be used to kick real users.

kegsay · 2017-01-10T08:40:06Z

scripts/remove-idle-users.py

+    parser.add_argument("-t", "--token", help="The access token", required=True)
+    parser.add_argument("-a", "--alias", help="The alias of the room eg '#freenode_#matrix-dev:matrix.org'", required=True)
+    parser.add_argument("-u", "--homeserver", help="Base homeserver URL eg 'https://matrix.org'", required=True)
+    parser.add_argument("-d", "--domain", help="Domain of the homserver eg 'matrix.org'", required=True)


Why is this A Thing? You never use it.

ah, 'twas left over from the user template thing

- Removed homeserver_domain as it is not used - Use membership state API, as Synapse doesn't pass the reason on /kick - Handle non JSON body error responses - Improved logging

kegsay · 2017-01-10T14:12:01Z

scripts/remove-idle-users.py

+                failure["response_json"] = res.json()
+            except Exception as e:
+                print("Could not get JSON body from failure response: %s" % e)
+            failure_responses.append()


Appending nothing?

kegsay · 2017-01-10T14:14:15Z

scripts/remove-idle-users.py

        else:
            count += 1
-    print("Kicked %s users in total" % count)
+    print("Kicked %s/%s users in total (%s failed requests)" % (count, len(user_ids), len(failure_responses)))


My earlier comment was explicit for a reason: len(user_ids) will include ones you want to filter out according to prefix. It's probably fine since you're giving all the information required to work out the values, but it'll be confusing to see things like:

Kicked 5/10 users in total (2 failed requests)

and wonder what happened to the remaining 3.

oh, good point

kegsay · 2017-01-10T14:27:01Z

LGTM

lukebarnard1 requested a review from kegsay January 9, 2017 15:06

kegsay reviewed Jan 9, 2017

View reviewed changes

Code review

a88e7e8

kegsay reviewed Jan 9, 2017

View reviewed changes

Luke Barnard added 2 commits January 9, 2017 18:10

Code review

93e55d8

Use joined_members endpoint, use user_prefix, add a count to show suc…

896de7b

…cesses, add summary of failures

kegsay reviewed Jan 10, 2017

View reviewed changes

Code review response:

f8ec915

- Removed homeserver_domain as it is not used - Use membership state API, as Synapse doesn't pass the reason on /kick - Handle non JSON body error responses - Improved logging

kegsay reviewed Jan 10, 2017

View reviewed changes

Rejiggle summary log

0c36fc1

lukebarnard1 merged commit f81050b into develop Jan 10, 2017

ara4n mentioned this pull request Jan 11, 2017

The official network wishlist for matrix.org / Element #208

Open

jaller94 deleted the luke/script-remove-idle-users branch March 27, 2020 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add script to kick idle bridged users from a room #340

Add script to kick idle bridged users from a room #340

lukebarnard1 commented Jan 9, 2017

kegsay Jan 9, 2017

kegsay Jan 9, 2017

kegsay Jan 9, 2017

kegsay Jan 9, 2017

kegsay Jan 9, 2017

lukebarnard1 Jan 9, 2017

lukebarnard1 Jan 9, 2017

kegsay Jan 9, 2017

lukebarnard1 Jan 9, 2017

kegsay Jan 9, 2017

kegsay Jan 9, 2017

kegsay Jan 9, 2017

lukebarnard1 Jan 9, 2017

kegsay Jan 10, 2017

lukebarnard1 Jan 10, 2017

kegsay Jan 10, 2017

kegsay Jan 10, 2017

kegsay Jan 10, 2017

kegsay Jan 10, 2017

kegsay Jan 10, 2017

kegsay Jan 10, 2017

lukebarnard1 Jan 10, 2017

kegsay Jan 10, 2017

lukebarnard1 Jan 10, 2017

kegsay Jan 10, 2017

kegsay Jan 10, 2017

lukebarnard1 Jan 10, 2017

kegsay commented Jan 10, 2017

Add script to kick idle bridged users from a room #340

Add script to kick idle bridged users from a room #340

Conversation

lukebarnard1 commented Jan 9, 2017

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

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

Choose a reason for hiding this comment

kegsay commented Jan 10, 2017