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.

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

Fix SSLHandshake Exception by using bypass #7657

Merged
merged 6 commits into from
Apr 22, 2021

Conversation

ruanych
Copy link
Contributor

@ruanych ruanych commented Apr 21, 2021

Fixes #7616

  • Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
  • Tests created for changes (if applicable)
  • Manually tested changed features in running JabRef (always required)
  • Screenshots added in PR description (for UI changes)
  • Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

File download path: https://www.serbski-institut.de/os/dnlarchiv/Kleine_Reihe_27_web.2217.pdf


A valid authentication path cannot be found from the site where the downloaded file is located, prompting the user with the option of whether to continue the download.
warning


The user chooses to cancel the download.
canceled_tip


The user chooses to download anyway.
download

@ruanych ruanych mentioned this pull request Apr 21, 2021
@Siedlerchr
Copy link
Member

You can ignore the fetcher and the mac test. But please have a look at the Checkstyle test. Either click on details at the failing or you can go to the files changes tab to see the Reviewdog comments

urlDownload.canBeReached();
} catch (kong.unirest.UnirestException ex) {
if (ex.getCause() instanceof javax.net.ssl.SSLHandshakeException) {
if (dialogService.showConfirmationDialogAndWait(Localization.lang("Download file"),
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need to ask the user, or should we just bypass ssl verification? For example, firefox downloads the linked pdf without any prompt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that bypassing SSL verification should let users know.
We can add an option in the preferences to allow users to select all downloads to bypass the verification. The advantage of this feature that the user does not need to click on the trusted site every time. The disadvantage is that the code changes may be a bit big.

But I found that JabRef uses URLDownload.bypassSSLVerification() directly in some places, such as org.jabref.logic.importer.fetcher.IEEE#getURLForQuery, line 253.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's okay to show a dialog here. Should not happen that often.

URLDownload.bypassSSLVerification();
return true;
} else {
dialogService.showInformationDialogAndWait(Localization.lang("Download file"),
Copy link
Member

Choose a reason for hiding this comment

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

I would use notify here, so that the user doesn't need to click "ok" again in the second dialog.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, I agree with this idea!

@ruanych
Copy link
Contributor Author

ruanych commented Apr 21, 2021

The user chooses to cancel the download, use Notify instead of InformationDialog to prompt the user.

canceled_tip2

@Siedlerchr Siedlerchr changed the title Fix for issue 7616 Fix SSLHanedshake Exception by using bypass Apr 21, 2021
@Siedlerchr
Copy link
Member

Just a minor improvement for the variable naming, otherwise lgtm!

@Siedlerchr Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 21, 2021
@Siedlerchr Siedlerchr changed the title Fix SSLHanedshake Exception by using bypass Fix SSLHandshake Exception by using bypass Apr 21, 2021
…ownload.java


Update the variable naming in src/main/java/org/jabref/logic/net/URLDownload.java, function setSSLVerification().

Co-authored-by: Christoph <siedlerkiller@gmail.com>
@ruanych
Copy link
Contributor Author

ruanych commented Apr 22, 2021

@Siedlerchr @tobiasdiez Thank you so much for your suggestions and help!

Fix the bug of the variable name change.
@Siedlerchr Siedlerchr merged commit 2af578f into JabRef:main Apr 22, 2021
@Siedlerchr
Copy link
Member

We have to thiank you for your contribution @ryyyc !

Siedlerchr added a commit that referenced this pull request Apr 24, 2021
…om.tngtech.archunit-archunit-junit5-api-0.18.0

* upstream/main:
  Fix exception when searching (#7659)
  Fixes #7660 (#7663)
  Fix for issue 5850: Journal abbreviations in UTF-8 not recognized (#7639)
  Fix SSLHandshake Exception by using bypass (#7657)
  Fix for issue 7633: Unable to download arXiv pdfs if Title contains curly brackets (#7652)
  Fix#7195 partly Opacity of disabled icon-buttons
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSLHandshakeException
3 participants