fix(recovery): Delete the known secrets from 4s when disabling recovery #4629
Conversation
poljar
commented
Feb 5, 2025
poljar
commented
- Public API changes documented in changelogs (optional)
poljar
force-pushed
the
poljar/delete-4s-whith-recovery
branch
from
1e43495 to
53b104c
Compare
bnjbvr
left a comment
bnjbvr
left a comment
There was a problem hiding this comment.
TIL there's no endpoint to remove global account data, and one can only reset them to default values 🥴
Anyhow, thanks, the patch looks good.
| let content = SecretEventContent::new(Default::default()); | ||
| let secret_content = Raw::from_json( | ||
| to_raw_value(&content) | ||
| .expect("We should be able to serialize a raw empty secret event content"), |
There was a problem hiding this comment.
Any chance we could use a plain ? here, since this function returns a Result type anyways?
There was a problem hiding this comment.
As the recovery module has a specialized error type we can't just use ? to convert the error. We either need to add a error variant for this or muck it somewhere into the matrix_sdk::Error type.
But I don't think it makes much sense, serializing the default event content should never fail.
There was a problem hiding this comment.
Makes sense to keep it like this, then 👍
|
Oh my god, you can't upload the empty content? Good thing I made an integration test. |
davidegirardi
left a comment
davidegirardi
left a comment
There was a problem hiding this comment.
Sounds reasonable. Do we need to delete (or overwrite with empty JSONs) the room keys too?
No, the backup API has a |
richvdh
requested review from
a team and
BillCarsonFr
and removed request for
a team
BillCarsonFr
left a comment
BillCarsonFr
left a comment
There was a problem hiding this comment.
We might want also to delete the actual key
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4629 +/- ##
==========================================
+ Coverage 85.72% 85.74% +0.02%
==========================================
Files 292 292
Lines 33490 33506 +16
==========================================
+ Hits 28709 28730 +21
+ Misses 4781 4776 -5 ☔ View full report in Codecov by Sentry. |
poljar
force-pushed
the
poljar/delete-4s-whith-recovery
branch
from
409fbe9 to
c444bc9
Compare
poljar
force-pushed
the
poljar/delete-4s-whith-recovery
branch
from
b1264c1 to
56c25f3
Compare
| // deserialize the content and find out the key ID. | ||
| // | ||
| // Then we finally set the event to an empty JSON content. | ||
| if let Ok(Some(default_event)) = |
There was a problem hiding this comment.
As an aside, I think that referring to account data as an "event" which can be "deleted" or changed in any way, is a confusing anti-pattern. One of the defining properties of an "event" in most environments is that it's something that happened in the past, so changing it is nonsense.
(I know that the spec uses that term, but I'd like that to change (matrix-org/matrix-spec#2030), and we can avoid spreading the mess any further in the meantime.)
There was a problem hiding this comment.
So, what should we call them?
There was a problem hiding this comment.
"Account data". Or possibly, "Account data entries".
