coderly / ember-gdrive

Google Drive bindings for Ember Data
MIT License
18 stars 0 forks source link

Refactored login hint process #131

Closed begedin closed 9 years ago

begedin commented 9 years ago

Hopefully resolves #130

Regarding the createFromState and openFromState methods, they are already implemented and seem to be working fine. The thing that did seem to change are the parameters we need to send through the URL. I updated the design_notes.txt (replaced wth an .md) with an explanation how to form the URL exactly.

Regarding extraction of cacheLoginHint into a library, I went a step further and also extracted the methods required for login hint retrieval (used in the authenticator) into the same library. This way, we keep that logic in one place.

venkatd commented 9 years ago

A few points:

As a reminder, track these hours separately :)

begedin commented 9 years ago

@venkatd: I wasted some time trying to move it all into the document-source library, but eventually, I figured out a simple solution for both of the issues you described.

By moving the login hint caching from the model hook into afterMode, it is now called for any case, regardless of how the document was opened or created. While it was being called from model, it would only work with cases where there's a document id in the parameter list, meaning only direct load access through regular routing, since that's the only time the model hook get's executed.

For open/createFromState, we fetch or create the document first and then pass it as a model directly to the route, so the model hook is skipped. afterModel is never skipped, though, so that's where the logic really needed to be in the first place.

As for authorization, I simply moved the fetching of the login hint (and the GOOGLE_CLIENT_ID config property) directly into the auth library, since it always needs to be called so it makes more sense for it to be there than in the authenticator.

Since opening from google drive is functionally equal to opening from state, this change applies to that as well and there's an attempt to determine the login hint every time.

venkatd commented 9 years ago

@begedin we would still need to handle figuring out the login hint from the state property when calling openFromState and createFromState. This is because we may not have any login_hint cached but the login_hint will be provided in the state (via the userId property).

If we don't provide a login_hint in this situation then the user will unnecessarily be prompted with an auth dialog to pick which account--even though we already know which account the are accessing the document from.

begedin commented 9 years ago

@venkatd You're right. I completely missed that part. The simplest way to do it is to send in a login hint from the authenticate call in the application-route-mixin, where the document is first opened or created from state. We already have the state object there, so it's a simple matter if retrieving the userID from that and sending it as a login_hint.

That way, the session is still the thing that handles our authentication in all cases.

I also realized there was some code duplication in the login hint library, so I moved some stuff around to eliminate it. Basically, login_hint had an identical method to one being exported from the uri library. Additionally, another method getDocumentIdFromLocation is also something that fits with the uri library more than it does with login_hint, so I moved it and am now importing it from there.