Skip to content

Conversation

@clokep
Copy link
Member

@clokep clokep commented Jul 22, 2021

This tests what happens if a server is unable to authorize a room join (due to not being able to invite users or not being able to check the proper allowed rooms).

Corresponds to matrix-org/synapse#10447

Based on #145

Copy link
Member

Choose a reason for hiding this comment

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

This should probably be synced with alice.SyncUntilTimelineHas as well right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there any reason this is just always synced? Seems like it would be an improvement.

Copy link
Member

Choose a reason for hiding this comment

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

Why don't you use setupRestrictedRoom in the previous test?

Copy link
Member Author

Choose a reason for hiding this comment

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

The joiners need to be on the same server while the creator needs to be on a separate server. Unfortunately BlueprintFederationTwoLocalOneRemote has alice and bob on one server and charlie on another.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we wait for these to come down alice sync stream?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't matter if alice sees them.

Copy link
Member

Choose a reason for hiding this comment

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

Fairly sure there won't be any guarantee that hs2 would've seen the join from hs3 because the join was between hs3 and hs1. Therefore, doing this will be racey.

@clokep clokep force-pushed the clokep/restricted-auth-err branch from 0209430 to 231d6be Compare July 22, 2021 19:06
@clokep
Copy link
Member Author

clokep commented Jul 26, 2021

I rebased this, sorry about that. Wanted to remove the commits from the previous PR and forgot there had been a review on this already. 🤦

@clokep clokep marked this pull request as ready for review July 26, 2021 16:48
@clokep clokep requested a review from a team July 26, 2021 16:48
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

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

This looks pretty sensible to me now from a test coverage perspective. Just one minor comment on a previous discussion.

@clokep clokep merged commit f254629 into master Jul 30, 2021
@clokep clokep deleted the clokep/restricted-auth-err branch July 30, 2021 18:25
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