the-djmaze / snappymail

Simple, modern & fast web-based email client
https://snappymail.eu
GNU Affero General Public License v3.0
1.01k stars 121 forks source link

Log ClientExceptions to logfile #705

Closed cm-schl closed 1 year ago

cm-schl commented 1 year ago

I'm actually writing a plugin for SnappyMail to add additional mailboxes by a ldap query. There I had a problem with the domain check that is done in https://github.com/the-djmaze/snappymail/blob/2634edd842a45ff9d3c8e5a5d3a9a34ba59b200c/snappymail/v/0.0.0/app/libraries/RainLoop/Model/Account.php#L132

If the domain found in ldap does not exist in the domain configuration of SnappyMail in theory this function would throw a ClientException to inform the user that the domain is not allowed / unknown. As this interaction is not possible when a plugin is trying to register a new mail address to SnappyMail the user (and developer) of the plugin can't get informed about this problem.

Describe the solution you'd like I would like to propose a change to snappymail/v/0.0.0/app/libraries/RainLoop/Exceptions/ClientException.php so that a thrown error is also logged in the log file. This way the admin / developer is informed what happened and why a login is not possible.

the-djmaze commented 1 year ago

Would this help?

https://github.com/the-djmaze/snappymail/blob/2634edd842a45ff9d3c8e5a5d3a9a34ba59b200c/snappymail/v/0.0.0/app/libraries/RainLoop/ActionsAdmin.php#L256-L270

cm-schl commented 1 year ago

sorry, i don't get it... 🙂.

The idea behind my request is to give the developer / admin the possibility to see if a user (or a plugin) tries to login with a mail address that is not in the list if configured domains. Maybe this is only a side effect how I'm trying to register a new account out of my plugin - there I use RainLoop\Model\AdditionalAccount::NewInstanceFromCredentials to add a new additional account. If the domain is not in the list of configured domains I can't see any error inside the log file of SnappyMail.

the-djmaze commented 1 year ago

What i mean is: Should your plugin check if Domain is available BEFORE trying to login with NewInstanceFromCredentials ?

My code above does three things for that:

  1. resolveLoginCredentials() tries to make $sEmail as valid as possible according to all the rules, plugins, etc.
  2. DomainProvider()->Load() loads the Domain matching the $sEmail
  3. $oDomain->ValidateWhiteList($sEmail, $sLogin) checks if $sEmail is whitelisted

Step 1 is currently a private/protected function but you get the idea to:

  1. check if $sEmail is allowed (if not, log it yourself)
  2. when allowed add it as AdditionalAccount
cm-schl commented 1 year ago

Ok thanks for the explanation! I will add a check to my plugin 🙂👍

the-djmaze commented 1 year ago

If you want access to resolveLoginCredentials() i can change the scope.