-
Notifications
You must be signed in to change notification settings - Fork 30
Issue #156 - Change SSL help text to a Constant, remove ads #164
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
base: main
Are you sure you want to change the base?
Issue #156 - Change SSL help text to a Constant, remove ads #164
Conversation
… remove NextGen branding and ads from the message Signed-off-by: Jon Bartels <jon.bartels@teladochealth.com>
e443ee5
to
b75e606
Compare
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 understand the desire to not want to duplicate code, but I don't know if UIConstants is the right place to put connector specific data.
IMO, It is all on the client UI and never gets into anything connector specific on the server, so this is acceptable. |
I put it in UI Constants because multiple places showed this help text. There isn't any other common class to both WS and HTTP connectors that I could find. It is help text shown in the UI, so UIConstants seemed like the best choice. This isn't just duplicated code, its a whole block of HTML. Duplicating small strings maybe but an HTML block should never have been copy-pasted like the original. |
I’m late to the party here, but I think we should completely remove this warning and the scary icon/s that goes with it. There is no warning for http traffic (nor am I advocating for one here) which is arguably higher risk. When using https, we have reasonable defaults and the connection is reasonably secure. Further, the prior or the proposed warning does not give the user any beneficial or actionable information. There is no reason to believe that anything but the default cert store would be used and there is nothing inherently harmful by doing so. Also, mTLS is supported, it just needs to be configured with the client cert/key. We don’t currently provide a way to do that, but it can be done at the JVM level and the connection will pick it up and use it just fine. If we feel we need a message, then perhaps a link to the documentation for advanced configurations. |
Perhaps. I think we need to be sensitive to conditioned behavior for many users using this engine for many years. I would not remove it for this reason - it is perhaps a gentle reminder. |
public static final String SSL_WARNING_TEXT = "<html>\n" + | ||
"<body class=\"ssl-warning-panel\"><b><span style=\"color:#404040\">Important Notice:</span></b> The default system\n" + | ||
"certificate store will be used for this connection. As a result, certain security options are not available and mutual\n" + | ||
"authentication (two-way authentication) is not supported.\n" + | ||
"</body>\n" + | ||
"</html>"; |
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.
Why have there been \n
characters inserted into the string?
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 can't answer for @jonbartels, obviously, but I often put them in in that way to avoid the source code appearing different from the value of the constant. They are interpreted in HTML as whitespace - which avoids "The default systemcertificate..." vs "The default system certificate...".
Whether that's the reason or not, it seems like this PR moves us to a better spot and doesn't appear to introduce any new problems.
Issue #156 - Change SSL help text to a Constant, remove NextGen branding and ads from the message