wip commit - tests failing

5 files changed, 66 insertions, 53 deletions

diff --git a/synapse/api/auth.py b/synapse/api/auth.py
index 943a488339..d8ebbbc6e8 100644
--- a/synapse/api/auth.py
+++ b/synapse/api/auth.py
@@ -775,7 +775,7 @@ class Auth(object):
             )
 
     @defer.inlineCallbacks
-    def check_auth_blocking(self, error):
+    def check_auth_blocking(self):
         """Checks if the user should be rejected for some external reason,
         such as monthly active user limiting or global disable flag
         Args:
@@ -785,4 +785,6 @@ class Auth(object):
         if self.hs.config.limit_usage_by_mau is True:
             current_mau = yield self.store.get_monthly_active_count()
             if current_mau >= self.hs.config.max_mau_value:
-                raise error
+                raise AuthError(
+                    403, "MAU Limit Exceeded", errcode=Codes.MAU_LIMIT_EXCEEDED
+                )
diff --git a/synapse/storage/client_ips.py b/synapse/storage/client_ips.py
index 83d64d1563..2489527f2c 100644
--- a/synapse/storage/client_ips.py
+++ b/synapse/storage/client_ips.py
@@ -86,7 +86,7 @@ class ClientIpStore(background_updates.BackgroundUpdateStore):
             last_seen = self.client_ip_last_seen.get(key)
         except KeyError:
             last_seen = None
-        yield self._populate_monthly_active_users(user_id)
+        yield self.populate_monthly_active_users(user_id)
         # Rate-limited inserts
         if last_seen is not None and (now - last_seen) < LAST_SEEN_GRANULARITY:
             return
@@ -95,25 +95,6 @@ class ClientIpStore(background_updates.BackgroundUpdateStore):
 
         self._batch_row_update[key] = (user_agent, device_id, now)
 
-    @defer.inlineCallbacks
-    def _populate_monthly_active_users(self, user_id):
-        """Checks on the state of monthly active user limits and optionally
-        add the user to the monthly active tables
-
-        Args:
-            user_id(str): the user_id to query
-        """
-
-        store = self.hs.get_datastore()
-        if self.hs.config.limit_usage_by_mau:
-            is_user_monthly_active = yield store.is_user_monthly_active(user_id)
-            if is_user_monthly_active:
-                yield store.upsert_monthly_active_user(user_id)
-            else:
-                count = yield store.get_monthly_active_count()
-                if count < self.hs.config.max_mau_value:
-                    yield store.upsert_monthly_active_user(user_id)
-
     def _update_client_ips_batch(self):
         def update():
             to_update = self._batch_row_update
diff --git a/synapse/storage/monthly_active_users.py b/synapse/storage/monthly_active_users.py
index d06c90cbcc..6def6830d0 100644
--- a/synapse/storage/monthly_active_users.py
+++ b/synapse/storage/monthly_active_users.py
@@ -19,6 +19,10 @@ from synapse.util.caches.descriptors import cached, cachedInlineCallbacks
 
 from ._base import SQLBaseStore
 
+# Number of msec of granularity to store the monthly_active_user timestamp
+# This means it is not necessary to update the table on every request
+LAST_SEEN_GRANULARITY = 60 * 60 * 1000
+
 
 class MonthlyActiveUsersStore(SQLBaseStore):
     def __init__(self, dbconn, hs):
@@ -30,8 +34,9 @@ class MonthlyActiveUsersStore(SQLBaseStore):
         """
         Cleans out monthly active user table to ensure that no stale
         entries exist.
-        Return:
-            Defered()
+
+        Returns:
+            Deferred[]
         """
         def _reap_users(txn):
 
@@ -49,7 +54,7 @@ class MonthlyActiveUsersStore(SQLBaseStore):
                 """
             txn.execute(sql, (self.hs.config.max_mau_value,))
 
-        res = self.runInteraction("reap_monthly_active_users", _reap_users)
+        res = yield self.runInteraction("reap_monthly_active_users", _reap_users)
         # It seems poor to invalidate the whole cache, Postgres supports
         # 'Returning' which would allow me to invalidate only the
         # specific users, but sqlite has no way to do this and instead
@@ -57,16 +62,16 @@ class MonthlyActiveUsersStore(SQLBaseStore):
         # is racy.
         # Have resolved to invalidate the whole cache for now and do
         # something about it if and when the perf becomes significant
-        self.is_user_monthly_active.invalidate_all()
+        self._user_last_seen_monthly_active.invalidate_all()
         self.get_monthly_active_count.invalidate_all()
         return res
 
     @cached(num_args=0)
     def get_monthly_active_count(self):
-        """
-            Generates current count of monthly active users.abs
-            Return:
-                Defered(int): Number of current monthly active users
+        """Generates current count of monthly active users.abs
+
+        Returns:
+            Defered[int]: Number of current monthly active users
         """
 
         def _count_users(txn):
@@ -82,10 +87,10 @@ class MonthlyActiveUsersStore(SQLBaseStore):
             Updates or inserts monthly active user member
             Arguments:
                 user_id (str): user to add/update
-            Deferred(bool): True if a new entry was created, False if an
+            Deferred[bool]: True if a new entry was created, False if an
                 existing one was updated.
         """
-        self._simple_upsert(
+        is_insert = self._simple_upsert(
             desc="upsert_monthly_active_user",
             table="monthly_active_users",
             keyvalues={
@@ -96,24 +101,49 @@ class MonthlyActiveUsersStore(SQLBaseStore):
             },
             lock=False,
         )
-        self.is_user_monthly_active.invalidate((user_id,))
-        self.get_monthly_active_count.invalidate(())
+        if is_insert:
+            self._user_last_seen_monthly_active.invalidate((user_id,))
+            self.get_monthly_active_count.invalidate(())
 
     @cachedInlineCallbacks(num_args=1)
-    def is_user_monthly_active(self, user_id):
+    def _user_last_seen_monthly_active(self, user_id):
         """
             Checks if a given user is part of the monthly active user group
             Arguments:
                 user_id (str): user to add/update
             Return:
-                bool : True if user part of group, False otherwise
+                int : timestamp since last seen, None if never seen
+
         """
-        user_present = yield self._simple_select_onecol(
+        result = yield self._simple_select_onecol(
             table="monthly_active_users",
             keyvalues={
                 "user_id": user_id,
             },
-            retcol="user_id",
-            desc="is_user_monthly_active",
+            retcol="timestamp",
+            desc="_user_last_seen_monthly_active",
         )
-        defer.returnValue(bool(user_present))
+        timestamp = None
+        if len(result) > 0:
+            timestamp = result[0]
+        defer.returnValue(timestamp)
+
+    @defer.inlineCallbacks
+    def populate_monthly_active_users(self, user_id):
+        """Checks on the state of monthly active user limits and optionally
+        add the user to the monthly active tables
+
+        Args:
+            user_id(str): the user_id to query
+        """
+
+        if self.hs.config.limit_usage_by_mau:
+            last_seen_timestamp = yield self._user_last_seen_monthly_active(user_id)
+            now = self.hs.get_clock().time_msec()
+
+            if last_seen_timestamp is None:
+                count = yield self.get_monthly_active_count()
+                if count < self.hs.config.max_mau_value:
+                    yield self.upsert_monthly_active_user(user_id)
+            elif now - last_seen_timestamp > LAST_SEEN_GRANULARITY:
+                yield self.upsert_monthly_active_user(user_id)
diff --git a/tests/storage/test_client_ips.py b/tests/storage/test_client_ips.py
index e1552510cc..7a58c6eb24 100644
--- a/tests/storage/test_client_ips.py
+++ b/tests/storage/test_client_ips.py
@@ -64,7 +64,7 @@ class ClientIpStoreTestCase(tests.unittest.TestCase):
         yield self.store.insert_client_ip(
             user_id, "access_token", "ip", "user_agent", "device_id",
         )
-        active = yield self.store.is_user_monthly_active(user_id)
+        active = yield self.store._user_last_seen_monthly_active(user_id)
         self.assertFalse(active)
 
     @defer.inlineCallbacks
@@ -80,7 +80,7 @@ class ClientIpStoreTestCase(tests.unittest.TestCase):
         yield self.store.insert_client_ip(
             user_id, "access_token", "ip", "user_agent", "device_id",
         )
-        active = yield self.store.is_user_monthly_active(user_id)
+        active = yield self.store._user_last_seen_monthly_active(user_id)
         self.assertFalse(active)
 
     @defer.inlineCallbacks
@@ -88,13 +88,13 @@ class ClientIpStoreTestCase(tests.unittest.TestCase):
         self.hs.config.limit_usage_by_mau = True
         self.hs.config.max_mau_value = 50
         user_id = "@user:server"
-        active = yield self.store.is_user_monthly_active(user_id)
+        active = yield self.store._user_last_seen_monthly_active(user_id)
         self.assertFalse(active)
 
         yield self.store.insert_client_ip(
             user_id, "access_token", "ip", "user_agent", "device_id",
         )
-        active = yield self.store.is_user_monthly_active(user_id)
+        active = yield self.store._user_last_seen_monthly_active(user_id)
         self.assertTrue(active)
 
     @defer.inlineCallbacks
@@ -103,7 +103,7 @@ class ClientIpStoreTestCase(tests.unittest.TestCase):
         self.hs.config.max_mau_value = 50
         user_id = "@user:server"
 
-        active = yield self.store.is_user_monthly_active(user_id)
+        active = yield self.store._user_last_seen_monthly_active(user_id)
         self.assertFalse(active)
 
         yield self.store.insert_client_ip(
@@ -112,5 +112,5 @@ class ClientIpStoreTestCase(tests.unittest.TestCase):
         yield self.store.insert_client_ip(
             user_id, "access_token", "ip", "user_agent", "device_id",
         )
-        active = yield self.store.is_user_monthly_active(user_id)
+        active = yield self.store._user_last_seen_monthly_active(user_id)
         self.assertTrue(active)
diff --git a/tests/storage/test_monthly_active_users.py b/tests/storage/test_monthly_active_users.py
index c32109ecc5..0bfd24a7fb 100644
--- a/tests/storage/test_monthly_active_users.py
+++ b/tests/storage/test_monthly_active_users.py
@@ -40,18 +40,18 @@ class MonthlyActiveUsersTestCase(tests.unittest.TestCase):
         self.assertEqual(1, count)
 
     @defer.inlineCallbacks
-    def test_is_user_monthly_active(self):
+    def test__user_last_seen_monthly_active(self):
         user_id1 = "@user1:server"
         user_id2 = "@user2:server"
         user_id3 = "@user3:server"
-        result = yield self.store.is_user_monthly_active(user_id1)
-        self.assertFalse(result)
+        result = yield self.store._user_last_seen_monthly_active(user_id1)
+        self.assertFalse(result == 0)
         yield self.store.upsert_monthly_active_user(user_id1)
         yield self.store.upsert_monthly_active_user(user_id2)
-        result = yield self.store.is_user_monthly_active(user_id1)
-        self.assertTrue(result)
-        result = yield self.store.is_user_monthly_active(user_id3)
-        self.assertFalse(result)
+        result = yield self.store._user_last_seen_monthly_active(user_id1)
+        self.assertTrue(result > 0)
+        result = yield self.store._user_last_seen_monthly_active(user_id3)
+        self.assertFalse(result == 0)
 
     @defer.inlineCallbacks
     def test_reap_monthly_active_users(self):