Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

MSC3861: m.3pid_changes capability should be disabled #16134

Merged
merged 5 commits into from Aug 22, 2023

Conversation

MatMaul
Copy link
Contributor

@MatMaul MatMaul commented Aug 18, 2023

Fixes #16128.

Pull Request Checklist

@MatMaul MatMaul marked this pull request as ready for review August 18, 2023 13:47
@MatMaul MatMaul requested a review from a team as a code owner August 18, 2023 13:47
@hughns
Copy link
Member

hughns commented Aug 18, 2023

FWIW, the approach that has been used elsewhere for MSC3861 is to error if the admin has tried to explicitly enable a feature that is not compatible with delegated auth. i.e. https://github.com/matrix-org/synapse/blob/develop/synapse/config/experimental.py#L151

I was intended to write a PR for this myself which did:

  1. Change the default for enable_3pid_changes to be False if experimental MSC3861 is enabled: at https://github.com/matrix-org/synapse/blob/develop/synapse/config/registration.py#L136
  2. Added a runtime check at https://github.com/matrix-org/synapse/blob/develop/synapse/config/experimental.py#L151
  3. Unit tests to the runtime config error fires when misconfigured: https://github.com/matrix-org/synapse/blob/develop/tests/config/test_oauth_delegation.py#L56

@erikjohnston erikjohnston added the X-Release-Blocker Must be resolved before making a release label Aug 21, 2023
clokep
clokep previously requested changes Aug 22, 2023
Copy link
Contributor

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Per @hughns's review.

@MatMaul MatMaul force-pushed the mv/delegate-auth-disable-3pid-changes branch from e049888 to 5b146fc Compare August 22, 2023 12:11
@MatMaul MatMaul requested a review from clokep August 22, 2023 12:15
@clokep clokep requested review from hughns and a team and removed request for clokep August 22, 2023 12:19
@clokep clokep dismissed their stale review August 22, 2023 12:19

PR Updated

Copy link
Member

@hughns hughns left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@clokep
Copy link
Contributor

clokep commented Aug 22, 2023

I combined the news file with #16127.

@clokep clokep enabled auto-merge (squash) August 22, 2023 13:58
@clokep clokep merged commit 0ba1777 into develop Aug 22, 2023
37 checks passed
@clokep clokep deleted the mv/delegate-auth-disable-3pid-changes branch August 22, 2023 14:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
X-Release-Blocker Must be resolved before making a release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

m.3pid_changes capability should be disabled when experimental MSC3861 auth is enabled
4 participants