Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use new TLSErrorDialog #9667

Merged
merged 7 commits into from
May 13, 2022
Merged

Use new TLSErrorDialog #9667

merged 7 commits into from
May 13, 2022

Conversation

TheOneRing
Copy link
Member

@TheOneRing TheOneRing commented May 11, 2022 •
edited
Loading

No description provided.

@TheOneRing TheOneRing linked an issue May 11, 2022 that may be closed by this pull request
Remove old SSL error handler dialog #9655
Closed
@TheOneRing TheOneRing force-pushed the work/9655-remove-old-ssl-error-handler-dialog branch from 5919704 to 64a9eb0 Compare May 11, 2022 15:04
@TheOneRing TheOneRing changed the title Work/9655 remove old ssl error handler dialog Use new TLSErrorDialog May 11, 2022
@TheOneRing TheOneRing requested a review from fmoc May 11, 2022 15:05
@owncloud owncloud deleted a comment from CLAassistant May 11, 2022
@ownclouders
Copy link
Contributor

ownclouders commented May 11, 2022 •
edited
Loading

Results for GUI-tests https://drone.owncloud.com/owncloud/client/11749/6/1

💥 The GUI tests failed.

GUI Logs: https://cache.owncloud.com/public/owncloud/client/11749/guiReportUpload/index.html

Server Logs: https://cache.owncloud.com/public/owncloud/client/11749/guiReportUpload/serverlog.log

Screenshots:

@TheOneRing TheOneRing force-pushed the work/9655-remove-old-ssl-error-handler-dialog branch 4 times, most recently from e9aa602 to 000ff7f Compare May 11, 2022 15:52
@owncloud owncloud deleted a comment from CLAassistant May 11, 2022
@TheOneRing TheOneRing force-pushed the work/9655-remove-old-ssl-error-handler-dialog branch from 000ff7f to a06fd8e Compare May 11, 2022 17:00
fmoc
fmoc reviewed May 11, 2022
View reviewed changes
changelog/unreleased/9655 Outdated Show resolved Hide resolved
src/gui/connectionvalidator.cpp Outdated
@@ -98,6 +99,8 @@ void ConnectionValidator::systemProxyLookupDone(const QNetworkProxy &proxy)
// The actual check
void ConnectionValidator::slotCheckServerAndAuth()
{
// ensure we receive ssl errors
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how to read that comment. Is that a referral to clearing the connection cache by resetting the access manager?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes its about the qnam reset.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is very relevant in this context. Mind to adjust the comment to include this really important bit of information?

src/libsync/creds/httpcredentials.cpp Outdated Show resolved Hide resolved
@TheOneRing TheOneRing force-pushed the work/9655-remove-old-ssl-error-handler-dialog branch 2 times, most recently from 1051769 to e018fde Compare May 12, 2022 13:50
@sonarcloud
Copy link

sonarcloud bot commented May 12, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 7 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@fmoc fmoc force-pushed the work/9655-remove-old-ssl-error-handler-dialog branch from e018fde to f88a7fb Compare May 13, 2022 06:54
@TheOneRing TheOneRing added this to the 3.0 milestone May 13, 2022
@TheOneRing TheOneRing force-pushed the work/9655-remove-old-ssl-error-handler-dialog branch from f88a7fb to 8392d1d Compare May 13, 2022 08:54
TheOneRing and others added 2 commits May 13, 2022 10:58
@TheOneRing
Allow to use the ConnectionValidator without checking the auth
bb0c432 
That way we can use it to check for ssl errors
@TheOneRing
Make the CheckServer job independent of the account
01800b6 
The job needs to run on an independent QNAM
@TheOneRing TheOneRing force-pushed the work/9655-remove-old-ssl-error-handler-dialog branch 2 times, most recently from 34cbfd2 to 075e0f0 Compare May 13, 2022 09:34
fmoc
fmoc reviewed May 13, 2022
View reviewed changes
src/gui/connectionvalidator.cpp
// Note: Why would this happen on a status.php request?
_errors.append(tr("Authentication error: Either username or password are wrong."));
} else {
//_errors.append(tr("Unable to connect to %1").arg(_account->url().toString()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see that one

src/gui/connectionvalidator.cpp
@@ -223,18 +245,18 @@ void ConnectionValidator::slotAuthFailed(QNetworkReply *reply)
void ConnectionValidator::slotAuthSuccess()
{
_errors.clear();
if (!_updateConfig) {
reportResult(Connected);
if (_mode != ConnectionValidator::ValidationMode::ValidateAuth) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe Q_ASSERT you're not in ValidateServer? I assume the use of != rather than > implies this.

src/libsync/account.h
AccessManager *accessManager();
QSharedPointer<AccessManager> sharedAccessManager();
[[deprecated]] QSharedPointer<AccessManager> sharedAccessManager();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Long time overdue.

src/libsync/creds/httpcredentials.cpp Outdated Show resolved Hide resolved
@TheOneRing TheOneRing force-pushed the work/9655-remove-old-ssl-error-handler-dialog branch from 5bcb6f4 to b0b513d Compare May 13, 2022 11:07
@TheOneRing TheOneRing merged commit e8064c0 into master May 13, 2022
@delete-merged-branch delete-merged-branch bot deleted the work/9655-remove-old-ssl-error-handler-dialog branch May 13, 2022 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmoc fmoc fmoc left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.0
Development

Successfully merging this pull request may close these issues.

Remove old SSL error handler dialog
3 participants
@TheOneRing @ownclouders @fmoc