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

omniauth-ebay-oauth implementation #2

Merged
merged 21 commits into from Nov 13, 2017

Conversation

Projects
None yet
3 participants
@ignat-z
Contributor

ignat-z commented Nov 4, 2017

No description provided.

let(:failure_result) { File.read('spec/fixtures/result_failure.xml') }
let(:success_result) { File.read('spec/fixtures/result_success.xml') }
before { stub_const("#{described_class}::USER_REQUEST", body) }

This comment has been minimized.

@palkan

palkan Nov 5, 2017

Member

Why do we have to stub this const and not use it as body?

This comment has been minimized.

@ignat-z

ignat-z Nov 5, 2017

Contributor

The main reason -- it's easier to read when something went wrong, compare:

       You can stub this request with the following snippet:

       stub_request(:post, "https://api.com/endpoint").
         with(:body => "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n<GetUserRequest xmlns=\"urn:ebay:apis:eBLBaseComponents\">\n  <ErrorLanguage>en_US</ErrorLanguage>\n  <WarningLevel>High</WarningLevel>\n  <DetailLevel>ReturnAll</DetailLevel>\n</GetUserRequest>\n",
              :headers => {'Accept'=>'*/*', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'Content-Type'=>'text/xml', 'Host'=>'api.com', 'User-Agent'=>'Ruby', 'X-Ebay-Api-Call-Name'=>'GetUser', 'X-Ebay-Api-Compatibility-Level'=>'967', 'X-Ebay-Api-Iaf-Token'=>'token+1', 'X-Ebay-Api-Siteid'=>'0'}).
         to_return(:status => 200, :body => "", :headers => {})

       registered request stubs:

       stub_request(:post, "https://api.com/endpoint").
         with(:body => "<?xml version=\"1.0\" encoding=\"utf-8\"?>\n<GetUserRequest xmlns=\"urn:ebay:apis:eBLBaseComponents\">\n  <ErrorLanguage>en_US</ErrorLanguage>\n  <WarningLevel>High</WarningLevel>\n  <DetailLevel>ReturnAll</DetailLevel>\n</GetUserRequest>\n",
              :headers => {'Accept'=>'*/*', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'Content-Type'=>'text/xml', 'Host'=>'api.com', 'User-Agent'=>'Ruby', 'X-Ebay-Api-Call-Name'=>'GetUser', 'X-Ebay-Api-Compatibility-Level'=>'967', 'X-Ebay-Api-Iaf-Token'=>'token', 'X-Ebay-Api-Siteid'=>'0'})

vs

       You can stub this request with the following snippet:

       stub_request(:post, "https://api.com/endpoint").
         with(:body => "<>",
              :headers => {'Accept'=>'*/*', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'Content-Type'=>'text/xml', 'Host'=>'api.com', 'User-Agent'=>'Ruby', 'X-Ebay-Api-Call-Name'=>'GetUser', 'X-Ebay-Api-Compatibility-Level'=>'967', 'X-Ebay-Api-Iaf-Token'=>'token+1', 'X-Ebay-Api-Siteid'=>'0'}).
         to_return(:status => 200, :body => "", :headers => {})

       registered request stubs:

       stub_request(:post, "https://api.com/endpoint").
         with(:body => "<>",
              :headers => {'Accept'=>'*/*', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'Content-Type'=>'text/xml', 'Host'=>'api.com', 'User-Agent'=>'Ruby', 'X-Ebay-Api-Call-Name'=>'GetUser', 'X-Ebay-Api-Compatibility-Level'=>'967', 'X-Ebay-Api-Iaf-Token'=>'token', 'X-Ebay-Api-Siteid'=>'0'})

Seems like it will be nice even stub default headers, to reduce all this mass. Furthermore, we don't depend on this constants as code control structure, so we could stub it for free.

BTW, changed from stub_const to DI.

@Envek

Thank you for quick and clean implementation!

Some things that should be fixed for use in real apps:

  1. eBay username. It's unreliable to use it as user identifier as it may be changed by the user.

    See the code comment for details.

  2. In real apps, it is important to know when refresh_token will expire to send a reminder to the user, asking to log in again. Also, it allows distinguishing between token expiration and token revocation by the user.

    Please add refresh_token_expires_at field into credentials hash.

  3. OAuth scopes. Empty scope list is fine for sign-in purposes, but it should be documented better IMHO.

    Also, an example should be added (should user concatenate the list of scopes with space or can we do it for them?)

# https://github.com/omniauth/omniauth/wiki/Auth-Hash-Schema
class UserInfo
MAPPING = {
uid: %w[GetUserResponse User UserID],

This comment has been minimized.

@Envek

Envek Nov 6, 2017

Member

Unfortunately, eBay username is not reliable value as the user can change it and can change it frequently.

So it's better to use scary EIASToken value because it doesn't change. Username is still important though, so it's better to keep it in nickname field of info hash as per https://github.com/omniauth/omniauth/wiki/Auth-Hash-Schema#schema-10-and-later

This comment has been minimized.

@ignat-z

ignat-z Nov 7, 2017

Contributor

Fixed, UserID moved to nickname

private
def ensure_success_code
lambda { |response|

This comment has been minimized.

@Envek

Envek Nov 6, 2017

Member

Style note: you can get rid of lambdas and use just methods by using method method:

def call
  MultiXml.parse(…).tap(&method(:ensure_success_result))
end

def ensure_success_code(response)
 …
end

This comment has been minimized.

@ignat-z

ignat-z Nov 7, 2017

Contributor

A matter of taste, but changed to this way

Net::HTTP.new(@url.host, @url.port).tap do |http|
http.read_timeout = @read_timeout
http.use_ssl = true
http.verify_mode = OpenSSL::SSL::VERIFY_NONE

This comment has been minimized.

@Envek

Envek Nov 6, 2017

Member

Please don't do that! This is a security hole!

You may wish to enable bypassing SSL certificate check via an option, but that option must be disabled by default.

This comment has been minimized.

@ignat-z

ignat-z Nov 7, 2017

Contributor

Yep, agree, shame on me, fixed

README.md Outdated
Additional options:
- __sandbox__ - Are you running your application in [sandbox mode](<https://developer.ebay.com/api-docs/static/sandbox-landing.html>), default __`true`__.
- __scope__ - A list of [OAuth scopes](<https://developer.ebay.com/api-docs/static/oauth-details.html#scopes>) that provide access to the interfaces you call.

This comment has been minimized.

@Envek

Envek Nov 6, 2017

Member

Maybe it worth to note that by default that list of scopes is empty and users should provide it if they will use eBay APIs. Or perhaps it will be good to include public access scope https://api.ebay.com/oauth/api_scope by default.

This comment has been minimized.

@ignat-z

ignat-z Nov 7, 2017

Contributor

Not sure about default value, it's omniauth strategy and eBay returns enough information for simple SSO. Add ability to pass scope as array of scopes or string.

@Envek

Envek approved these changes Nov 7, 2017

Fix fragile test with timing
Spec
    Start     Time.now       Expiration time
      |          |___expiration___|
      v          v----------------v
|-----------------------------------| t
       ^       ^----------------^
       |       |___expiration___|
     Start  Time.now       Expiration time
Code

paderinandrey added a commit to paderinandrey/omniauth-ebay-oauth that referenced this pull request Nov 9, 2017

@Envek Envek merged commit 4fd58bf into evilmartians:master Nov 13, 2017

1 check passed

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

This comment has been minimized.

Member

Envek commented Nov 13, 2017

Released as v0.1.0. Congratulations and thank you for your work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment