[tutasdk] sdk should be able to make service calls #7708

tuta-sudipg · 2024-10-09T14:00:06Z

NOTE: Compressed String not yet supported!
RustGenerator should produce service files
EntityFacade should be able to encrypt entities
CreateSession ( without offline login and 2FA )
NativeRestClient based on hyper to use in non-mobile ( desktop ) devices

Test Notes

regression test mail notifications for ios + android

charlag · 2024-10-10T07:51:43Z

buildSrc/RustGenerator.js

@@ -13,7 +13,7 @@ import { AssociationType, Type } from "../src/common/api/common/EntityConstants.
 */
 export function generateRustType({ type, modelName }) {
 	let typeName = mapTypeName(type.name, modelName)
-	let buf = `#[derive(uniffi::Record, Clone, Serialize, Deserialize, Debug)]
+	let buf = `#[derive(uniffi::Record, Clone, Serialize, Deserialize, Debug, PartialEq)]


I think we discussed making PartialEq cfg(test)

charlag · 2024-10-10T07:53:44Z

tuta-sdk/rust/sdk/Cargo.toml

+mockall = { version = "0.13.0", optional = true }
+mockall_double = { version = "0.3.1", optional = true }
+
+# only used for the native rest client


can we gate it behind arch, feature or somehow else? Would be nice to not compile these deps 4 times on Android build

not sure what you mean, are the tests run 4 times?

isn't it in the list of normal dependencies? ahhh, it's top-level one, they are only referenced through the net feature?

Tried to play with mockall.
But now, they are referenced only through test. So they can stay in dev-dependencies now.

charlag · 2024-10-10T07:56:54Z

tuta-sdk/rust/sdk/src/crypto/crypto_facade.rs

@@ -32,22 +33,24 @@ const BUCKET_KEY_FIELD: &str = "bucketKey";

 #[derive(uniffi::Object)]
 pub struct CryptoFacade {
-	key_loader_facade: Arc<KeyLoaderFacade>,
+	key_loader_facade: Option<Arc<KeyLoaderFacade>>,


I am strongly opposed to making this a variable state. We tried really hard to make sure that certain parts can only be instantiated when we are logged in. I would like us to discuss the design for this again.

I also don't really like it. I think we can construct a proof that makes it impossible to call the methods requiring the KeyloaderFacade unless you successfully logged in 🤔

The current design makes it hard to do unauthenticated requests because the components that require a login are all over the rest stack

It is one thing to do unauthenticated requests (that is kinda hidden in headers provider) but another to request encrypted things. We have multiple EntityClients partially for this reason.

I really think you need two top-level ServiceExecutors, maybe they both delegate to the same thing under the hood though.

we had a long think about this and we came up with a few ideas to prevent mistakes (with forgotten session keys) at compile time, but nothing that we would implement now since it would require some more work.

We did a version that wraps a service executor that can resolve keys around a "normal" service executor though to not damage any invariants that were there before.

tuta-sdk/rust/sdk/src/crypto_entity_client.rs

charlag · 2024-10-10T08:55:32Z

tuta-sdk/rust/sdk/src/entities/entity_facade.rs

+		value_name: &str,
+		model_value: &ModelValue,
+		instance_value: &ElementValue,
+		session_key: &Option<GenericAesKey>,


my immediate reaction is that I am confused why encrypt_value would be called without a key. If the entity does not have a session key at all we shouldn't try to encrypt it, if I understand it right.

agreed. Will try to make it non-optional

changed the function name instead

but what is the motivation? In the other direction we just don't call decrypt unless we need to

ended up checking the encrypted flag in the service executor

charlag · 2024-10-10T09:22:16Z

tuta-sdk/rust/sdk/src/services/service_executor.rs

+
+pub struct ServiceExecutor {
+	auth_headers_provider: Arc<HeadersProvider>,
+	crypto_facade: Arc<CryptoFacade>,


I think this CryptoFacade here is the reason why we don't have strong guarantees about logged in state.

I think we should have similar separation as with EntityClient and have one without encryption support and one with encryption support.

charlag · 2024-10-10T09:25:30Z

tuta-sdk/rust/sdk/src/services/service_executor.rs

+					"type {:?} does not exist",
+					input_type_ref
+				)))?;
+			let encrypted_parsed_entity = self.entity_facade.encrypt_and_map_to_literal(


I think we should not call encrypt for entities that are not encrypted, like I mentioned before

charlag · 2024-10-10T09:27:01Z

tuta-sdk/rust/sdk/src/services/service_executor.rs

+						})?;
+					Ok(typed_entity)
+				},
+				// `resolve_session_key()` only returns none if the entity is unencrypted, so


should we match or just unwrap then?

charlag · 2024-10-10T09:32:45Z

tuta-sdk/rust/sdk/src/net/native_rest_client.rs

+use hyper_util::rt::TokioExecutor;
+use std::collections::HashMap;
+
+pub struct NativeRestClient {


I still think reqwest impl is much shorter but if it works, it works

charlag · 2024-10-10T09:35:22Z

tuta-sdk/rust/sdk/src/entities/entity_facade.rs

@@ -41,15 +62,238 @@ pub trait EntityFacade: Send + Sync {
 		entity: ParsedEntity,
 		resolved_session_key: ResolvedSessionKey,
 	) -> Result<ParsedEntity, ApiCallError>;
+
+	fn encrypt_and_map_to_literal(


I don't know if the "literal" in the name makes sense anymore, that was kind of js-speak for "object" I guess

charlag · 2024-10-10T16:19:30Z

tuta-sdk/rust/sdk/src/entities/entity_facade.rs

+		match (&association.cardinality, instance_association) {
+			(Cardinality::ZeroOrOne, ElementValue::Null) => Ok(ElementValue::Null),
+			(_, ElementValue::Null) => {
+				panic!("Undefined attribute {}:{association_name}", type_model.name)


when we can't find association it is Err but here we panic(). We should be consistent.

paw-hub

Had the opportunity to try out the new ServiceExecutor, and I have a request.

tuta-sdk/rust/sdk/src/services/service_executor.rs

was resolved

* CompressedString is still not supported! * RustGenerator produces service files which are expanded into service trait implementations by a declarative macro * EntityFacade gained the ability to encrypt entities * limited CreateSession ( without offline login and 2FA ) for testing purposes Co-authored-by: sug <[email protected]> Co-authored-by: jhm <[email protected]> Co-authored-by: paw-hub <[email protected]> Co-authored-by: ivk <[email protected]> Co-authored-by: nig <[email protected]> Co-authored-by: map <[email protected]>

tuta-sudipg requested review from paw-hub and charlag October 9, 2024 14:00

paw-hub approved these changes Oct 10, 2024

View reviewed changes

charlag reviewed Oct 10, 2024

View reviewed changes

ganthern force-pushed the tutasdk-service-executor branch from d5a1cff to 6221be6 Compare October 11, 2024 06:28

tuta-sudipg force-pushed the tutasdk-service-executor branch 2 times, most recently from 864fba4 to d333f8c Compare October 11, 2024 10:06

paw-hub previously requested changes Oct 11, 2024

View reviewed changes

tuta-sdk/rust/sdk/src/services/service_executor.rs Show resolved Hide resolved

paw-hub mentioned this pull request Oct 11, 2024

[sdk] Add trash mail and mark mail as read methods to MailFacade #7716

Merged

ganthern force-pushed the tutasdk-service-executor branch from 445135d to 27d746a Compare October 14, 2024 07:43

tuta-sudipg force-pushed the tutasdk-service-executor branch 2 times, most recently from 2d8af74 to 742ee8d Compare October 14, 2024 13:39

ganthern force-pushed the tutasdk-service-executor branch from 742ee8d to ef4cea6 Compare October 14, 2024 14:53

[tutasdk] use client version from bump-version script

628926f

ganthern force-pushed the tutasdk-service-executor branch 2 times, most recently from a80afda to 9728869 Compare October 14, 2024 14:59

ganthern and others added 2 commits October 14, 2024 17:08

[tutasdk] native RestClient based on hyper + rustls

7a25461

ganthern force-pushed the tutasdk-service-executor branch from 9728869 to c481f16 Compare October 14, 2024 15:10

ganthern merged commit c481f16 into master Oct 14, 2024
4 checks passed

ganthern deleted the tutasdk-service-executor branch October 14, 2024 16:08

ganthern added this to the Next mail release milestone Oct 14, 2024

charlag added the state:not-testable "tested" for blind fixes or old code removal with no expected regressions label Oct 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tutasdk] sdk should be able to make service calls #7708

[tutasdk] sdk should be able to make service calls #7708

tuta-sudipg commented Oct 9, 2024 •

edited by ganthern

Loading

charlag Oct 10, 2024

tuta-sudipg Oct 11, 2024

charlag Oct 10, 2024 •

edited

Loading

ganthern Oct 10, 2024

charlag Oct 10, 2024

tuta-sudipg Oct 10, 2024

charlag Oct 10, 2024

ganthern Oct 10, 2024 •

edited

Loading

charlag Oct 10, 2024

ganthern Oct 14, 2024 •

edited

Loading

charlag Oct 10, 2024

tuta-sudipg Oct 10, 2024 •

edited

Loading

tuta-sudipg Oct 10, 2024

charlag Oct 10, 2024

tuta-sudipg Oct 11, 2024

charlag Oct 10, 2024

charlag Oct 10, 2024

charlag Oct 10, 2024

charlag Oct 10, 2024

charlag Oct 10, 2024

charlag Oct 10, 2024

paw-hub left a comment

[tutasdk] sdk should be able to make service calls #7708

[tutasdk] sdk should be able to make service calls #7708

Conversation

tuta-sudipg commented Oct 9, 2024 • edited by ganthern Loading

Test Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlag Oct 10, 2024 • edited Loading

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

ganthern Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ganthern Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuta-sudipg Oct 10, 2024 • edited Loading

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

paw-hub left a comment

Choose a reason for hiding this comment

tuta-sudipg commented Oct 9, 2024 •

edited by ganthern

Loading

charlag Oct 10, 2024 •

edited

Loading

ganthern Oct 10, 2024 •

edited

Loading

ganthern Oct 14, 2024 •

edited

Loading

tuta-sudipg Oct 10, 2024 •

edited

Loading