Skip to content
This repository has been archived by the owner on Nov 25, 2024. It is now read-only.

Extend UserAPI to handle threepid validation sessions #1970

Conversation

PiotrKozimor
Copy link
Contributor

@PiotrKozimor PiotrKozimor commented Aug 11, 2021

  • UserInternalAPI is extended to handle validation sessions.
  • Tests are implemented for new methods using testMailer.
  • Tests for storage are written using sqlite3 with in-memory mode.
  • Sending emails is abstracted with Mailer interface.
  • SmtpMailer implementation of Mailer is provided.
  • UserAPI config is extended to configure SmtpMailer and email templates path.
  • ThreepidSessionType is defined. Three different templates are defined - account_password, account_threepid, register for respective /requestToken endpoints

Uncertainties

  • sqlite3 and postgres Database implementations share statements but diverge on schema definition. Is that ok?
  • New token is not generated when /requestToken is made for the same client_secret and email pair with sent_attempt bumped (both synapse and dendrite generate new tokens). I did not see any rationale behind generating new one. Proposed approach requires one SQL table instead of two.
  • github.com/matryer/is was used as extension to go testing framework. I found it minimal yet powerful to work with. Can we accept it as a test dependency?
  • I have tested SmtpMailer manually by calling docker run --rm -p 25:25 -v $(pwd)/emails:/var/lib/mock-smtp -d flaviovs/mock-smtp in userapi/mail and then go test -tags=test_mail and inspecting output manually. Can we automate these tests in Buildkite?

What's next

  • We can handle msisdn in simple way - just send sms instead of email in CreateSession
  • Implement routing in Client API
  • Implement registering and login with email

Pull Request Checklist

  • I have added any new tests that need to pass to sytest-whitelist as specified in docs/sytest.md
  • Pull request includes a sign off

Signed-off-by: Piotr Kozimor <[email protected]>

@PiotrKozimor
Copy link
Contributor Author

I made a progress with CI checks. I struggle however to understand why Local device key changes appear in /keys/changes test fails. I see that there are similar errors for latest commit in master: https://buildkite.com/matrix-dot-org/dendrite/builds/5441. @kegsay can you help?

@kegsay
Copy link
Member

kegsay commented Sep 10, 2021

Local device key changes appear in /keys/changes is a flakey test annoyingly, so feel free to ignore it. It is already ignored https://github.com/matrix-org/dendrite/blob/master/sytest-blacklist#L75 but for some reason sytest doesn't pick this up.

@PiotrKozimor
Copy link
Contributor Author

Nice, thanks! I think this PR is ready to be reviewed - I want to route extended User API in separate PR. Can you please refer to Uncertainties section? Also, sytest are failing now on Inviting an AS-hosted user asks the AS server, looks like join event for wrong room is received. I am not sure how to fix it. In logs there is a line: WARN: Ignoring incoming AS event of type m.room.member - maybe the right event was ignored?

@PiotrKozimor
Copy link
Contributor Author

PiotrKozimor commented Sep 16, 2021

Can login with 3pid and password using m.login.password fails because no email configuration is provided. Solved in matrix-org/sytest#1143

@PiotrKozimor
Copy link
Contributor Author

Sytest is passing 🎉 I have failure in Upgrades test. I will try to fix it soon.

@neilalexander
Copy link
Contributor

I was trying to merge the latest master commits into this PR to re-run it through CI, but it seems there's a protected branch restriction in place in your fork that prevents me from doing so?

@PiotrKozimor
Copy link
Contributor Author

I have played with it today, please check now, should be fixed. Sorry for inconvenience.

@neilalexander neilalexander requested a review from a team as a code owner January 6, 2022 09:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants