Skip to content

Return Drip's unique user ID as the UID, instead of the client id. #5

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.

Sign up for GitHub

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

Open
ryenski wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Return Drip's unique user ID as the UID, instead of the client id. #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
351c0cf
Merge remote-tracking branch 'DripEmail/master'
Mar 29, 2017
160556f
Return Drip's unique user ID as the UID, instead of the client id.
Mar 29, 2017
8f8b38e
Fixed `OAuth2::Error, invalid_grant` with OmniAuth::Oauth2 v1.4.0
Mar 29, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion lib/omniauth/strategies/drip.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class Drip < OmniAuth::Strategies::OAuth2
:token_url => 'https://www.getdrip.com/oauth/token'
}

uid { access_token.client.id }
uid { raw_info['accounts'][0]['id'] }
Copy link
Contributor

@derrickreimer derrickreimer Mar 29, 2017

Choose a reason for hiding this comment

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

I think this should actually be a unique identifier corresponding to the user. For Drip, the user's email address is this unique identifier.

uid { user_info["users"][0]["email"] }

For example, it's possible that a user does not actually have access to any accounts.

Copy link
Author

@ryenski ryenski Mar 29, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Gotcha... but isn't it the account that we're authorizing? For example, I login to Drip using one email address, but I have two Drip accounts. If the user email is used as the identifier, how would we know which Drip account it's linking to?

This is what it looks like when I go to the authorize url:
contentupgrade_me

If I authorize using my email address, and select the first Drip account, my user_info looks like this:

(byebug) user_info["users"]
[{"email"=>"ryan@•••••••.com", "name"=>"Ryan Heneise", "time_zone"=>"America/Chicago"}]

... and the account info looks like this:

(byebug) raw_info['accounts']
[{"id"=>"xxxx222", "href"=>"https://api.getdrip.com/v2/accounts/xxxx222", "name"=>"mysmallidea.com", "url"=>"mysmallidea.com" ...}]

Now if I want to connect the second Drip account (under the same user login), my user info looks the same:

(byebug) user_info["users"]
[{"email"=>"ryan@•••••••.com", "name"=>"Ryan Heneise", "time_zone"=>"America/Chicago"}]

... but the account info has the unique ID that I can use to identify the account:

(byebug) raw_info['accounts']
[{"id"=>"xxx337", "href"=>"https://api.getdrip.com/v2/accounts/xxxx337", "name"=>"memberman.com", "url"=>"www.memberman.com", "default_from_name"=>"Ryan Crispin Heneise", "default_from_email"=>"hello@•••••••.com" ...}]

If we use the user's email address, rather than the account ID, it would be impossible for my app to tell these two Drip accounts apart. In this case wouldn't it make more sense to use the account ID rather than the user email as the UID?

Copy link
Contributor

@derrickreimer derrickreimer Mar 29, 2017

Choose a reason for hiding this comment

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

Yeah, I see what you mean. I need to give this some thought. The complication is that most tokens issued these days are indeed always tied to one (and only one) active account, but there are some edge cases.

In the meantime, I would recommend making a call to the /accounts endpoint right after obtaining the token to figure out which account the token is authorized to access. This is what most integrators do, which is probably why this issue has not be raised to date.


info do
{
Expand All @@ -38,6 +38,10 @@ def user_info
def raw_info
@raw_info ||= JSON.parse(access_token.get("/v2/accounts").body)
end

def callback_url
options[:redirect_uri] || (full_host + script_name + callback_path)
end
Copy link
Contributor

@pjg pjg Jun 26, 2017

Choose a reason for hiding this comment

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

I don't know about the other change, but without this change this gem is essentially broken on omniauth-oauth2 v1.4+

end
end
end
Expand Down
9 changes: 9 additions & 0 deletions spec/omniauth/strategies/drip_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,13 @@
end
end

context 'uid' do
Copy link
Author

@ryenski ryenski Mar 29, 2017

Choose a reason for hiding this comment

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

This spec came from https://github.com/calebjclark/omniauth-constantcontact/blob/master/spec/omniauth/strategies/constantcontact_spec.rb#L30

before :each do
subject.stub(:raw_info) { { 'accounts' => [{ 'id' => '123' }] } }
end

it 'returns the id from raw_info' do
subject.uid.should eq('123')
end
end
end