Skip to content

Conversation

mxandreas
Copy link
Contributor

@mxandreas mxandreas commented Sep 3, 2025

The configuration options secure_backup_required and secure_backup_setup_methods are no longer available. This was not yet reflected in the docs.

Checklist

@mxandreas mxandreas requested a review from a team as a code owner September 3, 2025 10:40
@mxandreas mxandreas self-assigned this Sep 3, 2025
@CLAassistant
Copy link

CLAassistant commented Sep 3, 2025

CLA assistant check
All committers have signed the CLA.

@t3chguy
Copy link
Member

t3chguy commented Sep 3, 2025

Deprecation means still available but will be removed soon, this doesn't look like a valid deprecation which should have happened before their removal?

EW still seems to read the 2 fields you specified. It seems like the only codepath which still respects them is the force reset flow which can be accessed via Restore Key Backup -> Reset Recovery

@mxandreas
Copy link
Contributor Author

mxandreas commented Sep 3, 2025

Deprecation means still available but will be removed soon, this doesn't look like a valid deprecation which should have happened before their removal?

Ideally yes, however, the change here is in the user/UI behavior and UI is continuously updated in the apps without any deprecation warnings (whether config settings are involved in this behavior or not, does not matter much). I think the only thing to be changed here now is to use "removed" instead of "deprecated" as I agree it can be misleading.

which can be accessed via Restore Key Backup -> Reset Recovery

Is this still accessible somewhere on the UI?

@t3chguy
Copy link
Member

t3chguy commented Sep 3, 2025

Only accessible when the CryptoEvent.KeyBackupFailed event fires from the js-sdk with haveNewVersion=true

@richvdh
Copy link
Member

richvdh commented Sep 3, 2025

which can be accessed via Restore Key Backup -> Reset Recovery

Is this still accessible somewhere on the UI?

Only accessible when the CryptoEvent.KeyBackupFailed event fires from the js-sdk with haveNewVersion=true

I think this path got overlooked when we were updating the other Recovery setup flows (in #29232, I think). It's related to #29171. In any case, the fact that that specific flow honours the old settings should be considered a bug, not a feature.

@t3chguy
Copy link
Member

t3chguy commented Sep 3, 2025

Agreed, but the documentation should match reality, and not state something to the effect of "these options don't work anymore"

mxandreas and others added 3 commits September 4, 2025 09:09
@mxandreas
Copy link
Contributor Author

mxandreas commented Sep 4, 2025

Agreed, but the documentation should match reality, and not state something to the effect of "these options don't work anymore"

The documentation should help the user, so the question is who and how would mentioning of the "CryptoEvent.KeyBackupFailed event fires". If you have a clear idea about that in mind, please suggest the corresponding update.

@t3chguy
Copy link
Member

t3chguy commented Sep 4, 2025

The documentation should help the user

Users can't change the config, only admins of a given Element Web instance can, so not sure why users would be reading this documentation. User-facing documentation lives on https://docs.element.io/

@mxandreas
Copy link
Contributor Author

I consider admins also users. Why is it important for them to know that some part of code is still accessing some of these configuration options?

@t3chguy
Copy link
Member

t3chguy commented Sep 4, 2025

Because the change makes it sound like they can leave their config and it has zero effect anymore, when in reality it does still have effect, just in rare cases, and if they want consistency they would need to remove it.

@mxandreas
Copy link
Contributor Author

when in reality it does still have effect, just in rare cases, and if they want consistency they would need to remove it.

Yes, but then we have to be able to explain it to them in the language they understand - what's the impact to the behavior. Right now this is not fully clear to me, and that is why I am asking.

@t3chguy
Copy link
Member

t3chguy commented Sep 4, 2025

The impact is: the docs apply, except in certain cases where they are a lie, yielding confusion and no clear reason for them to remove their existing configuration which apparently does nothing anymore. We should just rip out the edge case which still uses the config, it could have probably been done in the time we've spent debating it.

@mxandreas
Copy link
Contributor Author

The impact is: the docs apply, except in certain cases where they are a lie, yielding confusion and no clear reason for them to remove their existing configuration which apparently does nothing anymore.

I did not mean this "impact", I meant "impact" of what happens when the admins do not remove the settings: where, when and what kind of weird things the users are going to see.

We should just rip out the edge case which still uses the config, it could have probably been done in the time we've spent debating it.

In my view this should not have blocked the updates to the docs (in their current form) at all, because the current version is way more worse and confusing. I'll see with @richvdh how we proceed.

@t3chguy
Copy link
Member

t3chguy commented Sep 4, 2025

where, when and what kind of weird things the users are going to see.

Where: in the app
When: that'd be a Q for the crypto team to better understand when that edge case fires
What: depends on the config which is applicable to that environment which this PR tries to claim no longer does anything at all

@richvdh
Copy link
Member

richvdh commented Sep 4, 2025

I decided it was easier to remove the remaining support than to file a bug tracking it. #30702 removes the remaining support for these two options. That whole area of the code is a complete mess, and #29171 tracks that work.

@richvdh richvdh added the T-Task Tasks for the team like planning label Sep 4, 2025
@richvdh richvdh changed the title Deprecate recovery setup options in E2EE docs Remove outdated recovery setup options from E2EE docs Sep 4, 2025
@richvdh richvdh added this pull request to the merge queue Sep 8, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 8, 2025
@richvdh
Copy link
Member

richvdh commented Sep 8, 2025

CI failed due to #28836

@richvdh richvdh added this pull request to the merge queue Sep 8, 2025
github-merge-queue bot pushed a commit that referenced this pull request Sep 8, 2025
* Deprecate secure_backup_required and secure_backup_setup_methods in docs.

* Wording enhancements.

Co-authored-by: Richard van der Hoff <[email protected]>

* Use removal, not deprecation for sake of clarity.

Co-authored-by: Richard van der Hoff <[email protected]>

* Use removal, not deprecation for sake of clarity.

Co-authored-by: Richard van der Hoff <[email protected]>

* prettier

---------

Co-authored-by: Richard van der Hoff <[email protected]>
Co-authored-by: Richard van der Hoff <[email protected]>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 8, 2025
@richvdh richvdh added this pull request to the merge queue Sep 8, 2025
Merged via the queue into develop with commit 207173d Sep 8, 2025
35 checks passed
@richvdh richvdh deleted the e2ee-recovery-docs-update branch September 8, 2025 18:32
spantaleev added a commit to spantaleev/matrix-docker-ansible-deploy that referenced this pull request Sep 23, 2025
…required` & `secure_backup_setup_methods`) from `/.well-known/matrix/client`

Ref:

- element-hq/element-web#30681
- element-hq/element-web#30702
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Tasks for the team like planning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants