`room_config` param to `user_may_create_room` spam-checker callback has invalid data when called for room upgrades

[hughns]

Background:

#18486 added the room_config param to the user_may_create_room spam-checker callback.

This callback is also called when the room upgrade API is used (not that this was documented). The is a bug that means that in the case of a room upgrade request the room_config data is junk (it was attempting a subset of what might have been in the original request and makes mistakes in how it constructs the initial_state data).

However, it transpires that this was unnecessary as you can use the on_create_room thirdparty rules callback and raise an Exception instead. That said, you then don't get the callback invoked in the upgrade case.

So, I'm not sure whether it makes sense to fix the broken behaviour or revert.

Some options to address this:

• Change behaviour of user_may_create_room to set room_config to None for room upgrades. This is what was drafted in Document and fix room_config param when user_may_create_room callback is invoked for a room upgrade #18721
• Remove the room_config param from user_may_create_room and point to the on_create_room callback instead. Basically revert Pass room_config argument to user_may_create_room spam checker module callback #18486 and add documentation. Also ignore the case of checking room_config for upgrades as it has never worked and therefore no-one can be relying on it.
• As above but somehow deprecate first (although it would probably then end up never being removed)

I'm happy to do whatever is needed to clean this up, but need some guidance on which way to go.

[anoadragon453]

Looks like @MadLittleMods has given his opinion in https://github.com/element-hq/synapse/pull/18721/files#r2237492469.

It'd be useful to add a user_may_upgrade_room/on_room_upgrade API method that modules can use to handle specifically room upgrades more clearly. But that can be a separate issue.

[MadLittleMods]

Also see #18721 (comment) for the ideal solution

[hughns]

Also see #18721 (comment) for the ideal solution

I agree that changing where the callback is invoked from would mean that full context could be passed.

I'm hesitant to change the location as it could have unintended consequences for existing users of the callback as it predates #18486. I also have no need for this nor am I aware of anyone else.

As the idea of going to None for an upgrade didn't go down, I instead propose doing what I originally tried which is to fix the format of initial_state for an upgrade and then improve the documentation for the callback to help future users understand the limitations.

This is in #18721