Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shields: Add a code field for easier mapping of the state. #3786

Merged
merged 3 commits into from
Aug 5, 2024
Merged

Shields: Add a code field for easier mapping of the state. #3786

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
20f2c44
chore: Strongly typed ShieldStates.
pixlwave Jul 31, 2024
a666b88
shields: Put the Code inside the Colour instead of the Colour inside …
pixlwave Aug 1, 2024
d2c553f
sdk-ui: Make the SentInClear shield red.
pixlwave Aug 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions bindings/matrix-sdk-crypto-ffi/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ anyhow = { workspace = true }
futures-util = { workspace = true }
hmac = "0.12.1"
http = { workspace = true }
matrix-sdk-common = { workspace = true }
matrix-sdk-common = { workspace = true, features = ["uniffi"] }
pbkdf2 = "0.12.2"
rand = { workspace = true }
ruma = { workspace = true }
Expand All @@ -35,7 +35,7 @@ sha2 = { workspace = true }
thiserror = { workspace = true }
tracing-subscriber = { workspace = true, features = ["env-filter"] }
# keep in sync with uniffi dependency in matrix-sdk-ffi, and uniffi_bindgen in ffi CI job
uniffi = { workspace = true , features = ["cli"]}
uniffi = { workspace = true, features = ["cli"] }
vodozemac = { workspace = true }
zeroize = { workspace = true, features = ["zeroize_derive"] }

Expand Down
21 changes: 13 additions & 8 deletions bindings/matrix-sdk-crypto-ffi/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ pub use error::{
use js_int::UInt;
pub use logger::{set_logger, Logger};
pub use machine::{KeyRequestPair, OlmMachine, SignatureVerification};
use matrix_sdk_common::deserialized_responses::ShieldState as RustShieldState;
use matrix_sdk_common::deserialized_responses::{ShieldState as RustShieldState, ShieldStateCode};
use matrix_sdk_crypto::{
olm::{IdentityKeys, InboundGroupSession, SenderData, Session},
store::{Changes, CryptoStore, PendingChanges, RoomSettings as RustRoomSettings},
Expand Down Expand Up @@ -726,19 +726,24 @@ pub enum ShieldColor {
#[allow(missing_docs)]
pub struct ShieldState {
color: ShieldColor,
code: Option<ShieldStateCode>,
message: Option<String>,
}

impl From<RustShieldState> for ShieldState {
fn from(value: RustShieldState) -> Self {
match value {
RustShieldState::Red { message } => {
Self { color: ShieldColor::Red, message: Some(message.to_owned()) }
}
RustShieldState::Grey { message } => {
Self { color: ShieldColor::Grey, message: Some(message.to_owned()) }
}
RustShieldState::None => Self { color: ShieldColor::None, message: None },
RustShieldState::Red { code, message } => Self {
color: ShieldColor::Red,
code: Some(code),
message: Some(message.to_owned()),
},
RustShieldState::Grey { code, message } => Self {
color: ShieldColor::Grey,
code: Some(code),
message: Some(message.to_owned()),
},
RustShieldState::None => Self { color: ShieldColor::None, code: None, message: None },
}
}
}
Expand Down
14 changes: 9 additions & 5 deletions bindings/matrix-sdk-ffi/src/timeline/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use matrix_sdk::{
AttachmentConfig, AttachmentInfo, BaseAudioInfo, BaseFileInfo, BaseImageInfo,
BaseThumbnailInfo, BaseVideoInfo, Thumbnail,
},
deserialized_responses::ShieldState as RustShieldState,
deserialized_responses::{ShieldState as RustShieldState, ShieldStateCode},
Error,
};
use matrix_sdk_ui::timeline::{
Expand Down Expand Up @@ -929,19 +929,23 @@ impl From<&matrix_sdk_ui::timeline::EventSendState> for EventSendState {
pub enum ShieldState {
/// A red shield with a tooltip containing the associated message should be
/// presented.
Red { message: String },
Red { code: ShieldStateCode, message: String },
/// A grey shield with a tooltip containing the associated message should be
/// presented.
Grey { message: String },
Grey { code: ShieldStateCode, message: String },
/// No shield should be presented.
None,
}

impl From<RustShieldState> for ShieldState {
fn from(value: RustShieldState) -> Self {
match value {
RustShieldState::Red { message } => Self::Red { message: message.to_owned() },
RustShieldState::Grey { message } => Self::Grey { message: message.to_owned() },
RustShieldState::Red { code, message } => {
Self::Red { code, message: message.to_owned() }
}
RustShieldState::Grey { code, message } => {
Self::Grey { code, message: message.to_owned() }
}
RustShieldState::None => Self::None,
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/matrix-sdk-base/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ experimental-sliding-sync = [
"ruma/unstable-msc3575",
"ruma/unstable-simplified-msc3575",
]
uniffi = ["dep:uniffi", "matrix-sdk-crypto?/uniffi"]
uniffi = ["dep:uniffi", "matrix-sdk-crypto?/uniffi", "matrix-sdk-common/uniffi"]

# "message-ids" feature doesn't do anything and is deprecated.
message-ids = []
Expand Down
2 changes: 2 additions & 0 deletions crates/matrix-sdk-common/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ targets = ["x86_64-unknown-linux-gnu", "wasm32-unknown-unknown"]

[features]
js = ["instant/wasm-bindgen", "wasm-bindgen-futures"]
uniffi = ["dep:uniffi"]

[dependencies]
async-trait = { workspace = true }
Expand All @@ -28,6 +29,7 @@ serde_json = { workspace = true }
thiserror = { workspace = true }
tracing = { workspace = true }
tokio = { workspace = true, features = ["rt", "time"] }
uniffi = { workspace = true, optional = true }

[target.'cfg(target_arch = "wasm32")'.dependencies]
futures-util = { workspace = true, features = ["channel"] }
Expand Down
72 changes: 56 additions & 16 deletions crates/matrix-sdk-common/src/deserialized_responses.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,19 +87,24 @@ impl VerificationState {
pub fn to_shield_state_strict(&self) -> ShieldState {
match self {
VerificationState::Verified => ShieldState::None,
VerificationState::Unverified(level) => {
let message = match level {
VerificationLevel::UnverifiedIdentity | VerificationLevel::UnsignedDevice => {
UNVERIFIED_IDENTITY
VerificationState::Unverified(level) => match level {
VerificationLevel::UnverifiedIdentity | VerificationLevel::UnsignedDevice => {
ShieldState::Red {
code: ShieldStateCode::UnverifiedIdentity,
message: UNVERIFIED_IDENTITY,
}
VerificationLevel::None(link) => match link {
DeviceLinkProblem::MissingDevice => UNKNOWN_DEVICE,
DeviceLinkProblem::InsecureSource => AUTHENTICITY_NOT_GUARANTEED,
}
VerificationLevel::None(link) => match link {
DeviceLinkProblem::MissingDevice => ShieldState::Red {
code: ShieldStateCode::UnknownDevice,
message: UNKNOWN_DEVICE,
},
};

ShieldState::Red { message }
}
DeviceLinkProblem::InsecureSource => ShieldState::Red {
code: ShieldStateCode::AuthenticityNotGuaranteed,
message: AUTHENTICITY_NOT_GUARANTEED,
},
},
},
}
}

Expand All @@ -123,19 +128,28 @@ impl VerificationState {
}
VerificationLevel::UnsignedDevice => {
// This is a high warning. The sender hasn't verified his own device.
ShieldState::Red { message: UNSIGNED_DEVICE }
ShieldState::Red {
code: ShieldStateCode::UnsignedDevice,
message: UNSIGNED_DEVICE,
}
}
VerificationLevel::None(link) => match link {
DeviceLinkProblem::MissingDevice => {
// Have to warn as it could have been a temporary injected device.
// Notice that the device might just not be known at this time, so callers
// should retry when there is a device change for that user.
ShieldState::Red { message: UNKNOWN_DEVICE }
ShieldState::Red {
code: ShieldStateCode::UnknownDevice,
message: UNKNOWN_DEVICE,
}
}
DeviceLinkProblem::InsecureSource => {
// In legacy mode, we tone down this warning as it is quite common and
// mostly noise (due to legacy backup and lack of trusted forwards).
ShieldState::Grey { message: AUTHENTICITY_NOT_GUARANTEED }
ShieldState::Grey {
code: ShieldStateCode::AuthenticityNotGuaranteed,
message: AUTHENTICITY_NOT_GUARANTEED,
}
}
},
},
Expand Down Expand Up @@ -181,14 +195,40 @@ pub enum DeviceLinkProblem {
pub enum ShieldState {
/// A red shield with a tooltip containing the associated message should be
/// presented.
Red { message: &'static str },
Red {
/// A machine-readable representation.
code: ShieldStateCode,
/// A human readable description.
message: &'static str,
},
/// A grey shield with a tooltip containing the associated message should be
/// presented.
Grey { message: &'static str },
Grey {
/// A machine-readable representation.
code: ShieldStateCode,
/// A human readable description.
message: &'static str,
},
/// No shield should be presented.
None,
}

/// A machine-readable representation of the authenticity for a `ShieldState`.
#[derive(Clone, Debug, Deserialize, Serialize, Eq, PartialEq)]
#[cfg_attr(feature = "uniffi", derive(uniffi::Enum))]
pub enum ShieldStateCode {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If some client app persist that and then later we add a new code, how will this behave? When deserializing an old version of this enum?

Copy link
Member Author

@pixlwave pixlwave Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no Rust expert but the below seems to indicate that it will be fine if serialised to JSON:

use serde::{Serialize, Deserialize};
use serde_json;

#[derive(Serialize, Deserialize, Debug)]
enum Color {
    Red,
}


#[derive(Serialize, Deserialize, Debug)]
enum ColorPop {
    Yellow,
    Red,
    Green,
}


fn main() {
    let color = Color::Red;
    let json_string = serde_json::to_string(&color).unwrap();
    println!("Serialized color: {}", json_string); // Serialized color: "Red"
    
    let color: ColorPop = serde_json::from_str(&json_string).unwrap();
    println!("Deserialized color: {:?}", color); // Deserialized color: Red
}

How come the ShieldState is serialisable in the first place? Shouldn't the app always ask the SDK for the state to be up to date?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be for the WASM use-case where we sometimes, instead of adding a new type in the bindings which the SDK type gets converted into, we return a JSON object. This is using serde-wasm-bindgen.

If that's not the case, and nobody needs the implementations, perhaps we should just remove the Serialize and Deserialize implementations since they might have been cargo culted.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, I didn't know that about WASM 👍

/// Not enough information available to check the authenticity.
AuthenticityNotGuaranteed,
/// The sending device isn't yet known by the Client.
UnknownDevice,
/// The sending device hasn't been verified by the sender.
UnsignedDevice,
/// The sender hasn't been verified by the Client's user.
UnverifiedIdentity,
/// An unencrypted event in an encrypted room.
SentInClear,
}

/// The algorithm specific information of a decrypted event.
#[derive(Clone, Debug, Deserialize, Serialize)]
pub enum AlgorithmInfo {
Expand Down
3 changes: 3 additions & 0 deletions crates/matrix-sdk-common/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,3 +97,6 @@ macro_rules! boxed_into_future {
pub type BoxFuture<'a, T> = Pin<Box<dyn Future<Output = T> + 'a>>;
#[cfg(not(target_arch = "wasm32"))]
pub type BoxFuture<'a, T> = Pin<Box<dyn Future<Output = T> + Send + 'a>>;

#[cfg(feature = "uniffi")]
uniffi::setup_scaffolding!();
7 changes: 5 additions & 2 deletions crates/matrix-sdk-ui/src/timeline/event_item/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use matrix_sdk::{
Client, Error,
};
use matrix_sdk_base::{
deserialized_responses::{SyncTimelineEvent, SENT_IN_CLEAR},
deserialized_responses::{ShieldStateCode, SyncTimelineEvent, SENT_IN_CLEAR},
latest_event::LatestEvent,
};
use once_cell::sync::Lazy;
Expand Down Expand Up @@ -382,7 +382,10 @@ impl EventTimelineItem {
Some(info.verification_state.to_shield_state_lax())
}
}
None => Some(ShieldState::Grey { message: SENT_IN_CLEAR }),
None => Some(ShieldState::Red {
code: ShieldStateCode::SentInClear,
message: SENT_IN_CLEAR,
}),
}
}

Expand Down
12 changes: 9 additions & 3 deletions crates/matrix-sdk-ui/src/timeline/tests/shields.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use assert_matches::assert_matches;
use eyeball_im::VectorDiff;
use matrix_sdk_base::deserialized_responses::ShieldState;
use matrix_sdk_base::deserialized_responses::{ShieldState, ShieldStateCode};
use matrix_sdk_test::{async_test, sync_timeline_event, ALICE};
use ruma::{
event_id,
Expand Down Expand Up @@ -41,7 +41,10 @@ async fn test_sent_in_clear_shield() {

let item = assert_next_matches!(stream, VectorDiff::PushBack { value } => value);
let shield = item.as_event().unwrap().get_shield(false);
assert_eq!(shield, Some(ShieldState::Grey { message: "Sent in clear." }));
assert_eq!(
shield,
Some(ShieldState::Red { code: ShieldStateCode::SentInClear, message: "Sent in clear." })
);
}

#[async_test]
Expand Down Expand Up @@ -111,5 +114,8 @@ async fn test_local_sent_in_clear_shield() {
// Then the remote echo should now be showing the shield.
assert!(!event_item.is_local_echo());
let shield = event_item.get_shield(false);
assert_eq!(shield, Some(ShieldState::Grey { message: "Sent in clear." }));
assert_eq!(
shield,
Some(ShieldState::Red { code: ShieldStateCode::SentInClear, message: "Sent in clear." })
);
}
Loading