Support cyrus server #3457

florentos17 · 2025-01-30T10:39:16Z

each commit has a self-explanatory message, but do not hesitate if you have any question !

i have tested the changes with both tmail-backend and cyrus to check that the expected behaviour happens, and it looks all good to me.

github-actions · 2025-01-30T10:49:30Z

This PR has been deployed to https://linagora.github.io/tmail-flutter/3457.

florentos17 · 2025-01-30T13:24:09Z

the force push was to fix the 'unused import' warnings and prefix each commit message with TF-2305

florentos17 · 2025-01-30T15:58:24Z

regarding the way URIExtension::toQualifiedUrl works: i did some adjustments to the unit tests because i feel like the wrong behaviour was expected: to me, if a trailing slash is advertized, then it should remain. I say this because it is the way cyrus works, but i dont know if this is universal so definitely tell me what you think about that.

with the current changes, tmail-flutter works well with tmail-backend and with cyrus. this issue (Session: handle relative URL) mentions compatibility with fastmail. i dont have a fastmail server at hand but if you have a URL for one i'd be happy to test !

dab246 · 2025-02-03T03:23:18Z

model/lib/extensions/session_extension.dart

+      try {
+        var principalsCapability = getCapabilityProperties<DefaultCapability>(
+            AccountId(Id(username.value)),
+            CapabilityIdentifier(Uri.parse('urn:ietf:params:jmap:principals')));


Please define this capability

I did not find it in spec. It is Cyrus customization? Can you share session example of Cyrus here?

here is one of the accountCapabilities from the cyrus session:

"urn:ietf:params:jmap:principals": { "currentUserPrincipalId": "bob", "urn:ietf:params:jmap:calendars": { "accountId": "bob", "account": null, "mayGetAvailability": true, "sendTo": { "imip": "mailto:[email protected]" } } },

it is the only place where the email address appears in the session, so benoit suggested in this comment to fetch the address from there if necessary

https://jmap.io/spec-sharing.html#principals

@hoangdat what do you think ? Indeed I dont think there is any doc about fetching the email from there ; the idea comes from @chibenwa 's comment

model/lib/extensions/session_extension.dart

dab246 · 2025-02-03T03:24:42Z

model/lib/extensions/session_extension.dart

@@ -53,6 +52,28 @@ extension SessionExtension on Session {
    }
  }

+  String? getEmailAddress() {


We also should write unit test for this function

I still prefer getUsername() or getOwnEmailAddress()?

Unit tests done !

If we want to get the username, we should simply return session.username. This method is used to retrieve the full email address (sometimes used as the username, sometimes not). Do you want me to change getEmailAddress() to getOwnEmailAddress() ?

term email address is not specific for this. The reason for usename is the current account use the app, we need to distinguish with other email addresses in delegate mode or multiple account. But ok with getOwnEmailAddress()

ok done, i changed the name !

core/lib/presentation/extensions/url_extension.dart

core/lib/presentation/extensions/uri_extension.dart

lib/features/composer/presentation/extensions/create_email_request_extension.dart

lib/features/home/domain/extensions/session_extensions.dart

lib/features/identity_creator/presentation/identity_creator_controller.dart

lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart

lib/features/composer/presentation/extensions/create_email_request_extension.dart

lib/features/thread/presentation/thread_view.dart

tddang-linagora · 2025-02-03T03:54:13Z

model/lib/extensions/session_extension.dart

+  String getDownloadUrl(String jmapUrl) {
+    final downloadUrlValid = downloadUrl.toQualifiedUrl(baseUrl: Uri.parse(jmapUrl));


Why is there a need to change it?

because

the jmapUrl attribute cannot be defaulted when using a cyrus server: it is not advertized in the session ;

it was already always provided anyway.

the jmapUrl attribute cannot be defaulted when using a cyrus server: it is not advertized in the session

Not really understand it.
Moreover, we also support other back-end: Stalwart, Cyrus, James. Keep it optional will keep flexible.

Previously the base URL jmapUrl attribute was optional and the default value was the downloadUrl provided in the session. For Tmail-backend, this is a full URL (so it's ok), but for Cyrus it is only the relative route:

"downloadUrl": "/jmap/download/{accountId}/{blobId}/{name}?accept={type}", "uploadUrl": "/jmap/upload/{accountId}/",

but I also made changes to the toQualifiedUrl method so that both cases are handled.

Thinking back about it, you are right that keeping it optional will keep it flexible, it is better.

On my latest commit you can see that I reverted it, but then I had to handle the case where the URL cannot be found at all by throwing an exception, is that ok ?

tddang-linagora · 2025-02-03T03:54:23Z

model/lib/extensions/session_extension.dart

+  Uri getUploadUri(AccountId accountId, String jmapUrl) {
+    final uploadUrlValid = uploadUrl.toQualifiedUrl(baseUrl: Uri.parse(jmapUrl));


we also support other back-end: Stalwart, Cyrus, James. Keep it optional will keep flexible.

lib/features/mailbox_dashboard/presentation/mailbox_dashboard_view_web.dart

lib/features/thread/presentation/thread_controller.dart

lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart

lib/features/composer/presentation/extensions/create_email_request_extension.dart

lib/features/manage_account/presentation/menu/settings/settings_first_level_view.dart

dab246 · 2025-02-04T03:35:26Z

@florentos17 Please confirm Done in the comments that you have fixed it. Thanks

hoangdat · 2025-02-04T06:53:25Z

core/test/presentation/extensions/uri_extension_test.dart

@@ -20,7 +20,7 @@ void main() {
      final baseUrl = Uri.parse('https://domain.com');
      final sourceUrl = Uri.parse('/jmap/');

-      final qualifiedUrlExpected = Uri.parse('https://domain.com/jmap');
+      final qualifiedUrlExpected = Uri.parse('https://domain.com/jmap/');


Why we need this? In what cases?

IMO, it is our normalization, please keep it in uniform.

cyrus advertizes the following in the session: "uploadUrl": "/jmap/upload/{accountId}/"

when querying https://localhost/jmap/upload/bob/, the upload works, but with https://localhost/jmap/upload/bob (without the trailing /), I get a 404 Not Found error.

okie, in this case, make it uniform, keep / for all qualified url, and update all the tests too.

But tmail-backend works the other way around: no / advertized, no / needed, and it does not work if the / is there...

So I feel like the solution is to keep the / if it is advertized, and not add it if not advertized ? But I agree that it is not uniform.

hoangdat · 2025-02-04T07:19:40Z

core/lib/presentation/extensions/uri_extension.dart

+      log('SessionUtils::toQualifiedUrl():baseUrlValid: $baseUrlValid | sourceUrlValid: $sourceUrlValid');
+      final qualifiedUrl = baseUrlValid + sourceUrlValid;
+      log('SessionUtils::toQualifiedUrl():qualifiedUrl: $qualifiedUrl');
+      return Uri.parse(qualifiedUrl);


What happens if parse failed? Can we use tryParse instead?

any update? @florentos17

What default behaviour would you expect if the parse fails and null is returned ?

The methods that call toQualifiedUrl (getQualifiedApiUrl, getDownloadUrl, getUploadUrl) mostly cannot work with null, so errors have to be thrown. But then it would be easier to just let toQualifiedUrl throw the error ?

hoangdat · 2025-02-04T07:33:35Z

model/lib/extensions/session_extension.dart

+  String getDownloadUrl(String jmapUrl) {
+    final downloadUrlValid = downloadUrl.toQualifiedUrl(baseUrl: Uri.parse(jmapUrl));


the jmapUrl attribute cannot be defaulted when using a cyrus server: it is not advertized in the session

Not really understand it.
Moreover, we also support other back-end: Stalwart, Cyrus, James. Keep it optional will keep flexible.

hoangdat · 2025-02-04T07:33:51Z

model/lib/extensions/session_extension.dart

+  Uri getUploadUri(AccountId accountId, String jmapUrl) {
+    final uploadUrlValid = uploadUrl.toQualifiedUrl(baseUrl: Uri.parse(jmapUrl));


we also support other back-end: Stalwart, Cyrus, James. Keep it optional will keep flexible.

hoangdat · 2025-02-04T07:39:09Z

model/lib/extensions/session_extension.dart

+      try {
+        var principalsCapability = getCapabilityProperties<DefaultCapability>(
+            AccountId(Id(username.value)),
+            CapabilityIdentifier(Uri.parse('urn:ietf:params:jmap:principals')));


I did not find it in spec. It is Cyrus customization? Can you share session example of Cyrus here?

hoangdat · 2025-02-04T07:40:56Z

model/lib/extensions/session_extension.dart

@@ -53,6 +52,28 @@ extension SessionExtension on Session {
    }
  }

+  String? getEmailAddress() {


I still prefer getUsername() or getOwnEmailAddress()?

hoangdat · 2025-02-04T07:43:48Z

model/lib/extensions/session_extension.dart

+        String wrappedAddress = ((((principalsCapability?.properties
+            ?.values.last) as Map<String, dynamic>)
+            .values.last) as Map<String, dynamic>)
+            .values.toString();


not a fan of hard force casting and prefer strong typed for this

something like that ?

final sendTo = principalsCapability?.properties?['urn:ietf:params:jmap:calendars']?['sendTo']; if (sendTo is Map<String, dynamic>) { final wrappedAddress = sendTo['imip']; if (wrappedAddress is String && wrappedAddress.startsWith('mailto:')) { String address = wrappedAddress.substring("mailto:".length); return address.isEmail ? address : null; } }

is that ok @hoangdat ?

lib/features/thread/presentation/thread_controller.dart

lib/features/mailbox_dashboard/presentation/widgets/search_filters/search_filter_button.dart

hoangdat · 2025-02-05T11:08:50Z

hi @florentos17, I rebased the branch from customer branch, so some files conflict. Please try to help us resolve it.

This ticket is the first priority. please let us know when you are done in resolving conflict and comments.

Thanks

florentos17 · 2025-02-05T13:46:18Z

@hoangdat when you forced pushed on master, it "added" 19 commits to this PR: commits that were previously on the master branch but are not anymore since the force push.

among these 19 commits:

12 are actually already on the new master (just a different hash), so i will force push on this branch to remove them from the current PR
7 are not on the new master at all... could that be an error ? here they are:

47b842d25 TF-3189 composer now correctly encodes subaddresses
20c4720c3 TF-3189 new `reply to` field in the mail composer
d939ffb1c TF-3189 new confirmation popup when enabling subaddressing for a folder
0f61e3de8 TF-3189 subaddressing features only shown if supported by the server
810e304c1 TF-3189 new option to copy a folder's subaddress
ed0b14c35 TF-3189 new option to enable/disable subaddressing for a personal folderc
...
1a58542ab TF-3265 Fix double scrolling composer

do you want me to leave them on the current PR so that they are reinstated ? or maybe cherry-pick them back yourself onto master ?

…rchive` does not exist

hoangdat · 2025-02-06T02:51:50Z

lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart

@@ -2755,7 +2755,7 @@ class MailboxDashBoardController extends ReloadableController
    dispatchAction(SelectionAllEmailAction());
  }

-  String get baseDownloadUrl => sessionCurrent?.getDownloadUrl(jmapUrl: dynamicUrlInterceptors.jmapUrl) ?? '';
+  String get baseDownloadUrl => sessionCurrent?.getDownloadUrl(dynamicUrlInterceptors.jmapUrl!) ?? '';


not a big fan of ! without pre-check

done, i reverted the change

lib/features/identity_creator/presentation/identity_creator_controller.dart

model/lib/extensions/session_extension.dart

lib/features/contact/presentation/contact_controller.dart

hoangdat · 2025-02-06T03:14:47Z

lib/features/email/presentation/controller/single_email_controller.dart

@@ -530,7 +530,7 @@ class SingleEmailController extends BaseController with AppLoaderMixin {
      )));
    } else {
      if (session != null && accountId != null) {
-        final baseDownloadUrl = session!.getDownloadUrl(jmapUrl: dynamicUrlInterceptors.jmapUrl);
+        final baseDownloadUrl = session!.getDownloadUrl(dynamicUrlInterceptors.jmapUrl!);


not safety: ! without pre-check

done, i reverted the change

hoangdat · 2025-02-06T03:33:03Z

model/lib/extensions/session_extension.dart

+          .values.last) as Map<String, dynamic>)
+          .values.toString();
+
+      String address = wrappedAddress.substring("(mailto:".length, wrappedAddress.length - ")".length);


Still not find the docs said that this field can use as email address of current account. Can you more details?

i answered on one of your earlier comments: #3457 (comment)

…ommit 5d91050)

…ring` and throw an exception if necessary (fixup for commit 5d91050)

…ption if necessary (fixup for commit 7a2380c)

dab246 · 2025-02-07T04:49:16Z

lib/features/composer/domain/usecases/upload_attachment_interactor.dart

    CancelToken? cancelToken,
  }) async* {
    try {
+      if (uploadUri == null) throw UnknownUriException();


We should not put the uploadUri == null check in this interactor. It is against the interactor's functionality. You should check it before calling this interactor. uploadUri is required in this interactor.

This is what I did at first, but it means the interactor is not even called in case of a null uploadUri, so there is no UploadAttachmentFailure, and the red toast notification "upload attachment failed" does not show on the app.

I thought it would be better for the UX to have the red toast notification show up, but I understand the concern. Should I change it to put the check before ? Maybe I can throw a GetUploadUriFailure earlier to still have the toast notification ?

Yes, you check uploadUri != null before calling the interactor. And throw UploadAttachmentFailure(UnknownUriException()) exception as soon as you check it for null

dab246 · 2025-02-07T04:55:15Z

model/lib/extensions/session_extension.dart

+    } else if (uploadUrl.hasOrigin) {
+      uploadUrlValid = uploadUrl;
+    } else {
+      return null;


Suggested change

return null;

throw UnknownUriException();

model/lib/extensions/session_extension.dart

lib/features/identity_creator/presentation/identity_creator_controller.dart

dab246 · 2025-02-07T06:45:59Z

lib/features/identity_creator/presentation/identity_creator_controller.dart

          .map((identity) => identity.toEmailAddressNoName())
          .toSet()
          .toList();
+
+      listEmailAddressDefault.add(EmailAddress(null, session?.getOwnEmailAddress()));


why do we need it?

cyrus does not advertize the address like tmail does, so without the change the list of possible addresses is empty (clicking on the widget does nothing):

with the change, the default address appears:

and I checked, it does not duplicate the address if it was already there when using tmail

So you should check if listEmailAddressDefault.isEmpty then add getOwnEmailAddress. Avoid duplicate emails.

no, it is already good, there is already no duplication.
i can add a listEmailAddressDefault.isEmpty but it makes no difference

dab246 · 2025-02-07T06:49:45Z

model/lib/extensions/session_extension.dart

+      String wrappedAddress = ((((principalsCapability?.properties
+          ?.values.last) as Map<String, dynamic>)
+          .values.last) as Map<String, dynamic>)
+          .values.toString();
+
+      String address = wrappedAddress.substring("(mailto:".length, wrappedAddress.length - ")".length);


IMO, Getting the email this way is inefficient, because we don't know exactly which field contains the email, the order of the keys of the returned json will be different on the servers, so truncating the string of the last element is redundant. So we should skip this.

the issue is that this is the only place where the full email address is advertized by cyrus: without this, the address wil be unknown.

final sendTo = principalsCapability?.properties?['urn:ietf:params:jmap:calendars']?['sendTo']; if (sendTo is Map<String, dynamic>) { final wrappedAddress = sendTo['imip']; if (wrappedAddress is String && wrappedAddress.startsWith('mailto:')) { String address = wrappedAddress.substring("mailto:".length); return address.isEmail ? address : null; } }

would that be ok ?

…for commit 7a2380c)

florentos17 temporarily deployed to PR-3457 January 30, 2025 10:39 — with GitHub Actions Inactive

florentos17 self-assigned this Jan 30, 2025

florentos17 force-pushed the Support-Cyrus-Server branch from 291f97a to 30f5323 Compare January 30, 2025 13:22

florentos17 temporarily deployed to PR-3457 January 30, 2025 13:22 — with GitHub Actions Inactive

florentos17 temporarily deployed to PR-3457 January 30, 2025 15:50 — with GitHub Actions Inactive

dab246 reviewed Feb 3, 2025

View reviewed changes

model/lib/extensions/session_extension.dart Outdated Show resolved Hide resolved

dab246 reviewed Feb 3, 2025

View reviewed changes

core/lib/presentation/extensions/url_extension.dart Outdated Show resolved Hide resolved

dab246 reviewed Feb 3, 2025

View reviewed changes

core/lib/presentation/extensions/uri_extension.dart Outdated Show resolved Hide resolved

dab246 reviewed Feb 3, 2025

View reviewed changes

lib/features/composer/presentation/extensions/create_email_request_extension.dart Outdated Show resolved Hide resolved

dab246 reviewed Feb 3, 2025

View reviewed changes

lib/features/home/domain/extensions/session_extensions.dart Outdated Show resolved Hide resolved

dab246 reviewed Feb 3, 2025

View reviewed changes

lib/features/identity_creator/presentation/identity_creator_controller.dart Outdated Show resolved Hide resolved

dab246 reviewed Feb 3, 2025

View reviewed changes

lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart Outdated Show resolved Hide resolved

tddang-linagora reviewed Feb 3, 2025

View reviewed changes

dab246 reviewed Feb 4, 2025

View reviewed changes

lib/features/composer/presentation/extensions/create_email_request_extension.dart Outdated Show resolved Hide resolved

dab246 reviewed Feb 4, 2025

View reviewed changes

lib/features/manage_account/presentation/menu/settings/settings_first_level_view.dart Outdated Show resolved Hide resolved

hoangdat reviewed Feb 4, 2025

View reviewed changes

florentos17 temporarily deployed to PR-3457 February 4, 2025 09:59 — with GitHub Actions Inactive

tddang-linagora previously approved these changes Feb 4, 2025

View reviewed changes

florentos17 dismissed tddang-linagora’s stale review via 87833ba February 4, 2025 10:56

florentos17 temporarily deployed to PR-3457 February 4, 2025 10:56 — with GitHub Actions Inactive

hoangdat force-pushed the master branch from 050a885 to 29e8fa7 Compare February 5, 2025 11:06

florentos17 added 2 commits February 5, 2025 17:12

TF-2305 Swiping left on a mail moves it to the Trash folder when `A…

58160a6

…rchive` does not exist

TF-2305 Cancel the "move to trash if no archive" feature

99b53ac

florentos17 force-pushed the Support-Cyrus-Server branch from 7bced46 to 99b53ac Compare February 5, 2025 16:13

florentos17 temporarily deployed to PR-3457 February 5, 2025 16:20 — with GitHub Actions Inactive

hoangdat reviewed Feb 6, 2025

View reviewed changes

tddang-linagora reviewed Feb 6, 2025

View reviewed changes

lib/features/identity_creator/presentation/identity_creator_controller.dart Outdated Show resolved Hide resolved

model/lib/extensions/session_extension.dart Show resolved Hide resolved

hoangdat reviewed Feb 6, 2025

View reviewed changes

lib/features/contact/presentation/contact_controller.dart Outdated Show resolved Hide resolved

hoangdat reviewed Feb 6, 2025

View reviewed changes

florentos17 added 4 commits February 6, 2025 16:00

TF-2305 Applied suggestion from code review (fixup for commit 8d763f8)

6d24e0c

TF-2305 Rename getEmailAddress to getOwnEmailAddress (fixup for c…

f948ed5

…ommit 5d91050)

TF-2305 Change getOwnEmailAddress return type from String? to `St…

da3cd7b

…ring` and throw an exception if necessary (fixup for commit 5d91050)

TF-2305 Cancel making the jmapUrl argument optional and throw an exce…

fa3008d

…ption if necessary (fixup for commit 7a2380c)

florentos17 temporarily deployed to PR-3457 February 6, 2025 15:01 — with GitHub Actions Inactive

TF-2305 Removed unused import (fixup for commit 5d91050)

1958e96

florentos17 temporarily deployed to PR-3457 February 6, 2025 15:12 — with GitHub Actions Inactive

TF-2305 fixed failing tests (for commit 5d91050)

c960db5

florentos17 temporarily deployed to PR-3457 February 6, 2025 15:51 — with GitHub Actions Inactive

dab246 reviewed Feb 7, 2025

View reviewed changes

model/lib/extensions/session_extension.dart Outdated Show resolved Hide resolved

dab246 reviewed Feb 7, 2025

View reviewed changes

lib/features/identity_creator/presentation/identity_creator_controller.dart Outdated Show resolved Hide resolved

dab246 reviewed Feb 7, 2025

View reviewed changes

florentos17 added 3 commits February 7, 2025 11:12

fixup for commit 04c97c0

cc1e503

fixup for commit 5d91050

b4ad37f

fixup for commit 8d763f8

fc04807

florentos17 temporarily deployed to PR-3457 February 7, 2025 10:15 — with GitHub Actions Inactive

moved uploadUri null check to before calling the interactor (fixup …

9d14de7

…for commit 7a2380c)

florentos17 deployed to PR-3457 February 7, 2025 15:24 — with GitHub Actions View deployment

		String getDownloadUrl(String jmapUrl) {
		final downloadUrlValid = downloadUrl.toQualifiedUrl(baseUrl: Uri.parse(jmapUrl));

		Uri getUploadUri(AccountId accountId, String jmapUrl) {
		final uploadUrlValid = uploadUrl.toQualifiedUrl(baseUrl: Uri.parse(jmapUrl));

Support cyrus server #3457

Are you sure you want to change the base?

Support cyrus server #3457

Conversation

florentos17 commented Jan 30, 2025

github-actions bot commented Jan 30, 2025

florentos17 commented Jan 30, 2025 • edited Loading

florentos17 commented Jan 30, 2025

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

Choose a reason for hiding this comment

dab246 commented Feb 4, 2025

Choose a reason for hiding this comment

florentos17 Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

hoangdat Feb 6, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoangdat commented Feb 5, 2025

florentos17 commented Feb 5, 2025 • 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

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

florentos17 Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

florentos17 commented Jan 30, 2025 •

edited

Loading

florentos17 Feb 4, 2025 •

edited

Loading

hoangdat Feb 6, 2025 •

edited

Loading

florentos17 commented Feb 5, 2025 •

edited

Loading

florentos17 Feb 7, 2025 •

edited

Loading