-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
added SMTP monitor #5489
base: master
Are you sure you want to change the base?
added SMTP monitor #5489
Conversation
This comment was marked as spam.
This comment was marked as spam.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a reasonable start.
I have left two comments, but mostly this needs better hinting in the frontend what this monior does and what it does not monitor.
server/monitor-types/smtp.js
Outdated
/** | ||
* @param {*} smtpSecurity the user's SMTP security setting | ||
* @returns {boolean} True if this should test SMTPS | ||
*/ | ||
isSMTPS(smtpSecurity) { | ||
return smtpSecurity === "secure"; | ||
} | ||
|
||
/** | ||
* @param {*} smtpSecurity the user's SMTP security setting | ||
* @returns {boolean} True if this should not attempt STARTTLS, even if it is available | ||
*/ | ||
isIgnoreTLS(smtpSecurity) { | ||
return smtpSecurity === "nostarttls"; | ||
} | ||
|
||
/** | ||
* @param {*} smtpSecurity the user's SMTP security setting | ||
* @returns {boolean} True if this should always test STARTTLS | ||
*/ | ||
isRequireTLS(smtpSecurity) { | ||
return smtpSecurity === "starttls"; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please inline all of these. Pulling a function from this is harder to read than nessesary
@@ -329,6 +332,15 @@ | |||
</select> | |||
</div> | |||
|
|||
<div v-if="monitor.type === 'smtp'" class="my-3"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add a helptext what this monitor actually does.
People will infer that this is sending mails otherwise..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve pushed the changes based upon what I saw elsewhere in the file; let me know if they weren’t correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is not what I meant.
I meant specifying as a helptext what kind of check the selected option does.
What does Ignore STARTTLS
actually check?
It does not send a mail as I would expect => please specify in the helptext that it just tries to connect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Thanks for clarifying.
I’ll describe them here, and also add them to the codebase:
Ignore STARTTLS
simply connects, saysHELO
and looks for a correct responseUse STARTTLS
connects, saysHELO
, looks forSTARTTLS
in the response, issues the command, and verifies the certificateSMTPS
connects over TLS, verifies the certificate, saysHELO
and looks for a correct response
Details are here. Update to the code coming shortly.
Tick the checkbox if you understand [x]:
Description
Add SMTP monitor
This allows users to monitor SMTP/SMTPS servers. They can optionally verify STARTTLS settings, or for SMTPS, they can just verify the TLS.
Type of change
Please delete any options that are not relevant.
Checklist
Screenshots (if any)