in our olm fork, one-time keys are now optional. however, in the identity service, we still have a couple places where we treat them as required, and fail certain requests if we're unable to fetch them. we should update these parts of the code to complete RPCs successfully regardless of OTK retrieval success or failure.
Details
- Manually deleted OTKs from my test account on staging, then called GetKeyserverKeys and GetOutboundKeysForUser and got successful responses from both (without any OTKs in the response)
- Modified get_one_time_key to return an error and confirmed that both RPCs were still successful
Diff Detail
- Repository
- rCOMM Comm
- Branch
- otk
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
I'm letting others review as well.
Generally feels alright and solves the original issue. Left one comment that's concerning to me
services/identity/src/database.rs | ||
---|---|---|
786–802 ↗ | (On Diff #44019) | I think we should not simply unwrap_or(None) here, at least without some considerations: I see two options here:
After a brief look, the conditions for option 2b are met, but I'd appreciate double-checking it. The same applies for the get_keyserver_keys |
services/identity/src/database.rs | ||
---|---|---|
786–802 ↗ | (On Diff #44019) | i think 2a makes sense. the duplication is a minor issue IMO |
Hey team, let's prioritize reviewing this one – I'm continuing to see identity errors in production related to missing OTKs, and I'm worried they're causing "unknown error" issues
It would be great if we could try and launch this this fix to production by the end of the week, or at least by Monday