Skip to content

Conversation

MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Oct 7, 2025

Introduce RootConfig.validate_config() which can be subclassed in HomeServerConfig to do cross-config class validation. This means we can move the open registration config validation from setup() to HomeServerConfig.validate_config() (much more sane).

Spawning from looking at this area of code in #19015

Dev notes

Open registration validation introduced in matrix-org/synapse#12091

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.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Code style is correct (run the linters)

obj.invoke_all("read_arguments", config_args)

return obj
return cls.load_config(description, argv_options)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change makes it so we go through the normal load_config(...) code path instead of inventing yet another way to load config.

It also means we can settle on calling validate_config(...) in one place as everything flows to load_config_with_parser(...) now.

"spam and abuse. If you would like to allow public registration, please consider adding email, "
"captcha, or token-based verification. Otherwise this check can be removed by setting the "
"`enable_registration_without_verification` config option to `true`."
)
Copy link
Contributor Author

@MadLittleMods MadLittleMods Oct 8, 2025

Choose a reason for hiding this comment

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

The only real change is that this has moved from a place that only runs for the main Synapse instance to everything including workers.

But I assume that's fine. We want the same warning for workers as we wouldn't want open registration there either.

@MadLittleMods MadLittleMods changed the title Move open registration config validation from setup to load_or_generate_config Introduce RootConfig.validate_config() which can be subclassed in HomeServerConfig to do cross-config class validation Oct 9, 2025
@MadLittleMods MadLittleMods marked this pull request as ready for review October 9, 2025 00:50
@MadLittleMods MadLittleMods requested a review from a team as a code owner October 9, 2025 00:50
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.

Nice idea. I support it.

We should ensure that we don't end up changing config option values in validate_config, else we once again won't be able to cross-validate on the final config set.

MadLittleMods and others added 5 commits October 9, 2025 12:19
@MadLittleMods MadLittleMods merged commit 47fb4b4 into develop Oct 9, 2025
76 of 78 checks passed
@MadLittleMods MadLittleMods deleted the madlittlemods/relocate-snowflake-config-validation branch October 9, 2025 19:56
@MadLittleMods
Copy link
Contributor Author

Thanks for the review @anoadragon453 🦋

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants