feat(): validated oob invitation #287

Christiantyemele · 2024-12-18T14:59:33Z

No description provided.

Hermann-Core · 2024-12-20T10:18:06Z

crates/web-plugins/oob-messages/src/models.rs

        let plaintext =
            to_string(oob_message).map_err(|e| format!("Serialization error: {}", e))?;
        let encoded_jwm = Base64Url.encode(plaintext.as_bytes());

-        Ok(format!("{}?_oob={}", url, encoded_jwm))
+        let ooburl = format!("{}/_oob{}", url, encoded_jwm);


You changed the format of the oob_url. Is there any reason to that?

yes i want to use the oob jwm as a routing path to the out band message and the previous format contains query parameters which will be misinterpreted if used as route path

You don't have to do what you plan to do. Plus, note that the OOB URL format is normative. Check https://identity.foundation/didcomm-messaging/spec/#standard-message-encoding.

crates/web-plugins/oob-messages/src/models.rs

crates/web-plugins/oob-messages/src/plugin.rs

Hermann-Core · 2024-12-20T10:27:41Z

crates/web-plugins/oob-messages/src/web.rs

    Router::new() //
        .route("/oob_url", get(handler_oob_inv))
        .route("/oob_qr", get(handler_oob_qr))
        .route("/", get(handler_landing_page_oob))
+        // handle oob invitation to invitation message
+        .route(&invitation_path, get(decode_oob_inv))


I dont understand what the new route is and why you added it

The qr_code and and oob url have no handled route, what i mean is if you can the oob qr_code you get nothing even the oob_url takes you no where as the route has not been handled and as such you can have its content without decoding it, that's where this route comes in

The oob_url should not necessarily take you to somewhere.

Blindspot22

Apart from the changes @Hermann-Core requested, I think this PR's good to go.

IngridPuppet

This PR changes the OOB URL into a format that is not normative. I left some comments regarding that and other points. Please, could you check?

IngridPuppet · 2025-01-08T14:49:21Z

crates/web-plugins/oob-messages/src/models.rs

+        let url = format!("{}", server_public_domain);
+        let encoded_oob = OobMessage::serialize_oob_message(&oobmessage, &url).unwrap();
+        encoded_oob


Add an assert_eq! to validate what you expect as valid output.

IngridPuppet · 2025-01-08T14:55:26Z

crates/web-plugins/oob-messages/src/web.rs

    Router::new() //
        .route("/oob_url", get(handler_oob_inv))
        .route("/oob_qr", get(handler_oob_qr))
        .route("/", get(handler_landing_page_oob))
+        // handle oob invitation to invitation message
+        .route(&invitation_path, get(decode_oob_inv))


The oob_url should not necessarily take you to somewhere.

crates/web-plugins/oob-messages/src/models.rs

IngridPuppet · 2025-01-08T15:28:17Z

crates/web-plugins/oob-messages/src/models.rs

        let plaintext =
            to_string(oob_message).map_err(|e| format!("Serialization error: {}", e))?;
        let encoded_jwm = Base64Url.encode(plaintext.as_bytes());

-        Ok(format!("{}?_oob={}", url, encoded_jwm))
+        let ooburl = format!("{}/_oob{}", url, encoded_jwm);


You don't have to do what you plan to do. Plus, note that the OOB URL format is normative. Check https://identity.foundation/didcomm-messaging/spec/#standard-message-encoding.

IngridPuppet · 2025-01-08T15:31:50Z

crates/web-plugins/oob-messages/src/models.rs

+        let serialized_msg = test_serialize_oobmessage();
+        let oobmessage: Vec<&str> = serialized_msg.split("/_oob").collect();
+        let oobmessage = oobmessage.get(1).unwrap().to_string();


Note that _oob must be a query parameter in a compliant OOB URL.

IngridPuppet · 2025-01-08T15:36:08Z

crates/web-plugins/oob-messages/src/plugin.rs

+        let oobmessage: Vec<&str> = oob_inv.split("/_").collect();
+        let oobmessage = oobmessage.get(1).unwrap_or(&"").to_string();
+
        self.state = Some(OOBMessagesState {
            filesystem: Arc::new(Mutex::new(fs)),
+            oobmessage,
        });


I don't see the point of extracting something like "oob=abcd...xyz", when you already have the full string...

Did you run cargo clippy on your changes? I feel like you can use unwrap_or_default here.

IngridPuppet · 2025-01-08T15:40:14Z

crates/web-plugins/oob-messages/src/web/handler.rs

+        <body>
+       <a href={}>out of band invitation url</a>
+        </body>


Please leave the URL plainly visible. It is more practical that way. See https://mediator.rootsid.cloud.

IngridPuppet · 2025-01-08T15:41:25Z

crates/web-plugins/oob-messages/src/web/handler.rs

+    let html_content =
+        match retrieve_or_generate_oob_inv(&mut *fs, &server_public_domain, &storage_dirpath) {


Could you rename this variable? I don't think it contains anything HTML.

IngridPuppet · 2025-01-08T15:48:26Z

crates/web-plugins/oob-messages/src/web/handler.rs

+pub(crate) async fn decode_oob_inv(State(state): State<Arc<OOBMessagesState>>) -> Response {
+    let encoded_inv = &state.oobmessage;
+    let encoded_inv: Vec<&str> = encoded_inv.split("oob").collect();
+    let encoded_inv = encoded_inv.get(1).unwrap();
+    let decoded_inv = Base64Url.decode(encoded_inv).unwrap_or_default();


I am not seeing the purpose of this handler... Do you have any valid use case?

crates/web-plugins/oob-messages/src/models.rs

Hermann-Core · 2025-01-09T12:59:07Z

crates/web-plugins/oob-messages/src/models.rs

        let plaintext =
            to_string(oob_message).map_err(|e| format!("Serialization error: {}", e))?;
        let encoded_jwm = Base64Url.encode(plaintext.as_bytes());

-        Ok(format!("{}?_oob={}", url, encoded_jwm))
+        let ooburl = format!("{}/_oob{}", url, encoded_jwm);


Suggested change

let ooburl = format!("{}/_oob{}", url, encoded_jwm);

let oob_url = format!("{}/_oob{}", url, encoded_jwm);

Hermann-Core · 2025-01-09T13:01:55Z

crates/web-plugins/oob-messages/src/models.rs

+    let url: &String = &format!("{}", server_public_domain);
    let oob_url = OobMessage::serialize_oob_message(&oob_message, url)


The conversion here is unnecessary.

Suggested change

let url: &String = &format!("{}", server_public_domain);

let oob_url = OobMessage::serialize_oob_message(&oob_message, url)

let oob_url = OobMessage::serialize_oob_message(&oob_message, server_public_domain)

Hermann-Core · 2025-01-09T13:03:33Z

crates/web-plugins/oob-messages/src/models.rs

+        let oobmessage: Vec<&str> = serialized_msg.split("/_oob").collect();
+        let oobmessage = oobmessage.get(1).unwrap().to_string();


Suggested change

let oobmessage: Vec<&str> = serialized_msg.split("/_oob").collect();

let oobmessage = oobmessage.get(1).unwrap().to_string();

let oob_message: Vec<&str> = serialized_msg.split("/_oob").collect();

let oob_message = oobmessage.get(1).unwrap().to_string();

Hermann-Core · 2025-01-09T13:05:07Z

crates/web-plugins/oob-messages/src/plugin.rs

 }

 #[derive(Clone)]
 pub(crate) struct OOBMessagesState {
    pub(crate) filesystem: Arc<Mutex<dyn FileSystem>>,
+    pub(crate) oobmessage: String,


what is the purpose of the field you added here?

Hermann-Core · 2025-01-09T13:07:06Z

crates/web-plugins/oob-messages/src/web.rs

+        let server_public_domain = std::env::var("SERVER_PUBLIC_DOMAIN").unwrap();
+        let storage_dirpath = std::env::var("STORAGE_DIRPATH").unwrap();
+        let mut fs = MockFileSystem;
+        let oobmessage =


Suggested change

let oobmessage =

let oob_message =

I have check all the comments and from our discussions it seems this PR actually solves no issue or brings and adjustments so, i guess it should be closed @Hermann-Core @IngridPuppet

I would agree. Also check the comment I left on the issue before planning to close it as well.

feat(): validated oob invitation

1809705

Christiantyemele linked an issue Dec 18, 2024 that may be closed by this pull request

Validate OOB invitation #286

Open

Christiantyemele requested review from IngridPuppet, Hermann-Core, ndefokou and Blindspot22 and removed request for Hermann-Core December 18, 2024 15:00

ndefokou approved these changes Dec 20, 2024

View reviewed changes

Hermann-Core requested changes Dec 20, 2024

View reviewed changes

Blindspot22 approved these changes Dec 20, 2024

View reviewed changes

Christiantyemele requested a review from Hermann-Core December 20, 2024 13:53

fix(): removed testing template

dda78f5

Christiantyemele self-assigned this Jan 6, 2025

IngridPuppet requested changes Jan 8, 2025

View reviewed changes

Hermann-Core requested changes Jan 9, 2025

View reviewed changes

Christiantyemele closed this Jan 10, 2025

chendiblessing deleted the 286-validate-oob-invitation branch January 16, 2025 10:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(): validated oob invitation #287

feat(): validated oob invitation #287

Christiantyemele commented Dec 18, 2024

Hermann-Core Dec 20, 2024

Christiantyemele Dec 20, 2024

IngridPuppet Jan 8, 2025

Hermann-Core Dec 20, 2024

Christiantyemele Dec 20, 2024

IngridPuppet Jan 8, 2025

Blindspot22 left a comment

IngridPuppet left a comment

IngridPuppet Jan 8, 2025

IngridPuppet Jan 8, 2025

IngridPuppet Jan 8, 2025

IngridPuppet Jan 8, 2025

IngridPuppet Jan 8, 2025

IngridPuppet Jan 8, 2025

IngridPuppet Jan 8, 2025

IngridPuppet Jan 8, 2025

Hermann-Core Jan 9, 2025

Hermann-Core Jan 9, 2025

Hermann-Core Jan 9, 2025

Hermann-Core Jan 9, 2025

Hermann-Core Jan 9, 2025

Christiantyemele Jan 9, 2025

IngridPuppet Jan 9, 2025

		let html_content =
		match retrieve_or_generate_oob_inv(&mut *fs, &server_public_domain, &storage_dirpath) {

	let ooburl = format!("{}/_oob{}", url, encoded_jwm);
	let oob_url = format!("{}/_oob{}", url, encoded_jwm);

		let url: &String = &format!("{}", server_public_domain);
		let oob_url = OobMessage::serialize_oob_message(&oob_message, url)

		let oobmessage: Vec<&str> = serialized_msg.split("/_oob").collect();
		let oobmessage = oobmessage.get(1).unwrap().to_string();

feat(): validated oob invitation #287

feat(): validated oob invitation #287

Conversation

Christiantyemele commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blindspot22 left a comment

Choose a reason for hiding this comment

IngridPuppet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment