Closed dani-garcia closed 3 weeks ago
Checkmarx One – Scan Summary & Details – 7e2fdc15-9cf0-41fa-a91e-b844ec2eca82
Attention: Patch coverage is 47.31458%
with 206 lines
in your changes missing coverage. Please review.
Project coverage is 60.07%. Comparing base (
e5a8dba
) to head (46f4b2c
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
📔 Objective
We're having some deadlock problems with the Passkey API by calling certain SDK functions on the
passkey-rs
callbacks. These issues are caused because we're holding a reference to the RwLock during the execution of the Passkey operation and so calling into the SDK from inside that will deadlock.To solve it we can't just use a lock at the client level, and so we need to move to using interior mutability, I've revived an old PR (#70) where we started doing that.
This PR is split into two commits, for ease of review:
&mut
references used in the bitwarden Client API. Note that this alone won't compile.Some notes of what the change entails:
EncryptionSettings
in aRwLock<Arc<>>
to get the API working, this is quite unfortunate as it forces us to cloneEncryptionSettings
to modify it when callingset_org_keys
. We can't wrap theorg_keys
in aRwLock
as we will get lifetime issues inget_key
. I think the best solution for this will be the work on creating a Crypto store that only returns opaque key references, so we might want to prioritize that.external_client
fromApiConfigurations
, as it's never mutated and it allows us to keep the same API forget_http_client
I hate that
Mutex
andRwLock
return a Result in case they are poisoned, that's never our case and it makes their usage so cumbersome 😢Note: I've tried to change the smallest amount of code to make the change, so the secrets manager APIs (and bitwarden-json) are still mutable. If we're okay with these changes I can go back to update those in a separate PR.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or ℹ️ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or 💭 (:thought_balloon:
) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:
) for suggestions / improvements:x:
) or ⚠️ (:warning:
) for more significant problems or concerns needing attention:seedling:
) or ♻️ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes