TLS error dialog for new setup wizard #9643
Conversation
|
|
||
| _ui->textBrowser->setHtml(errorStrings.join("\n")); | ||
|
|
||
| // FIXME: add checkbox for second confirmation |
There was a problem hiding this comment.
I think we have not decided yet whether to put a checkbox into the dialog or not to require the user to click twice in order to permanently trust a custom certificate.
There was a problem hiding this comment.
you are right, please create an issue to discuss the topic
src/gui/tlserrordialog.cpp
Outdated
| QString TlsErrorDialog::describeCertificateHtml(const QSslCertificate &certificate) | ||
| { | ||
| QString msg; | ||
| msg += QStringLiteral("<div id=\"cert\">"); |
There was a problem hiding this comment.
Don't append multiple times, with msg +=;
This breaks the benefits of the used stringbuilder.
There was a problem hiding this comment.
This was copied 1:1 from the old dialog. I'd rather not touch this at this point, but we could leave a TODO to improve it in the future (when removing the SSL error dialog).
There was a problem hiding this comment.
Best time to refactor code is when you touch it.
|
Can the old one be removed then? |
|
It's still used in the account manager. I'd replace it there in a future PR. |
| // therefore we clear the certificates storage before resolving the URL | ||
| _accountBuilder = {}; | ||
|
|
||
| delete _accessManager; |
There was a problem hiding this comment.
->deleteLater();
There might still be some objects using the old nam.
|
|
||
| _ui->textBrowser->setHtml(errorStrings.join("\n")); | ||
|
|
||
| // FIXME: add checkbox for second confirmation |
There was a problem hiding this comment.
you are right, please create an issue to discuss the topic
src/gui/tlserrordialog.cpp
Outdated
| QString TlsErrorDialog::describeCertificateHtml(const QSslCertificate &certificate) | ||
| { | ||
| QString msg; | ||
| msg += QStringLiteral("<div id=\"cert\">"); |
There was a problem hiding this comment.
Best time to refactor code is when you touch it.
fmoc
force-pushed
the
work/tlserrordialog
branch
2 times, most recently
from
676ce17 to
4329763
Compare
Otherwise, errors may slip through and yield false positives, causing the wizard to assume basic authentication even if the server actually requires OAuth2. This prevents synchronization from working until the user manually logs out and back in again.
|
Kudos, SonarCloud Quality Gate passed! |
Depends on #9642.
This PR implements a new TLS error dialog similar to the old SSL error dialog (which had a dependency on an account object and didn't implement separation of concerns)
There are some TODOs/FIXMEs left, please leave your opinion there.
Contributes to #9249.