-
Notifications
You must be signed in to change notification settings - Fork 32
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
Update QEverCloud and use system browser for OAuth (with ability to also use QtWebEngine or QtWebKit) #197
Conversation
Oh and I forgot to mention, to the best of my knowledge QtWebEngine cannot be used along with MinGW on Windows, it supports only MSVC. So I guess if the switch to QtWebEngine would be unconditional, it would break the way to build Nixnote2 with MinGW on Windows. Perhaps the dependency on QtWebEngine could thus be made optional so that on Windows there would be a way to continue using QtWebKit only, perhaps using a maintained fork of it instead of the official version which is known to not quite work for OAuth anymore. |
And one other notable thing, I switched the C++ standard from C++11 to C++14 along with updating QEverCloud because QEverCloud requires C++14 standard. C++14 is 9 years old by now so I guess it's ok to update it. |
Did some more tinkering here:
I also tried to switch docker build to QtWebEngine but I get some problems there which I have already encountered with Quentier: there are some libraries which, when included into AppImage, lead to crashes, in particular libnss3.so and libnssutil3.so. They seem to depend on some other system specific stuff which is not bundled into the AppImage. In case of Quentier excluding them from the AppImage worked but in case of Nixnote2 I see some SSL related errors in the log and OAuth still doesn't work. Will see if I can sort it out. |
if you can solve the AppImage problem, this would be great |
Welp, I discovered that Quentier AppImage doesn't work on my machine as well. I tried to install OpenSSL 1.1 manually on my system, it didn't help. I also tried to bundle these libs manually into the AppImage, it didn't help as well. Probably the only viable option is to provide two AppImages, one for systems older than Ubuntu 22.04 and one for newer systems... |
If you are talking about AppImage, can you please include dark QT theme inside? |
I don't know the details of what needs to be bundled into the AppImage to support themes. And there are more important problems with the AppImage as I described above than some missing themes - it won't work on Ubuntu 22.04 and beyond if this PR is merged. |
My vote is to only support newer systems, I respect your time to support this stuff. |
Hi, d1vanov, What you want to achieve is to login successfully, right? If so, that will not be a big problem, so will this issue(#171). When you type in username and password in the login page, click login button, blank page appears as issue 171 said. At the moment, you could just wait for seconds and then close it, nixnote will pop up a message saying that login fails. You could just ignore that, and then click the sync button again, the authorization page will appear normally this time, and then you click authorize button to finish it. I found this issue months ago, but I didn't think it serious. If we want to fix this some confusing issue, we could just help QtWebKit finish the jump-off manually, which will be more economical than using QtWebEngine, I think. And also, replacing QtWebKit with QtWebEngine might bring problems such as: So, in summary, I think, using QtWebEngine is not a good bargain. |
Interesting, I haven't tried to try authentication twice. And in my case the page wasn't blank at the start, it showed me the field to enter my login but when I entered "continue" a new field for password should have appeared but it didn't. With QtWebEngine it worked properly. I still think it would be useful to offer at least an option to use QtWebEngine for OAuth because I'm not surprised that unmaintained QtWebKit has problems with it - modern web pages use modern JS frameworks and whatnot while QtWebKit has been stale for years. And you misunderstood, this PR won't replace QtWebKit with QtWebEngine for the note editor but only for OAuth page. So the only problem would be just having a dependency on it. |
I met this sometmes, I fixed it with another two methods:
|
It would be interesting to see this code, maybe it would be a simpler solution to this problem. |
I remember that it was just serveral lines. I'll add it to this repo later. |
Most Linux distributions does not have QtWebKit anymore in their repos, so we cannot use it as a dependency. |
Thanks for your info. I didn't focus on QtWebKit dev too much ever before. This is an imporant point I neglected. |
I looked into the code again, and realized that if we want to show the password form programatically, we have to write some application level code within QEverCloud api, which is not a reasonable thing. Concerning to the fact that QtWebKit is dropped by most distros as vitaly-zdanevich said, using QtWebEngine to replace QtWebKit is probably more reasonable in the long term. |
I also think, for long term, replacing of QtWebKit by QtWebEngine is sure preferable, just it was not in my time budget to do it other option which I tried (back in 2018) was port of WebKit - see e.g. https://github.com/movableink/webkit |
Could you please demonstrate how this could be done nevertheless? As the maintainer of QEverCloud I'm interested in getting it's OAuth to work with QtWebKit as well as QtWebEngine even if that would involve some hacky code. Maybe it won't be too hacky I could hide it inside QEverCloud internals. |
I also had a thought of another possible way to do Evernote OAuth - delegate the display of Evernote's login page to the system browser and thus not depend on either QtWebKit or QtWebEngine for OAuth. I'm not yet sure but it should be possible - if we start a HTTP server on some port on localhost, we could pass the address of this localhost server as the "callback url" which is one of the parameters with which Evernote's OAuth page is opened. After the user authorizes the app, the system browser should make a request to this callback url with the token for the app as one of URL parameters. With QtWebKit or QtWebEngine backends we just intercept this callback request and extract the token from it's url parameters. In case of local server we would also be able to extract the token from URL parameters. However, we'd also need to respond with some HTML which would say something like "Authentication is complete, you can now close this page and return to the app". |
I'll try to do an experiment and see whether this approach is feasible. |
OK. I just share the idea, but I don't recommned anyone to implement it. As I cannot reproduce this issue now, I suspect that it was caused by network condition in my case. And QWebEngine is almost the choice in the future, so I think when we finish the migration from QWebKit to QWebEngine, QEverCloud can use it together with the editor. The idea is simple, just find the element of password when the login page finishes loading, delete its 'invisible' attribute. A web frontend operation. It can be done with QWebView api. |
If we choose only QWebKit, we may have troubles in distribution, maybe we have to include the webkit code in the nixnote repo, and tolerate security issues, though its possibility is low. |
Good news - I was finally able to successfully authenticate to Evernote via QEverCloud and system browser instead of either QtWebKit or QtWebEngine. However, I ended up with quite messy code for the prototype and would like to refactor it somewhat before I update this PR. But I intend to do it some day. |
@d1vanov have you succeeded in creating an appimage for 22.04? When i tried doing that, the resulting appimage (created on 22.04 for 22.04) did not run.. |
I was able to create AppImage on 22.04 which did run on 22.04. However, I wasn't able to create an AppImage on 20.04 which would run on 22.04 due to problems with QtWebEngine and OpenSSL. When I'm finished with this PR, there shouldn't be a dependency on QtWebEngine and thus the AppImage problem should also be solved. |
thanks for the feedback! if you did use linuxdeployqt, may i ask what options got it working for you? if not, may i ask how you generated the 22.04 on 22.04 appimage? were you still including QtWebEngine at that point? thanks |
linuxdeployqt only recently started to work on 20.04 and doesn't work on 22.04 so I didn't use it. Instead I used linuxdeploy, linuxdeploy-plugin-qt and appimagetool. Roughly as follows:
The first I did include my changes with switching OAuth to QtWebEngine but only because I coudn't get the OAuth via QtWebKit to work properly. It seems that for some people it strangely works though so you might not need the QWebEngine related changes to authenticate the app with Evernote. |
… it across the codebase
…t OAuth via QtWebKit or QtWebEngine as config options
…em browser for OAuth
7f3cbd8
to
03837e6
Compare
Updated the PR - now it brings in QEverCloud 6.2.0 which can be built with different "backends" for OAuth - one using system browser, another one using QtWebEngine and another one using QtWebKit. I brought these configuration options to Nixnote2 as well and made OAuth via system browser the default option as it seems to work surprisingly well. Checked that with all three available options I can compile Nixnote2. QtWebKit backend doesn't work quite well but we already know that. I've also updated the script to build things with docker to use Ubuntu 20.04 image and checked that the AppImage generated by it works on Ubuntu 22.04. |
hi @d1vanov, the PR is from your side now ready to merge? |
Well, yes, I believe it is ready, if not for immediate merge then for review at least ) |
Hi @d1vanov, I found a minor problem. After initial sync with authorization via system browser, an error message is displayed, saying that the sync failed (but actually it worked fine and authorization is successful). After clicking OK on the error message, all seems to be OK and further sync, work. Can you maybe check that? A further really minor problem is: if the updated "docker build" is called with a branch name which is like "abc", then the git checkout fails. (If the branch name is like "feature/abc", then it works). Further, we need to update the ".travis.yml" to get the Travis build in sync with Anyway, it is a big improvement and I will merge the changes soon. |
Hmm, I think I caught this error during my tests but just once. I'll see if I can reproduce and debug it. |
Found a couple of things about this sync error:
|
Ok, I figured it out: wrong URL was used to initialize |
Pushed fix for that sync issue + attempt to update Travis CI configuration. It rarely goes smooth from the first attempt so I might need to do some back and forth steps to make it right... |
5219a7b
to
a68e2b9
Compare
a68e2b9
to
1a75e6c
Compare
Ok, I got Travis CI build fixed. |
OK great! I'll look at it later. Thanks. |
thanks! |
See my comment here for more details.
This PR updates the copy of QEverCloud within nixnote2's source tree to latest 6.1.0 release. The update includes adaptation of the entire codebase in places where QEverCloud API is called. The adaptation was quite straightforward so I shoudn't have broken anything. I was able to login to my account and sync it using nixnote2 built from this branch so I guess it counts for some testing.
One more thing that I did put into this PR as a separate commit is a change of default authentication token for public linked notebooks - I have a public notebook linked into my account and I had an error during sync until I replaced the token. It's kinda strange actually, the last time I checked Evernote's sync documentation it said public linked notebooks should not require auth token at all but my experience shows it actually does require it.
Why this PR is incomplete - because in order to fix OAuth I had to add a new dependency - on webengine and webenginewidgets Qt modules. I updated the
nixnote2.pro
file so it can be built but there are multiple build and package scripts in this repo - these I haven't updated and it's unlikely that I will. Moreover, there are downstream maintainers for Nixnote2 for various distros, they all would have to update their packaging scripts as well if this gets merged.So I encourage anyone interested to continue my work and update various build and package scripts to include the new dependency. The completed version could then be merged.