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.

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

fix(other): gmail messages not shown #1409

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

christer77
Copy link
Member

see issue

ref: RFC

@christer77 christer77 requested a review from kroky December 20, 2024 14:27
@indridieinarsson
Copy link
Contributor

May I add a comment?
The issue ( #1402 ) turns out to be really two different things. One is the inability of gmail to process BODY.PEEK[0.1] (but I think BODY[0.1] is fine, so might consider changing that one back). And I think this PR fixes that (but I'm not quite sure what info we might be missing by changing from [0.1] to [1]? )

The other thing is that in the combined view, the automatic check and renewal of the xoauth2 bearer token (which is valid for 1 hour after renewal) does not take place, and this PR doesn't fix that.

I made some code that seems to fix this part of it : indridieinarsson@3c74c2a
For the dedicated mailbox pages, the old code checks which mail server it is serving, and checks that server for expiry. But the request for the combined view yields and empty list of servers so no check is performed. In this case, I just check expiry for all available servers. A bit course, but maybe harmless.

@christer77
Copy link
Member Author

May I add a comment? The issue ( #1402 ) turns out to be really two different things. One is the inability of gmail to process BODY.PEEK[0.1] (but I think BODY[0.1] is fine, so might consider changing that one back). And I think this PR fixes that (but I'm not quite sure what info we might be missing by changing from [0.1] to [1]? )

The other thing is that in the combined view, the automatic check and renewal of the xoauth2 bearer token (which is valid for 1 hour after renewal) does not take place, and this PR doesn't fix that.

I made some code that seems to fix this part of it : indridieinarsson@3c74c2a For the dedicated mailbox pages, the old code checks which mail server it is serving, and checks that server for expiry. But the request for the combined view yields and empty list of servers so no check is performed. In this case, I just check expiry for all available servers. A bit course, but maybe harmless.

hello @indridieinarsson,

Can you push your suggestion in current PR please?

@indridieinarsson
Copy link
Contributor

indridieinarsson commented Jan 22, 2025

May I add a comment? The issue ( #1402 ) turns out to be really two different things. One is the inability of gmail to process BODY.PEEK[0.1] (but I think BODY[0.1] is fine, so might consider changing that one back). And I think this PR fixes that (but I'm not quite sure what info we might be missing by changing from [0.1] to [1]? )
The other thing is that in the combined view, the automatic check and renewal of the xoauth2 bearer token (which is valid for 1 hour after renewal) does not take place, and this PR doesn't fix that.
I made some code that seems to fix this part of it : indridieinarsson@3c74c2a For the dedicated mailbox pages, the old code checks which mail server it is serving, and checks that server for expiry. But the request for the combined view yields and empty list of servers so no check is performed. In this case, I just check expiry for all available servers. A bit course, but maybe harmless.

hello @indridieinarsson,

Can you push your suggestion in current PR please?

Sorry for the late answer - I'm quite busy these days.

I honestly don't know how I can push my branch into an existing PR. My code is available in it's own branch here : https://github.com/indridieinarsson/cypht/tree/issue_1402. Feel free to merge or cherry pick what you want.

My code is not ready. It works, in the way that one we navigate to the combined mailbox, the oauth token is renewed if needed. However, if the site is left open for a while, all gmail messages will disappear after about an hour (unless the page is reloaded). The reason is that after an hour, the token has expired, and the code that checks and renews it is only run on page load.
I was thinking this should rather be done closer to the imap module. I was thinking to catch the reply from the imap server (where it tells us that the token has expired), and trigger the renewal from there. But that's a bit more work, and I haven't found the time for it yet.

edit: forgot to mention @christer77 so he actually sees my message.

@marclaporte marclaporte marked this pull request as draft January 23, 2025 13:25
@christer77 christer77 force-pushed the Gmail-messages-not-shown branch from 663089c to f47a2ff Compare January 28, 2025 03:10
@christer77
Copy link
Member Author

May I add a comment? The issue ( #1402 ) turns out to be really two different things. One is the inability of gmail to process (but I think is fine, so might consider changing that one back). And I think this PR fixes that (but I'm not quite sure what info we might be missing by changing from [0.1] to [1]? )
The other thing is that in the combined view, the automatic check and renewal of the xoauth2 bearer token (which is valid for 1 hour after renewal) does not take place, and this PR doesn't fix that.
I made some code that seems to fix this part of it : indridieinarsson@3c74c2a For the dedicated mailbox pages, the old code checks which mail server it is serving, and checks that server for expiry. But the request for the combined view yields and empty list of servers so no check is performed. In this case, I just check expiry for all available servers. A bit course, but maybe harmless.BODY.PEEK[0.1]``BODY[0.1]

hello @indridieinarsson,
Can you push your suggestion in current PR please?

Sorry for the late answer - I'm quite busy these days.

I honestly don't know how I can push my branch into an existing PR. My code is available in it's own branch here : https://github.com/indridieinarsson/cypht/tree/issue_1402. Feel free to merge or cherry pick what you want.

My code is not ready. It works, in the way that one we navigate to the combined mailbox, the oauth token is renewed if needed. However, if the site is left open for a while, all gmail messages will disappear after about an hour (unless the page is reloaded). The reason is that after an hour, the token has expired, and the code that checks and renews it is only run on page load. I was thinking this should rather be done closer to the imap module. I was thinking to catch the reply from the imap server (where it tells us that the token has expired), and trigger the renewal from there. But that's a bit more work, and I haven't found the time for it yet.

edit: forgot to mention @christer77 so he actually sees my message.
Normally, we have a request that allows us to do this verification.Are you sure that it wasn't your session that had expressed and that you had to reconnect?
add_handler('ajax_hm_folders', 'imap_oauth2_token_check', true, 'imap', 'load_imap_servers_from_config', 'after');

@christer77
Copy link
Member Author

May I add a comment? The issue ( #1402 ) turns out to be really two different things. One is the inability of gmail to process (but I think is fine, so might consider changing that one back). And I think this PR fixes that (but I'm not quite sure what info we might be missing by changing from [0.1] to [1]? )
The other thing is that in the combined view, the automatic check and renewal of the xoauth2 bearer token (which is valid for 1 hour after renewal) does not take place, and this PR doesn't fix that.
I made some code that seems to fix this part of it : indridieinarsson@3c74c2a For the dedicated mailbox pages, the old code checks which mail server it is serving, and checks that server for expiry. But the request for the combined view yields and empty list of servers so no check is performed. In this case, I just check expiry for all available servers. A bit course, but maybe harmless. BODY.PEEK[0.1]BODY[0.1] ``

hello @indridieinarsson,
Can you push your suggestion in current PR please?

Sorry for the late answer - I'm quite busy these days.
I honestly don't know how I can push my branch into an existing PR. My code is available in it's own branch here : https://github.com/indridieinarsson/cypht/tree/issue_1402. Feel free to merge or cherry pick what you want.
My code is not ready. It works, in the way that one we navigate to the combined mailbox, the oauth token is renewed if needed. However, if the site is left open for a while, all gmail messages will disappear after about an hour (unless the page is reloaded). The reason is that after an hour, the token has expired, and the code that checks and renews it is only run on page load. I was thinking this should rather be done closer to the imap module. I was thinking to catch the reply from the imap server (where it tells us that the token has expired), and trigger the renewal from there. But that's a bit more work, and I haven't found the time for it yet.
edit: forgot to mention @christer77 so he actually sees my message.
Normally, we have a request that allows us to do this verification.Are you sure that it wasn't your session that had expressed and that you had to reconnect?
add_handler('ajax_hm_folders', 'imap_oauth2_token_check', true, 'imap', 'load_imap_servers_from_config', 'after');

@indridieinarsson

@indridieinarsson
Copy link
Contributor

Hello - I'm trying to get this to work.
This issue arises (e.g.) in the combined view. The combined view pages don't have a particular mail server attached to them, which leads to them not renewing the oauth bearer token. I made a partial fix, which re-authenticates brute-force for all imap servers within the Hm_Handler_imap_oauth2_token_check handler

But it's only a partial fix, since the combined pages refresh the message list regularly (via ajax, so without reloading the page). After 1hr, the gmail oauth bearer token becomes invalid, so we get a statuscode 400 when retreiving the gmail message list.

At this point, I'm unsure what to do, as the handler modules seem to only run on page load, which isn't happening here.

Could someone explain how the "handler modules" work and how I could trigger the process function on it from within the authenticate function on the HM_Imap class, without reloading the page?

@indridieinarsson
Copy link
Contributor

@christer77 @kroky
Ok, much ado about nothing. Seems the issue is fixed with the current PR. I was testing on a version since december, where apparently snoozing messages wasn't being called when logged in through an IMAP account. I nuked my config in the meantime, so can't bisect it properly.

(the snooze worker goes on in the background and runs the oauth refresh. Snooze is part of the imap module, just as the oauth stuff, so should not be accidentally disabled)

So, with the current master, this PR fixes it, for me at least.

@marclaporte marclaporte marked this pull request as ready for review February 15, 2025 12:39
@christer77 christer77 requested review from kroky and removed request for kroky March 10, 2025 07:02
@christer77 christer77 force-pushed the Gmail-messages-not-shown branch from f47a2ff to 19b55c1 Compare March 12, 2025 14:22
@christer77 christer77 marked this pull request as draft March 12, 2025 14:24
@christer77 christer77 force-pushed the Gmail-messages-not-shown branch from 19b55c1 to 56afcac Compare March 13, 2025 06:56
@christer77 christer77 marked this pull request as ready for review March 13, 2025 07:00
@christer77 christer77 requested a review from kroky March 13, 2025 07:04
@@ -938,9 +938,11 @@ public function get_message_list($uids, $raw=false, $include_content_body = fals
$x_auto_bcc = '';
$x_snoozed = '';
$count = count($vals);
$head_finded = false;
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to be nit-picky but proper past tense of the verb find is found...

@@ -1035,6 +1040,27 @@ public function get_message_structure($uid) {
return $struct;
}

private function parseEmailBody($raw_data) {
Copy link
Member

Choose a reason for hiding this comment

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

This function will deal with fraction of the cases as there are numerous transfer encodings, mime details and other stuff that might break here. Can you use zbateson/mail-mime-parser that is already included in Cypht - it has methods to get the first plaintext part of html part or whatever you need. If it is html part, then, maybe you need to strip tags first before trimming?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants