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

PICARD-615: OAuth authentication #305

Merged
merged 10 commits into from Dec 3, 2014

Conversation

Projects
None yet
4 participants
@lalinsky
Copy link
Member

commented Apr 25, 2014

Motivated with a chat with user on IRC, who mentioned the plain text passwords in Windows registry, which is something that I should have fixed a long time ago.

This is not yet finished from UI perspective, but it's fully functional.

I'd like to get some feedback on would people expect the authorization process work. I'm not really good at UI work, so I made a very crude version first. It just opens the browser and a minimal popup that expects that authorization code. There isn't any progress or error reporting.

We could probably also use the HTTP server we have on localhost, but I'm not sure if that's actually better than copy&paste of the authorization code and it's way more work as we have to make the page nice.

Also, if you are not logged in, Picard asks you for username/password but never saves those. Should we disallow the prompt for MB?

scopes = "profile tag rating collection submit_isrc submit_barcode"
authorization_url = self.tagger.xmlws.oauth_manager.get_authorization_url(scopes)
webbrowser2.open(authorization_url)
authorization_code, ok = QInputDialog.getText(self,

This comment has been minimized.

Copy link
@zas

zas Apr 26, 2014

Collaborator

Is there any reason to keep a standalone dialog instead of a text field in options page ? If not, it could be an text input field, hidden until Log In button is pressed, and after successful login (when Log Out is displayed).

We could use localhost server to receive the authorization code (as Tagger is done), and still permit cut'n'paste.

This comment has been minimized.

Copy link
@lalinsky

lalinsky Apr 26, 2014

Author Member

Yep, this is the part I wanted to talk about.

I think it should be a (custom) dialog that tells you that there is a browser window open, that you need to authorize the application there and come back. It would ask you for the authorization code, but we can skip that if we use localhost (but I'm probably not going to do this myself). After you enter the authorization code, it would display some kind of loading info to let you know it's doing something that then it would either show an error or that everything went fine.

The current solution is just the simplest thing I could do with the minimal amount of code.

I don't think this should be a text input in the options page, because there is some state hidden behind the input. I think it would be better to have something to guide you though the authorization process.

This comment has been minimized.

Copy link
@lalinsky

lalinsky Apr 26, 2014

Author Member

One more thing, I think the same dialog should pop up when we receive a 401 error (with bearer authorization request) from MB. The password dialog would be only used for non-MB sites. But this is something I have been reluctant to do before some XmlWebService refactoring.

This comment has been minimized.

Copy link
@zas

zas Apr 26, 2014

Collaborator

The current solution is just the simplest thing I could do with the minimal amount of code.

Yes, and it works ;)

I don't think this should be a text input in the options page, because there is some state hidden behind the input. I think it would be better to have something to guide you though the authorization process.

Then perhaps pressing Log In button should pop up a dialog, with explanation
about the process, and the authorization code input field.
Imho, it should also (optionally?) display the url sent to the browser in case of Picard fails to open the browser automatically (for manual cut'n'paste).
Can you describe each step ?

The password dialog would be only used for non-MB sites.

In which cases does this happen ?

@zas

This comment has been minimized.

Copy link
Collaborator

commented Apr 26, 2014

This move is needed, and implementation is ok for me (details can be improved later).
I think we need another way than the cut'n'paste even though it is very easy and should'nt happen too often.

@@ -138,6 +138,15 @@ def load_release_type_scores(setting):
_s[opt] = load_release_type_scores(_s.raw_value(opt))


def upgrade_to_v1_3_0_dev_5():
"""Options "username" and "password" are removed

This comment has been minimized.

Copy link
@zas

zas Apr 26, 2014

Collaborator

Perhaps add "OAuth" keyword in this comment ;)

url = QUrl()
if host in MUSICBRAINZ_SERVERS and port == 80:
url.setScheme("https")
url.setHost(config.setting["server_host"])

This comment has been minimized.

Copy link
@zas

zas Apr 26, 2014

Collaborator

You can use host here

else:
url.setScheme("http")
url.setHost(config.setting["server_host"])
url.setHost(config.setting["server_port"])

This comment has been minimized.

Copy link
@zas

zas Apr 26, 2014

Collaborator

You can use host and port instead of config.setting[].

mblogin=False, cacheloadcontrol=None, refresh=None):
def _start_request_continue(self, method, host, port, path, data, handler, xml,
mblogin=False, cacheloadcontrol=None, refresh=None,
access_token=None):

This comment has been minimized.

Copy link
@zas

zas Apr 26, 2014

Collaborator

A lot of parameters on this method now, i wonder if we shouldn't think about shortening it somehow.

This comment has been minimized.

Copy link
@lalinsky

lalinsky Apr 26, 2014

Author Member

I think a general refactoring of this class is in order. There are too many conditions hidden inside the methods.

@zas

This comment has been minimized.

Copy link
Collaborator

commented Apr 26, 2014

Also, if you are not logged in, Picard asks you for username/password but never saves those. Should we disallow the prompt for MB?

Imho, yes, and replace it with a message dialog with how to setup auth.

@Sophist-UK

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2014

I haven't had the time to try this code yet, however...

  1. Am I right in thinking that you only need to authorise Picard to access your account once, and then Picard can then log in automatically?
  2. Is it possible to open the web site via an internal http connection and to solicit the userid and password via a dialog box and then make another call to the website to get authorised i.e. not involve the browser at all? Even if it IS technically possible, is it a good idea?
  3. Is the code designed e.g. to allow plugins to have access to MB data?
  4. Is the code designed to support e.g. plugins accessing other OAuth sites (like Discogs)? (I was thinking of adding a Discogs cover art plugin, but needed to do OAuth and so gave it a miss. But if the Picard OAuth code allows plugins to authenticate to other sites, then I might have another go at this.)
@phw

This comment has been minimized.

Copy link
Member

commented Apr 27, 2014

Nice work, this should definitely be the way the authentication is handled in Picard.

We could probably also use the HTTP server we have on localhost, but I'm not sure if that's actually better than copy&paste of the authorization code and it's way more work as we have to make the page nice.

I would definitely use a localhost callback here. Copying the access token feels strange, as a user I would expect the application to do the work for me. Most desktop apps which use oAuth I have seen so far even use an embedded browser window to make the process look more seemless. But we don't have to go that far.

I also don't think it is that much work. I would reuse most of the look and feel of the currently displayed page (minus all the navigation links), inline all CSS and have it as a static HTML file which gets handed out by Picard's internal server.

@zas

This comment has been minimized.

Copy link
Collaborator

commented May 8, 2014

I think we should merge this PR, as it is functional, UI improvements can come later.

@phw

This comment has been minimized.

Copy link
Member

commented May 8, 2014

I think we should merge this PR, as it is functional, UI improvements can come later.

Agreed

@phw

This comment has been minimized.

Copy link
Member

commented May 10, 2014

I have found two issues with this which IMHO should be resolved before merging:

  1. The collections feature uses the username variable: https://github.com/musicbrainz/picard/blob/master/picard/album.py#L127 . I am not sure how to resolve this, it looks like the collection feature requires the current user's username to find the correct collection. But with oAuth this is not available anymore.
  2. As luks mentioned in his initial comment, the username/password dialog shows up when one is not logged in yet, but obviously this does not save the username anymore. Instead the oAuth process should be triggered.

I would like to see this integrated, but I am not sure if I can find the time to tackle these issues.

@lalinsky

This comment has been minimized.

Copy link
Member Author

commented May 11, 2014

Well, I'm still planning to fix the issues that Philipp mentioned, but I'm not sure when. One big problem I encountered is that I can't set Bearer auth headers when using QAuthenticator, which means that I have no way to handle OAuth in the auth dialog.

One option would be to implement something like GitHub has where you can use OAuth Bearer token with HTTP Basic auth. But this needs a server change:

https://developer.github.com/v3/auth/#via-oauth-tokens

Another option is to simply abort the request and ask the use to log in and do the action again.

I guess I'll close the pull request for now, as I mainly wanted to get some feedback. Of course if somebody wants to take this over, I'll be more than happy. :)

@lalinsky

This comment has been minimized.

Copy link
Member Author

commented Jun 1, 2014

I have updated the branch to fix the collection service, which needs an username. Unfortunately, I can't do much about the password dialog, without either throwing out QNetworkManager or switching to some way of using HTTP Basic Auth with OAuth tokens (which would need to be implemented on the server first). I'm probably also not going to be able to do a better UI. So let's decide whether to merge this or not.

@zas

This comment has been minimized.

Copy link
Collaborator

commented Aug 6, 2014

Any progress on this ?

@lalinsky

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2014

Ok, so I experimented with this a little more and the only thing I can do, without completely replacing QNetworkAccessManager, is to show a message that the user needs to log in, possibly open the options window and then let the request fail. This would happen for 401 errors returned from the currently configured MB server. For other servers we would still have the password dialog. Either we do this, or we keep the password dialog also for MB requests (maybe we some extra message that the user can also log in permanently in the options).

The problem is that with QNetworkAccessManager::authenticationRequired() I need to supply username/password within the signal handler, which means I can't run complete OAuth authentication because that requires multiple requests to happen in between.

@zas zas added the Needs more work label Sep 10, 2014

@zas zas added this to the 1.4 (dev) milestone Sep 10, 2014

@zas

This comment has been minimized.

Copy link
Collaborator

commented Dec 3, 2014

@zas zas changed the title Initial version of OAuth authentication PICARD-615: OAuth authentication Dec 3, 2014

@zas

This comment has been minimized.

Copy link
Collaborator

commented Dec 3, 2014

What about merging this and solve remaining issues afterwards ?

@phw

This comment has been minimized.

Copy link
Member

commented Dec 3, 2014

+1 from me for this approach

lalinsky added some commits Dec 3, 2014

Merge remote-tracking branch 'origin/master' into oauth
Conflicts:
	picard/__init__.py
	picard/const/locales.py
@lalinsky

This comment has been minimized.

Copy link
Member Author

commented Dec 3, 2014

I have merged master in and resolved the conflicts. Do you want me to merge it?

@zas

This comment has been minimized.

Copy link
Collaborator

commented Dec 3, 2014

Yes, please do ;)

lalinsky added a commit that referenced this pull request Dec 3, 2014

Merge pull request #305 from lalinsky/oauth
PICARD-615: OAuth authentication

@lalinsky lalinsky merged commit 9a0d01b into metabrainz:master Dec 3, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@zas zas removed the Needs more work label Jun 30, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.