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

First cut of the module #1

Merged
merged 12 commits into from
Apr 5, 2022
Merged

First cut of the module #1

merged 12 commits into from
Apr 5, 2022

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).

README.md Outdated Show resolved Hide resolved
synapse_bind_sydent/__init__.py Outdated Show resolved Hide resolved
@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 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!

synapse_bind_sydent/__init__.py Outdated Show resolved Hide resolved
synapse_bind_sydent/__init__.py Outdated Show resolved Hide resolved
Co-authored-by: reivilibre <olivier@librepush.net>
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