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

Already on GitHub? Sign in to your account

Add requestToken endpoints #915

Merged
merged 6 commits into from Jul 13, 2016

Conversation

Projects
None yet
2 participants

@erikjohnston erikjohnston changed the title from Implement https://github.com/matrix-org/matrix-doc/pull/346/files to Add requestToken endpoints Jul 8, 2016

@dbkr dbkr referenced this pull request in matrix-org/matrix-react-sdk Jul 8, 2016

Merged

Use HS proxy API for requestToken on adding email #336

@erikjohnston erikjohnston commented on an outdated diff Jul 8, 2016

synapse/rest/client/v2_alpha/account.py
@defer.inlineCallbacks
def on_POST(self, request):
yield run_on_reactor()
+ if '/account/password/email/requestToken' in request.path:
@erikjohnston

erikjohnston Jul 8, 2016

Owner

Why aren't these just separate handlers?

@erikjohnston erikjohnston commented on an outdated diff Jul 8, 2016

synapse/rest/client/v2_alpha/account.py
+ logger.error("hi")
+
+ required = ['id_server', 'client_secret', 'email', 'send_attempt']
+ absent = []
+ for k in required:
+ if k not in body:
+ absent.append(k)
+
+ if len(absent) > 0:
+ raise SynapseError(400, "Missing params: %r" % absent, Codes.MISSING_PARAM)
+
+ existingUid = yield self.hs.get_datastore().get_user_id_by_threepid(
+ 'email', body['email']
+ )
+
+ logger.error("existing %r", existingUid)
@erikjohnston

erikjohnston Jul 8, 2016

Owner

Left over debug logs?

@erikjohnston

erikjohnston Jul 11, 2016

Owner

Is this a thing?

@dbkr dbkr referenced this pull request in matrix-org/matrix-react-sdk Jul 8, 2016

Merged

Error if email already in use when resetting pw #337

dbkr added some commits Jul 11, 2016

Member

dbkr commented Jul 11, 2016

ptal

@erikjohnston erikjohnston commented on an outdated diff Jul 11, 2016

synapse/rest/client/v2_alpha/account.py
+ def __init__(self, hs):
+ super(PasswordRequestTokenRestServlet, self).__init__()
+ self.hs = hs
+ self.identity_handler = hs.get_handlers().identity_handler
+
+ @defer.inlineCallbacks
+ def on_POST(self, request):
+ body = parse_json_object_from_request(request)
+
+ required = ['id_server', 'client_secret', 'email', 'send_attempt']
+ absent = []
+ for k in required:
+ if k not in body:
+ absent.append(k)
+
+ if len(absent) > 0:
@erikjohnston

erikjohnston Jul 11, 2016

Owner

Nit: Its more pythonic to just do if absent:

@erikjohnston erikjohnston and 1 other commented on an outdated diff Jul 11, 2016

synapse/rest/client/v2_alpha/account.py
+
+ if len(absent) > 0:
+ raise SynapseError(400, "Missing params: %r" % absent, Codes.MISSING_PARAM)
+
+ existingUid = yield self.hs.get_datastore().get_user_id_by_threepid(
+ 'email', body['email']
+ )
+
+ if existingUid is None:
+ raise SynapseError(400, "Email not found", Codes.THREEPID_NOT_FOUND)
+
+ ret = yield self.identity_handler.requestEmailToken(**body)
+ defer.returnValue((200, ret))
+
+ def on_OPTIONS(self, _):
+ return 200, {}
@erikjohnston

erikjohnston Jul 11, 2016

Owner

on_OPTIONS should be handled automatically?

@dbkr

dbkr Jul 12, 2016

Member

Ah, good to know, just copied it from the other handlers.

@erikjohnston erikjohnston commented on the diff Jul 11, 2016

synapse/rest/client/v2_alpha/register.py
+ absent = []
+ for k in required:
+ if k not in body:
+ absent.append(k)
+
+ if len(absent) > 0:
+ raise SynapseError(400, "Missing params: %r" % absent, Codes.MISSING_PARAM)
+
+ existingUid = yield self.hs.get_datastore().get_user_id_by_threepid(
+ 'email', body['email']
+ )
+
+ if existingUid is not None:
+ raise SynapseError(400, "Email is already in use", Codes.THREEPID_IN_USE)
+
+ ret = yield self.identity_handler.requestEmailToken(**body)
@erikjohnston

erikjohnston Jul 11, 2016

Owner

Does this not explode if there are extra keys in the body?

@dbkr

dbkr Jul 12, 2016

Member

requestEmailToken takes **kwargs so it shouldn't do (next_link is passed through this way)

dbkr added some commits Jul 12, 2016

Owner

erikjohnston commented Jul 13, 2016

LGTM

Owner

erikjohnston commented Jul 13, 2016

(Is this sytest'ed or even sytest'able?)

Member

dbkr commented Jul 13, 2016

It would be tricky to sytest: currently we could test if it rejected appropriately, but since we allow the HS to HTTP 200 the request and tell the user by email that the email is already taken / not found, the tests would then fail if an HS did that.

@dbkr dbkr merged commit a37ee22 into develop Jul 13, 2016

9 of 10 checks passed

Sytest Dendron (Merged PR) Build finished.
Details
Flake8 + Packaging (Commit) Build #1123 origin/dbkr/more_requesttokens succeeded in 51 sec
Details
Flake8 + Packaging (Merged PR) Build finished.
Details
Sytest Dendron (Commit) Build #237 origin/dbkr/more_requesttokens succeeded in 6 min 0 sec
Details
Sytest Postgres (Commit) Build #1077 origin/dbkr/more_requesttokens succeeded in 6 min 1 sec
Details
Sytest Postgres (Merged PR) Build finished.
Details
Sytest SQLite (Commit) Build #1097 origin/dbkr/more_requesttokens succeeded in 5 min 56 sec
Details
Sytest SQLite (Merged PR) Build finished.
Details
Unit Tests (Commit) Build #1162 origin/dbkr/more_requesttokens succeeded in 1 min 33 sec
Details
Unit Tests (Merged PR) Build finished.
Details

@richvdh richvdh deleted the dbkr/more_requesttokens branch Dec 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment