feat(PhishingDetection): Add support for the $Phishing IMAP flag
#12187
Conversation
DerDreschner
requested review from
ChristophWurst,
GretaD and
kesselb
as code owners
|
Thanks for opening your first pull request in this repository! ✌️ |
DerDreschner
force-pushed
the
feature/support-phishing-flag
branch
from
38aac97 to
36e7f72
Compare
ChristophWurst
left a comment
ChristophWurst
left a comment
There was a problem hiding this comment.
Thanks a lot!
This looks very good!
I've added some comments for clean-up and coding style
| $messagesTable = $schema->getTable('mail_messages'); | ||
| $messagesTable->addColumn('flag_phishing', Types::BOOLEAN, [ | ||
| 'notnull' => false, | ||
| 'default' => false, | ||
| ]); |
There was a problem hiding this comment.
The messages table is rather big already, so a migration is expensive. The flag data for the phishing service is coming right from IMAP, not the DB. Therefore I suggest we remove the migration and $phishing flag persistence.
There was a problem hiding this comment.
I'm on your side. I've only put the flag persistence into the PR to be consistent with the other flag implementations, which are stored there as well ($junk, $nonjunk, $mdnsent). But that only happens because the other flags can be actively manipulated via the mail app and have to be synced to the IMAP server later, right?
The other question I have is what happens to the flag (and my code) when the IMAP server is down when the user opens a message, as these are being fetched directly from the server? I would guess that this case is (or should be) supported somehow as the mail messages are stored in the database? Or is it just for performance reasons?
There was a problem hiding this comment.
The flags are in the database for quick filtering and for the sections of the priority inbox. This allows us to build a list of threads for a specific search query without involving the (slow) IMAP server.
For phishing I'd see filtering as problematic because users would only see emails flagged by the server. They would not see emails that are only considered as phishing by our app.
Technically, we could process every email and check it for phishing and persist that state (e.g. via that flag). Then the user filtering would be accurate. Something for another day 😉
Offline support is limited. The oc_mail_messages contents alone are insufficient to show an email. You still need IMAP to fetch the body. So either that works and you can also do the phishing processing or you won't see the email anyway.
DerDreschner
changed the title
$Phishing IMAP flag and fix minor bugs$Phishing IMAP flag
DerDreschner
force-pushed
the
feature/support-phishing-flag
branch
from
6ac0cca to
cbb6a4d
Compare
|
Regarding the coding style: My own philosophy is "only coding styles that are applied by linters are good coding styles". I've seen that there is a rule for checking the How is this being handled here? Is it okay to adjust the |
Very good point. I was surprised the whitespace wasn't already caught by our cs fixer rules. I think this change would definitely make sense for https://github.com/nextcloud/coding-standard. The enforced strict type is probably also a good idea to have there 👍 |
ChristophWurst
left a comment
ChristophWurst
left a comment
There was a problem hiding this comment.
Tested and works. Code looks very clean now 👏
|
One last challenge, and sorry for forgetting to tell you about this convention before the PR, could you clean up the git history? I prefer a linear git history with somewhat atomic commits. The reason is that it allows for easier reverts if something goes wrong, and that we use release automation, which generates changelogs based on semantic commits. I can also clean it up for you if it turns out to be tricky with the current history :) |
Signed-off-by: David Dreschner <github-2017@dreschner.net>
DerDreschner
force-pushed
the
feature/support-phishing-flag
branch
from
cbb6a4d to
6c01c8a
Compare
|
@ChristophWurst : I've squashed all my commits into one. 😃 Also in the other PR. Thanks also for the explanation! Makes sense to me now. I'll have a look about the CS enforcement in the other repository. |
|
Looks like someone needs a manual re-run due to some DNS or server issues @ChristophWurst. Would trigger it myself, but... 😉 |
2 participants
This PR introduces support for the
$PhishingIMAP flag defined since IMAP4rev2 (see RFC9051). When the flag is set by the delivery agent (e.g., Dovecot), the following warning is being shown to the user:Important: For compatibility reasons, the warning is only being shown when both the
$Junk/Junkand$Phishingflags are present. That matches the behavior described in the RFC with the exception that Thunderbird is being supported as well, although it uses a non-standard flag for junk/non-junk flagging.Additionally, I've added some unit tests for the phishing warning front-end component.
Closes #11169