Enforce limit on number of devices per user #35515
Conversation
gherceg
force-pushed
the
gh/devices-per-user
branch
from
53dc172 to
6d87fb4
Compare
Admins can trigger restores from HQ without a device_id
gherceg
force-pushed
the
gh/devices-per-user
branch
from
c164e3a to
2af1f45
Compare
gherceg
changed the title
There was a problem hiding this comment.
Thanks for this @gherceg. Do you have a sense whether blocking the restore is the best option or whether we should also block any other endpoints like phone_keys or app_install? I think we can't block app install because it's unauthenticated right?
Is the logic for blocking sync that you really use a new device until you've done an initial sync?
| @@ -0,0 +1 @@ | |||
| DEVICES_PER_USER = 100 | |||
There was a problem hiding this comment.
How about making this a setting to that it can be changed without a code deploy and 3rd parties can configure their own limits.
On the topic of 3rd parties, this change probably warrants a changelog entry.
| self._create_synclog(self.domain, 'abc123', 'device-id', date=self.within_past_day) | ||
|
|
||
| with patch('corehq.apps.ota.utils.DEVICES_PER_USER', 1): | ||
| self.assertTrue(can_login_on_device('abc123', 'device-id')) |
There was a problem hiding this comment.
Is this meant to be None?
| self.assertTrue(can_login_on_device('abc123', 'device-id')) | |
| self.assertTrue(can_login_on_device('abc123', None)) |
| if device_id.startswith("WebAppsLogin"): | ||
| return True | ||
|
|
||
| end_time = datetime.now() |
There was a problem hiding this comment.
Should this use utcnow() or whatever the acceptable equivalent is these days?
Aside: I find it annoying that utcnow() is deprecated. It's so easy to remember.
|
|
||
|
|
||
| def can_login_on_device(user_id, device_id): | ||
| if not device_id or device_id.startswith("WebAppsLogin"): |
There was a problem hiding this comment.
Can device ids be set by the submitting device? In other words, would it be possible for someone to make their mobile device(s) ids start with WebAppsLogin or be empty and exempt themselves from the limit? Do we care?
Product Description
Technical Summary
https://dimagi.atlassian.net/browse/SAAS-16355
This returns a 403 if a user is attempting to restore from a new device id after exceeding the device limit. If they have used the device within the last 24 hours, they will be allowed to use it even if the device limit is exceeded.
The investigation done in this ticket highlighted that the distribution of device counts per user is mostly made up of 5 or below (taken from one random day):
It seems reasonable to set a very conservative device limit to start since the user experience when hitting this error isn't going to be great until the mobile team is able to handle this error specifically.
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Rollback instructions
Labels & Review