Implement XOAuth2 Refresh Token logic

[leostera]

Hi there, I was tinkering around Switchboard when I realized that sending refresh_token information wouldn't work. After some tracing I saw that there actually was no request being made to refresh the token (even though a refresh url was being passed, and the app.config and Vagrantfile had fields for google's app id and secret).

So I forked off, assuming that this refreshing was to be done by a third party server, and implemented it in this branch.

I see now that there's a dedicated cowboy app to handle the social auth, but I fail to see how I can pass both the access_token and the refresh_token and let this social endpoints take care of the refreshing whenever necessary – correct me if I'm wrong, but the ConnSpec takes for token only one token, right?

I'm ready to clean this code up, squash it, and resubmit if necessary. Just need a greenlight on my thought process.

Kudos for the great work to @jtmoulia and the rest of the contributors.

[leostera]

It just occurred to me to take a look at your branches @jtmoulia, and I found this: https://github.com/jtmoulia/switchboard/commit/12ff4ccd043d726318c6e0ecfbe1fa95f784784e

Which seems to better implement what I was attempting.

Is there any reason for some of these branches not to have been merged to the master branch here at thusfresh/switchboard?

I will try it out now.

[jtmoulia]

Hey @ostera,

Bleh, I guess when you send in an email comment github knows no reason. Sorry for this mess of a comment!

You're right -- the connect refresh handling is broken. Thanks for opening the PR!

Alright, I'm going to get into the weeds, but only because I'm happy to have someone else digging into the code :)

ConnSpec only carries the data necessary to set up the transport, hopefully SSL over TCP. Application level IMAP authorization is done using the Auth datatype, which can be either a plain, OAuth access, or OAuth refresh token.

The public switchboard interface exposes switchboard:add/2,3 for creating a new monitored IMAP connection. These functions accept an Auth variable for login.

The way the social app gets the tokens is the google OAuth login flow; it's very well hidden at /auth/google/login if Vagrant is configured with the google client id and secret.

So, there is OAuth via the login flow, but, as you've discovered, it's broken for using refresh tokens with the connect command.

As for jtmoulia/oauth-refresh-fix --

I know I started this, but having the OAuth calls in imap.erl is a bit ugly. I stopped working on this in favor of getting social integrated. The original hope was to use social's functions to call out to imap.erl, but it uses a strange HTTP client -- https://github.com/jtmoulia/social/tree/httpc attempts to swap it out with httpc.

Let me try to fix up that branch, and add a function for exchanging an access for refresh token to cowboy_social_google.

todo:

• improve documentation re /auth/google/login
• fix social httpc branch.
• add access -> refresh function in social

Thanks, @ostera!

[leostera]

@jtmoulia, grand.

I see how it works now, thanks for taking the time to answer. If you need any help with https://github.com/jtmoulia/social/tree/httpc let me know, I wouldn't mind diving into it to get the refresh tokens working here on Switchboard.

Just a question, given that oftentimes there already is an access_token that can be used alongside a refresh_token, what was the train of thought behind not allowing an Auth datatype to have both an AccessToken to be used, and a RefreshToken to fallback to? Just a thought, so that you don't have to request another AccessToken when registering a new monitored IMAP connection.

Again, thanks @jtmoulia and if I can be of any help please let me know!

[jtmoulia]

Hey @ostera,

I see how it works now, thanks for taking the time to answer. If you need any help with https://github.com/jtmoulia/social/tree/httpc let me know, I wouldn't mind diving into it to get the refresh tokens working here on Switchboard.

That branch is a bit of a mess -- let me wrap it up, and then it'd be awesome if you could get it integrated into switchboard as part of this PR.

Just a thought, so that you don't have to request another AccessToken when registering a new monitored IMAP connection.

Good point! Since that's going to require changes all the way down to the documentation I think that's worth a new issue.

Cheers!

[leostera]

it'd be awesome if you could get it integrated into switchboard as part of this PR.

:+1: Perfect, I'll do that. Just hit me up when it's good to go.

[jtmoulia]

Hey @ostera -- I'm going to test this tomorrow (man, testing oauth is a pain), but here are the broad strokes: https://github.com/jtmoulia/switchboard/tree/fix/social-httpc

It's not using the social oauth lib's functions -- there was nothing really there to take advantage of in this case. I moved the oauth related functions out of imap.erl and into switchboard_social_oauth.

[leostera]

@jtmoulia, could you please take a look at these last 2 commits? I found out that getting to the OAuth provider based on the domain of the email address wouldn't work for custom domains.

Say in my case, I have google apps for business running under @ostera.io, and I could set it up as a proxy to https://accounts.google.com/o/oauth2/token so Switchboard could automatically try that based on my email domain and it would work (hypothetically) – but I can't expect everyone else to do do that, be it because they don't know how or are already using those endpoints. Anyway this would be adding too much guess-work to get to the OAuth provider, and it occurred to me that it should be much simpler.

It's far easier to simply specify which provider a certain token belongs to (and I can't come up right now with a case in which this is not known), look it up on the providers list in the config, and get the URL from there. Also less typing, and less data being passed around.

I did however keep the domain_to_provider-related code, but I feel that in order to provide a fallback to any users already using Switchboard as part of their stacks/projects, we should be using Provider as RefreshUrl and looking it up in the config if the first name-based lookup fails. Otherwise just let it crash, right?

Hopefully, as I'm mimicking the style in place, the one I'm using is appropriate.

[leostera]

Okay! So this is in a working state now. Rough, but working. I'm ready to clean it up or re-structure if necessary (shouldn't be hard to add support for both tokens at once as per #44).

A quick checklist might look like this

• [x] Update documentation accordingly
• [x] Make necessary tests for it

Anything else you can think of @jtmoulia ? :smile:

[jtmoulia]

Awesome @ostera -- nice cleanup, and I like the change of allowing the provider to be passed instead of the refresh url.

Re testing, I'm all ears for an easy way to test the oauth flow. I'd rather not mock the calls since that cuts out most of the key parts we'd like to test. Alternatively, we could package an oauth server and test against that, tho it's still hard to do the integration test of oauth -> imap login.

But, I'd say the integration testing is outside the scope of this PR. Within the PR, there is room for unit/property of functions in switchboard_oauth, e.g. to_query_string/1.

And, yup, looks like the doc needs to change url to provider. I can take that unless you want it.

The last thing [but before merging] it'd be great if you could squash your commits to cleanup the fixups. (let me know if this doesn't make sense!)

[leostera]

Going to finish this up tomorrow then. For reference:

• [x] Finish replacing RefreshUrl with Provider in the docs
• [x] Smooth out merge conflicts

[leostera]

Okay! So I couldn't find any more references in the docs, I think we're finished on that front – though better examples are always on demand, I'll keep that in mind and see if I can come up with something that helps other people onboard faster/more-easily.

Regarding the merge conflicts, both are very simple (choosing between 2 lines in .gitignore and the Makefile) so it shouldn't be a problem. I've made sure it's working after the merge. :+1:

@jtmoulia, are we good to go here?

[jtmoulia]

Awesome, I'll get this in today. There are some issues with the new version of erlang.mk, but I'll push to thusfresh/master as soon as I get it fixed. Code up on jtmoulia/master