Skip to content

Commit

Permalink
[identity] avoid returning errors if one-time keys are missing
Browse files Browse the repository at this point in the history
Summary: 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.

Test Plan:
- 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

Reviewers: bartek, kamil, will

Reviewed By: kamil

Subscribers: ashoat, tomek

Differential Revision: https://phab.comm.dev/D13278
  • Loading branch information
vdhanan committed Sep 12, 2024
1 parent c65a6e9 commit 5dd8a28
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 13 deletions.
36 changes: 32 additions & 4 deletions services/identity/src/database.rs
Original file line number Diff line number Diff line change
Expand Up @@ -497,15 +497,29 @@ impl DatabaseClient {
OlmAccountType::Notification,
true,
)
.await?;
.await
.unwrap_or_else(|e| {
error!(
errorType = error_types::OTK_DB_LOG,
"Error retrieving notification one-time key: {:?}", e
);
(None, true)
});
let (content_one_time_key, _) = self
.get_one_time_key(
user_id,
&keyserver.device_id,
OlmAccountType::Content,
!requested_more_keys,
)
.await?;
.await
.unwrap_or_else(|e| {
error!(
errorType = error_types::OTK_DB_LOG,
"Error retrieving content one-time key: {:?}", e
);
(None, true)
});

debug!(
"Able to get notif one-time key for keyserver {}: {}",
Expand Down Expand Up @@ -787,15 +801,29 @@ impl DatabaseClient {
OlmAccountType::Notification,
true,
)
.await?;
.await
.unwrap_or_else(|e| {
error!(
errorType = error_types::OTK_DB_LOG,
"Error retrieving notification one-time key: {:?}", e
);
(None, true)
});
(device_keys.content_one_time_key, _) = self
.get_one_time_key(
user_id,
device_id_key,
OlmAccountType::Content,
!requested_more_keys,
)
.await?;
.await
.unwrap_or_else(|e| {
error!(
errorType = error_types::OTK_DB_LOG,
"Error retrieving content one-time key: {:?}", e
);
(None, true)
});
}
}

Expand Down
10 changes: 3 additions & 7 deletions services/identity/src/database/one_time_keys.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use comm_lib::{
DBItemAttributeError, DBItemError,
},
};
use tracing::{debug, error, info};
use tracing::{debug, error, info, warn};

use crate::{
constants::{
Expand Down Expand Up @@ -94,7 +94,7 @@ impl DatabaseClient {
.await?
.pop()
else {
return Err(Error::NotEnoughOneTimeKeys);
return Ok((None, requested_more_keys));
};

let delete_otk_operation = otk_row.as_delete_request();
Expand Down Expand Up @@ -209,11 +209,7 @@ impl DatabaseClient {

if let Some(limit) = num_keys {
if otk_rows.len() != limit {
error!(
errorType = error_types::OTK_DB_LOG,
"There are fewer one-time keys than the number requested"
);
return Err(Error::NotEnoughOneTimeKeys);
warn!("There are fewer one-time keys than the number requested");
}
}

Expand Down
2 changes: 0 additions & 2 deletions services/identity/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ pub enum Error {
IllegalState,
#[display(...)]
InvalidFormat,
#[display(...)]
NotEnoughOneTimeKeys,
}

#[derive(Debug, derive_more::Display, derive_more::Error)]
Expand Down

0 comments on commit 5dd8a28

Please sign in to comment.