Neilj/fix threepid auth check #4435
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #4435 +/- ##
==========================================
- Coverage 73.7% 73.7% -0.01%
==========================================
Files 300 300
Lines 29705 29707 +2
Branches 4882 4883 +1
==========================================
+ Hits 21895 21896 +1
+ Misses 6385 6382 -3
- Partials 1425 1429 +4 |
There was a problem hiding this comment.
I note that one of the two callsites already has a None-guard, and I have to say I wonder if we shouldn't be adding a guard to the other, rather than adding the guard to is_threepid_reserved, which makes the behaviour of the latter a bit weird.
I could live with doing it your way, but if you're going to do it, please can you update the docstring on is_threepid_reserved to document the behaviour when threepid is None.
| """Check the threepid against the reserved threepid config | ||
| Args: | ||
| config(ServerConfig) - to access server config attributes | ||
| reserved_threepids([dict]) - list of reserved threepids | ||
| threepid(dict) - The threepid to test for |
There was a problem hiding this comment.
evidently this can also be not a dict?
There was a problem hiding this comment.
Okay have moved the guard out of the method.
| @@ -416,8 +416,11 @@ def on_POST(self, request): | |||
| ) | |||
| # Necessary due to auth checks prior to the threepid being | |||
| # written to the db | |||
| if is_threepid_reserved(self.hs.config, threepid): | |||
| yield self.store.upsert_monthly_active_user(registered_user_id) | |||
| if threepid: | |||
There was a problem hiding this comment.
for future ref: I'd be inclined to merge these into one if statement, to reduce indentation.
There was a problem hiding this comment.
yeah I was in two minds - I tried it on one line but it seemed less readable which I why I went with the indent. Noted for future
If mau_limits_reserved_threepids is populated but the threepid to test is None the method will barf. This PRs adds a guard to prevent this.