From 788ffaf298ad94f670516a2f51aed82d52f1de3c Mon Sep 17 00:00:00 2001 From: Shankari Date: Fri, 7 Feb 2025 11:30:21 -0800 Subject: [PATCH] =?UTF-8?q?=F0=9F=92=A9=20=F0=9F=94=A7=20Enable=20suspendi?= =?UTF-8?q?ng=20data=20collection=20for=20certain=20subgroups?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This revises the initial implementation in https://github.com/e-mission/e-mission-server/pull/1016 of a hack for e-mission/e-mission-docs#1104 The request in the issue was to implement the ability to suspend and resume data collection. The previous implementation checked for suspended subgroups in the auth method, and used the `request.path` to determine whether it was a `usercache/put` call or not This was needed because, by design, we only see the opcode in the auth module; the rest of the code works only with UUIDs The problem with that approach was that if the subgroup was suspended, we returned a UUID of None, which triggered a 403 error, which, in turn, caused the phone app to detect a failed call, so it did not delete the entries locally. https://github.com/e-mission/e-mission-server/pull/1016#issuecomment-2642123756 https://github.com/e-mission/e-mission-server/pull/1016#issuecomment-2643329374 We fix this by returning the non-random parts of the opcode (at least the subgroup) in a context, and perform the validation downstream (in the `putIntoCache` method). So if the subgroup is suspended, `putIntoCache` just returns early instead of storing the values. Note that this is also a bit of a hack, given that the way we handle the dynamic auth right now is to reset the global `auth_method`!! So we can't use a check for `auth_method == 'dynamic'` anywhere outside `resolve_auth` and have to use the existence of the dynamic config as the check for whether the auth method was dynamic or not Testing done: - TestWebserver.py passes - TestAuthSelection.py does not need any changes since we made sure to be backwards compatible When trying to push to a suspended subgroup, we get ``` 2025-02-07 11:10:26,127:DEBUG:6362165248:START POST /usercache/put 2025-02-07 11:10:26,127:DEBUG:6362165248:Called userCache.put 2025-02-07 11:10:26,189:DEBUG:6362165248:methodName = skip, returning 2025-02-07 11:10:26,189:DEBUG:6362165248:Using the skip method to verify id token nrelop_dev- emulator-study_default_123 of length 37 2025-02-07 11:10:26,190:DEBUG:6362165248:subgroup default found in list ['test', 'default'] 2025-02-07 11:10:26,190:DEBUG:6362165248:retContext = {'subgroup': 'default', 'user_id': UUID ('eb4a7aae-f2d4-4480-ba85-c568a45591b5')} 2025-02-07 11:10:26,191:DEBUG:6362165248:user_uuid {'subgroup': 'default', 'user_id': UUID('e b4a7aae-f2d4-4480-ba85-c568a45591b5')} 2025-02-07 11:10:26,191:DEBUG:6362165248:suspended_subgroups=['default'] 2025-02-07 11:10:26,191:INFO:6362165248:Received put message for subgroup default in suspende d_subgroups=['default'], ignoring 2025-02-07 11:10:26,191:DEBUG:6362165248:END POST /usercache/put eb4a7aae-f2d4-4480-ba85-c568 ``` When trying to push to a non-suspended subgroup, we get ``` 2025-02-07 11:26:13,627:DEBUG:12901707776:START POST /usercache/put 2025-02-07 11:26:13,628:DEBUG:12901707776:Called userCache.put 2025-02-07 11:26:13,631:DEBUG:12901707776:methodName = skip, returning 2025-02-07 11:26:13,631:DEBUG:12901707776:Using the skip method to verify id token nrelop_dev-emulator-study_test_123 of length 34 2025-02-07 11:26:13,634:DEBUG:12901707776:subgroup test found in list ['test', 'default'] 2025-02-07 11:26:13,634:DEBUG:12901707776:retContext = {'subgroup': 'test', 'user_id': UUID('feb70456-abf4-444b-8848-1515fc3470cf')} 2025-02-07 11:26:13,634:DEBUG:12901707776:user_uuid {'subgroup': 'test', 'user_id': UUID('feb70456-abf4-444b-8848-1515fc3470cf')} 2025-02-07 11:26:13,634:DEBUG:12901707776:suspended_subgroups=['default'] 2025-02-07 11:26:13,644:DEBUG:12901707776:Updated result for user = feb70456-abf4-444b-8848-1515fc3470cf, key = stats/client_time, write_ts = 1738956332.243175 = {'n': 1, 'nModified': 0, 'upserted': ObjectId('67a65e5546756f8244f024f6'), 'ok': 1.0, 'updatedExisting': False} 2025-02-07 11:26:13,646:DEBUG:12901707776:Updated result for user = feb70456-abf4-444b-8848-1515fc3470cf, key = stats/client_time, write_ts = 1738956332.246808 = {'n': 1, 'nModified': 0, 'upserted': ObjectId('67a65e5546756f8244f024f8'), 'ok': 1.0, 'updatedExisting': False} 2025-02-07 11:26:13,857:DEBUG:12901707776:Updated result for user = feb70456-abf4-444b-8848-1515fc3470cf, key = statemachine/transition, write_ts = 1738956373.598038 = {'n': 1, 'nModified': 0, 'upserted': ObjectId('67a65e5546756f8244f025cd'), 'ok': 1.0, 'updatedExisting': False} 2025-02-07 11:26:13,858:DEBUG:12901707776:Updated result for user = feb70456-abf4-444b-8848-1515fc3470cf, key = background/battery, write_ts = 1738956373.5991821 = {'n': 1, 'nModified': 0, 'upserted': ObjectId('67a65e5546756f8244f025cf'), 'ok': 1.0, 'updatedExisting': False} 2025-02-07 11:26:13,859:DEBUG:12901707776:Updated result for user = feb70456-abf4-444b-8848-1515fc3470cf, key = stats/client_nav_event, write_ts = 1738956373.607026 = {'n': 1, 'nModified': 0, 'upserted': ObjectId('67a65e5546756f8244f025d1'), 'ok': 1.0, 'updatedExisting': False} 2025-02-07 11:26:13,860:DEBUG:12901707776:END POST /usercache/put feb70456-abf4-444b-8848-1515fc3470cf 0.2325270175933838 ``` --- emission/net/api/cfc_webapp.py | 28 +++++++++++++------- emission/net/auth/auth.py | 18 +++++++------ emission/tests/netTests/TestAuthSelection.py | 6 ++--- 3 files changed, 32 insertions(+), 20 deletions(-) diff --git a/emission/net/api/cfc_webapp.py b/emission/net/api/cfc_webapp.py index 99864b98c..dbf1c1c5a 100644 --- a/emission/net/api/cfc_webapp.py +++ b/emission/net/api/cfc_webapp.py @@ -244,10 +244,16 @@ def getFromCache(): @post('/usercache/put') def putIntoCache(): logging.debug("Called userCache.put") - user_uuid=getUUID(request) - logging.debug("user_uuid %s" % user_uuid) - from_phone = request.json['phone_to_server'] - return usercache.sync_phone_to_server(user_uuid, from_phone) + user_context=getUUID(request, return_context=True) + logging.debug("user_uuid %s" % user_context) + suspended_subgroups = dynamic_config.get("opcode", {}).get("suspended_subgroups", []) + logging.debug(f"{suspended_subgroups=}") + curr_subgroup = user_context.get('subgroup', None) + if curr_subgroup in suspended_subgroups: + logging.info(f"Received put message for subgroup {curr_subgroup} in {suspended_subgroups=}, ignoring") + else: + from_phone = request.json['phone_to_server'] + usercache.sync_phone_to_server(user_context['user_id'], from_phone) @post('/usercache/putone') def putIntoOneEntry(): @@ -531,13 +537,17 @@ def get_user_or_aggregate_auth(request): logging.debug(f"Aggregate call, checking {aggregate_call_auth} policy") return aggregate_call_map[aggregate_call_auth](request) -def getUUID(request, inHeader=False): +def getUUID(request, inHeader=False, return_context=False): try: - retUUID = enaa.getUUID(dynamic_config, request, auth_method, inHeader) - logging.debug("retUUID = %s" % retUUID) - if retUUID is None: + retContext = enaa.getUUID(request, auth_method, inHeader, dynamic_config) + logging.debug("retUUID = %s" % retContext) + if retContext is None: raise HTTPError(403, "token is valid, but no account found for user") - return retUUID + + if return_context: + return retContext + else: + return retContext['user_id'] except ValueError as e: traceback.print_exc() abort(403, e) diff --git a/emission/net/auth/auth.py b/emission/net/auth/auth.py index 029d5ac20..bf01f5269 100644 --- a/emission/net/auth/auth.py +++ b/emission/net/auth/auth.py @@ -54,7 +54,7 @@ def __getToken__(request, inHeader): return userToken def getSubgroupFromToken(token, config): - if "opcode" in config: + if config is not None and "opcode" in config: # new style study, expects token with sub-group tokenParts = token.split('_'); if len(tokenParts) <= 3: @@ -83,18 +83,20 @@ def getSubgroupFromToken(token, config): logging.debug('Old-style study, expecting token without a subgroup...'); return None; -def getUUID(dynamicConfig, request, authMethod, inHeader=False): +def getUUID(request, authMethod, inHeader=False, dynamicConfig = None): retUUID = None userToken = __getToken__(request, inHeader) curr_subgroup = getSubgroupFromToken(userToken, dynamicConfig) - suspended_subgroups = dynamicConfig.get("opcode", {}).get("suspended_subgroups", []) - if request.path == "/usercache/put": - if curr_subgroup in suspended_subgroups: - logging.info(f"Received put message for subgroup {curr_subgroup} in {suspended_subgroups=}, returning uuid = None") - return None retUUID = getUUIDFromToken(authMethod, userToken) request.params.user_uuid = retUUID - return retUUID + # TODO: We should really think about how long we want to continue supporting + # non-dynamic config. Check with community on who's using it and what they need. + if dynamicConfig is not None: + subgroup = getSubgroupFromToken(userToken, dynamicConfig) + return {"subgroup": subgroup, "user_id": retUUID} + else: + # if the authmethod is "skip" or "token_list", the token can be in any format + return retUUID # Should only be used by the profile creation code, since we may not have a # UUID yet. All others should only use the UUID. diff --git a/emission/tests/netTests/TestAuthSelection.py b/emission/tests/netTests/TestAuthSelection.py index 1d3c72d32..b7161695e 100644 --- a/emission/tests/netTests/TestAuthSelection.py +++ b/emission/tests/netTests/TestAuthSelection.py @@ -143,7 +143,7 @@ def testGetUUIDSkipAuth(self): logging.debug("Found request body = %s" % request.body.getvalue()) logging.debug("Found request headers = %s" % list(request.headers.keys())) user = ecwu.User.register(self.test_email) - self.assertEqual(enaa.getUUID({}, request, "skip", inHeader=False), user.uuid) + self.assertEqual(enaa.getUUID(request, "skip", inHeader=False), user.uuid) ecwu.User.unregister(self.test_email) def testGetUUIDTokenAuthSuccess(self): @@ -161,7 +161,7 @@ def testGetUUIDTokenAuthSuccess(self): logging.debug("Found request body = %s" % request.body.getvalue()) logging.debug("Found request headers = %s" % list(request.headers.keys())) user = ecwu.User.register(self.test_email) - self.assertEqual(enaa.getUUID({}, request, "token_list", inHeader=False), user.uuid) + self.assertEqual(enaa.getUUID(request, "token_list", inHeader=False), user.uuid) ecwu.User.unregister(self.test_email) def testGetUUIDTokenAuthFailure(self): @@ -180,7 +180,7 @@ def testGetUUIDTokenAuthFailure(self): user = ecwu.User.register(self.test_email) ecwu.User.unregister(self.test_email) with self.assertRaises(ValueError): - self.assertEqual(enaa.getUUID({}, request, "token_list", inHeader=False), user.uuid) + self.assertEqual(enaa.getUUID(request, "token_list", inHeader=False), user.uuid) if __name__ == '__main__': import emission.tests.common as etc