Don't set new room alias before potential 403

[AndrewFerr]

Fixes: #10929

Signed-off-by: Andrew Ferrazzutti fair@miscworks.net

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
• Pull request includes a sign off
• Code style is correct (run the linters)

[DMRobertson]

Thanks for spotting! Just to check I understand, what goes wrong without this change? I'm guessing we'll end up putting the alias in the DB anyway, so that

a) we have an alias to a room that doesn't exist
b) we can't reuse that alias in the future

It'd be great to have a test case that catches this---would you be up for writing one?

[DMRobertson]

Ahh sorry, I just found #10929 which describes the problem in detail (thank you again).

It'd be great to have a test case that catches this---would you be up for writing one?

Just wanted to add that I'd be more than happy to assist with this!

[DMRobertson]

Oh, minor thing: the changelog.d entry number needs to match the PR number, not the issue number. With that fixed, CI should run the test suite.

[DMRobertson]

I don't think this test would fail before your changes. I think we'd need to add logic which checks that we haven't written to the DB. Let me see if I can find an example...

[DMRobertson]

Here's an example where we select columns from a table with a where clause, and assert that we selected no rows:

synapse/tests/storage/databases/main/test_room.py

Lines 57 to 65 in dc75fb7

	room_creator_before = self.get_success(
	self.store.db_pool.simple_select_one_onecol(
	table="rooms",
	keyvalues={"room_id": room_id},
	retcol="creator",
	allow_none=True,
	)
	)
	self.assertEqual(room_creator_before, None)

We'd have to work out what table is being written to and so on.

Alternatively, we could follow your description on #10929 and try to create a room with the same alias, but as a user with permission. That user should succeed in doing so.

Try to create the same room again, but with a different user that has permission to publish rooms
Attempts to use the same alias for the new room will fail, since the alias is still in use for the room that failed to be created

That would involve

• self.register_user to make a new user
• self.login to generate an access token for that user
• passing access_token=... to self.make_request calls to act as that user.

Here's an example:

synapse/tests/rest/admin/test_room.py

Lines 1035 to 1045 in e584534

	# Have another user join the room
	user_1 = self.register_user("foo", "pass")
	user_tok_1 = self.login("foo", "pass")
	self.helper.join(room_id_1, user_1, tok=user_tok_1)
	url = "/_synapse/admin/v1/rooms/%s" % (room_id_1,)
	channel = self.make_request(
	"GET",
	url.encode("ascii"),
	access_token=self.admin_user_tok,
	)

Sorry, that's quite an info dump. How does that feel---does it make sense/feel doable?

[AndrewFerr]

I added a test to create a published room as a denied, then allowed, user.

I managed to do it just by setting a custom user_id in calls to self.helper.create_room_as, while the test context is managed by an admin user. It seems to work fine, but please let me know if what I did is problematic.

[DMRobertson]

Not sure I see why we'd want to repeat these tests in this specific order. What's your thinking here?

[AndrewFerr]

If multiple room creation attempts are made in the same test function, the database persists the effects of the attempts, namely reserved aliases. To make it clearer, I tweaked & added some tests.

The test_denied_then_retry tests should succeed because their final creation attempt should be permitted. Without my code changes, they fail because of the requested alias having been consumed from the first (failed) attempt.

[DMRobertson]

Many thanks for this---I think it's almost there.

It's a bit of a nitpick, but I think the test method names could be a little more descriptive. Perhaps something like this?

• test_denied_without_publication_permission
• test_allowed_when_creating_private_room
• test_allowed_with_publication_permission
• test_can_create_as_private_room_after_rejection
• test_can_create_with_permission_after_rejection

[DMRobertson]

This is going to mutate self.data, which might affect tests called afterwards. That means that tests can be sensitive to the order they're ran in, or might not run the same in isolation!

For this specific case, I think the other calls to create_room_as explicitly set is_public=False and so each test will behave the same no matter which order they're ran in. But I think we should try to avoid mutating self.data anyway, so the reader doesn't have to reason through this.

How about:

Suggested change

	self.data["visibility"] = "public"
	self.helper.create_room_as(
	self.denied_user_id, extra_content=self.data, expect_code=403
	)
	data = self.data.copy()
	data["visibility"] = "public"
	self.helper.create_room_as(
	self.denied_user_id, extra_content=data, expect_code=403
	)

[richvdh]

This is going to mutate self.data, which might affect tests called afterwards.

I'm not sure this is correct: I think there will be a new instance of TestCreatePublishedRoomACL for each test.

[richvdh]

ohhh, but each instance will end up pointing at the same dict, I guess, because it's a class-level property rather than an instance-level one. Maybe it'd be better to intialise self.data in prepare?

[DMRobertson]

I'm not sure this is correct: I think there will be a new instance of TestCreatePublishedRoomACL for each test.

Looks like it! See https://gist.github.com/DMRobertson/40fe186843a327658cbee31bb381e1b2

[richvdh]

@AndrewFerr unfortunately force-pushing makes it impossible to see what has changed since the previous review. Please could you reverse the force-push?

[richvdh]

cannot review after force-push

[AndrewFerr]

@AndrewFerr unfortunately force-pushing makes it impossible to see what has changed since the previous review. Please could you reverse the force-push?

Sorry about that. The force-push made 2 changes:

1. included Make is_public Optional[bool] for create_room_as test util (#10951) #10963
2. rebased to the tip of develop at the time

Since this change must now eventually include #10963, may I wait for that to be merged before making any more changes here?

[AndrewFerr]

What I pushed just now is what I believe was the state of this PR before the force-push.

Since #10964 is merged, I have it included in another local branch on the tip of develop, which I can push if that would be cleaner. Or should that go in a new PR altogether?

[AndrewFerr]

And this latest push includes an extra change that I had made on top of the create_room/is_public change, which I forgot to mention in the list of changes I said my force-push had included.

[richvdh]

I'm not entirely following you, but if you merge from matrix-org:develop into your local develop, that should update this PR to include any recent changes including #10963.

[AndrewFerr]

Alright. Is it fine if I rebase instead of merge?

[richvdh]

no, because that makes it hard for us to see what has changed during the rebase. Please merge.

[AndrewFerr]

Done.

This merges what I had prior to the rebase that messed everything up, adds a change on top of #10963 (d971452), and makes a fix to linter errors.

If this is still unusable, I'll make whatever changes are needed to fix it.

[richvdh]

Thanks! It looks better now, though it looks like you've included some changes to saml.py which are causing a CI failure.

un-rebased

[DMRobertson]

LGTM! Thank you for seeing this through! I guess @richvdh might want to approve before merging?

[richvdh]

thank you!