Skip to content
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

Can register using an email address via identity server #1034

Closed
wants to merge 11 commits into from
Closed

Can register using an email address via identity server #1034

wants to merge 11 commits into from

Conversation

PiotrKozimor
Copy link
Contributor

@PiotrKozimor PiotrKozimor commented Apr 21, 2021

New test case. Why is it neccessary? Test case Can register using an email address expect homeserver to send email. There is currently no way to test configuration when identity server is requested to send email (by /requestToken endpoint) to mocked identity server. When received, email is considered verified (the part with /submitToken is skipped).

There is backward-compatible change in configuration provided to Dendrite. Is is described in PR for Dendrite

Signed-off-by: Piotr Kozimor <p1996k@gmail.com>

@PiotrKozimor PiotrKozimor marked this pull request as ready for review April 21, 2021 16:42
tests/11register.pl Outdated Show resolved Hide resolved
@PiotrKozimor
Copy link
Contributor Author

@richvdh how do I request review on this PR?

@clokep clokep requested a review from a team April 22, 2021 11:46
@clokep
Copy link
Member

clokep commented Apr 22, 2021

how do I request review on this PR?

I've set the review flag for you! 😄

Copy link
Member

@erikjohnston erikjohnston left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm failing to remember quite how the registration stuff works, so I'll leave for someone else to review properly.

@@ -54,6 +54,7 @@
binmode(STDOUT, ":utf8");

our $BIND_HOST = "localhost";
our $ID_SERVER_PORT = 34586;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we now need to set an explicit port ooi?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that it can be added to trusted servers in homeserver configuration. I suppose that each time id_server_fixture is called random port is assigned.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, I'm blind. We don't need it for Synapse because there's an option to trust all ID servers, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is quite messy in Synapse. For example in /register/email/requestToken id_server field will be just ignored. I could not figure out how does it work in user-interactive authentication. I didn't either found option to trust all ID servers in Synapse.

tests/11register.pl Outdated Show resolved Hide resolved
tests/11register.pl Outdated Show resolved Hide resolved
@erikjohnston
Copy link
Member

Is this blocked until matrix-org/dendrite#1837 lands?

@erikjohnston erikjohnston requested a review from a team April 26, 2021 14:27
PiotrKozimor and others added 2 commits April 26, 2021 16:43
Co-authored-by: Erik Johnston <erikj@jki.re>
Co-authored-by: Erik Johnston <erikj@jki.re>
@PiotrKozimor
Copy link
Contributor Author

Is this blocked until matrix-org/dendrite#1837 lands?

Good question, I think this PR and the one mentioned by you blocks each other. I can think of following way to resolve this:

  • Overwrite dendrite branch in sytest CI,
  • Merge this PR once all checks are successful,
  • Merge dendrite PR with check run against default sytest branch.

BTW this test case is not whitelisted in dendrite nor synapse for now - only in this branch

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I wrote on matrix-org/dendrite#1837, I'm not sure that this is something we should be introducing in new code. In particular, the concept of a "trusted" identity server is pretty broken. It's likely we'll remove that option soon in Synapse (matrix-org/synapse#5881), and we certainly shouldn't be adding more code that relies on it.

lib/SyTest/Homeserver/Dendrite.pm Outdated Show resolved Hide resolved
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@PiotrKozimor
Copy link
Contributor Author

We want to stick with HS sending validation emails - see this issue, so closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants