Skip to content

Conversation

babolivier
Copy link
Contributor

This work is happening in the context of the Tchap mainlining. In Tchap, when a user is registered (which always requires an email to be set), the email is automatically bound to a Sydent instance using the internal API (so that their email can be looked up immediately, and without having to go through additional manual validation).

This module implements this behaviour by using the on_user_registration account validity callback, as well as a module API method added in matrix-org/synapse#12195.

The CI is expecting to fail since matrix-org/synapse#12195 hasn't made it into mainline CI yet, but it's all passing locally.

@babolivier babolivier requested a review from a team March 21, 2022 17:27
reivilibre
reivilibre previously approved these changes Mar 23, 2022
Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

Looks good to me, I assume you've tested it works, so the comment is really just a question about how it works (with perhaps a point that it might be nice to document it readily, as it seems not immediately obvious).

@babolivier
Copy link
Contributor Author

babolivier commented Mar 25, 2022

I've rewritten a bit of the module so it now uses the callback to matrix-org/synapse#12302, and binds assocs when they're verified on the HS rather than only on registration.
CI is still failing (because none of the PRs have made it into a stable release of Synapse), but I've ran the CI against a local branch that has both PRs merged in and it's passing fine:

$ ./scripts-dev/lint.sh 
+ isort synapse_bind_sydent tests
+ python3 -m black synapse_bind_sydent tests
All done! ✨ 🍰 ✨
3 files left unchanged.
+ flake8 synapse_bind_sydent tests
+ mypy synapse_bind_sydent tests
Success: no issues found in 3 source files

@babolivier babolivier requested a review from reivilibre March 25, 2022 18:48
@babolivier babolivier dismissed reivilibre’s stale review March 25, 2022 18:57

Code has changed quite a bit since the review

Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

Looks fine otherwise, I think!

@babolivier babolivier requested a review from reivilibre March 31, 2022 14:57
Copy link
Contributor

@reivilibre reivilibre left a comment

Choose a reason for hiding this comment

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

Seems to require reformatting, but AST-wise this is fine

Edit: actually it looks like black might be broken? Hrmph.

@babolivier babolivier merged commit 6d91119 into main Apr 5, 2022
@babolivier babolivier deleted the babolivier/init branch April 5, 2022 10:57
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.

2 participants