New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Neilj/fix threepid auth check #4435

Merged
merged 2 commits into from Jan 24, 2019

Conversation

Projects
None yet
3 participants
@neilisfragile
Copy link
Contributor

neilisfragile commented Jan 22, 2019

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.

@neilisfragile neilisfragile changed the base branch from master to develop Jan 22, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 22, 2019

Codecov Report

Merging #4435 into develop will decrease coverage by <.01%.
The diff coverage is 85.71%.

@@            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

@neilisfragile neilisfragile requested a review from matrix-org/synapse-core Jan 22, 2019

@richvdh
Copy link
Member

richvdh left a 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

This comment has been minimized.

@richvdh

richvdh Jan 22, 2019

Member

evidently this can also be not a dict?

This comment has been minimized.

@neilisfragile

neilisfragile Jan 22, 2019

Author Contributor

Okay have moved the guard out of the method.

@richvdh richvdh self-requested a review Jan 24, 2019

@richvdh
Copy link
Member

richvdh left a comment

lgtm

@@ -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:

This comment has been minimized.

@richvdh

richvdh Jan 24, 2019

Member

for future ref: I'd be inclined to merge these into one if statement, to reduce indentation.

This comment has been minimized.

@neilisfragile

neilisfragile Jan 24, 2019

Author Contributor

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

@neilisfragile neilisfragile merged commit 10b89d5 into develop Jan 24, 2019

5 checks passed

ci/circleci: sytestpy2merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy2postgresmerged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3merged Your tests passed on CircleCI!
Details
ci/circleci: sytestpy3postgresmerged Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment