Implementation Ebay OAuth2 Strategy for OmniAuth #1

paderinandrey · 2017-11-03T16:29:53Z

Hi, please review this implementation and let me know if there is some bugs

Envek

Thank you for your contribution!

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

info hash layout. While it's backward compatible with omniauth-ebay gem (which is a good thing, it is somewhat uncommon for omniauth strategies in general).
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 (before token really expires and our app will stop to work). Also, it allows distinguishing between token expiration and token revocation by the user.

Please add refresh_token_expires_at field into credentials hash.
OAuth scopes. Public scope provided by default is awesome! But it should be documented better IMHO in README.

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

I like that you've used omniauth-oauth2 gem, as it solves some problems for free. Your implementation works.

However, code is a bit messy. Try to clean and refactor it with rubocop, for example.

Envek · 2017-11-06T08:49:38Z

omniauth-ebay-oauth.gemspec

+    spec.metadata["allowed_push_host"] = "TODO: Set to 'http://mygemserver.com'"
+  else
+    raise "RubyGems 2.0 or newer is required to protect against " \
+      "public gem pushes."


We don't need this as it is open source.

Envek · 2017-11-06T08:52:31Z

omniauth-ebay-oauth.gemspec

+
+  spec.add_runtime_dependency 'omniauth', '~> 1.0'
+  spec.add_runtime_dependency 'omniauth-oauth2', '~> 1.0'
+  spec.add_runtime_dependency 'oauth2', '~> 1.3'


Do we need explicit requirement on oauth2 for some reason? omniauth-oauth2 already depends on ~> 1.0 of it.

Envek · 2017-11-06T08:54:42Z

Gemfile

+  gem 'rack-test'
+  gem 'rspec', '~> 3.6.0'
+  gem 'simplecov', require: false
+  gem 'webmock'


It is probably better to move them it into the gemspec's add_development_dependency.

Envek · 2017-11-06T09:56:24Z

lib/omniauth/strategies/ebay.rb

+
+      info do
+        {
+          ebay_id: raw_info['UserID'],


It is 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

Envek · 2017-11-06T10:02:29Z

lib/ebay_api.rb

+	      <ErrorLanguage>en_US</ErrorLanguage>
+	      <WarningLevel>High</WarningLevel>
+      </GetUserRequest>
+      )


It is better to be moved to constant and freezed. Or even to a file.

Envek · 2017-11-06T10:03:21Z

lib/ebay_api.rb

+	      <ErrorLanguage>en_US</ErrorLanguage>
+	      <WarningLevel>High</WarningLevel>
+      </GetUserRequest>
+      )


Also, you should add DetailLevel tag to get user's full name, phone, etc.

<DetailLevel>ReturnAll</DetailLevel>

Envek · 2017-11-06T10:07:20Z

lib/ebay_api.rb

+    headers = ebay_request_headers(call_name, request.length.to_s)
+    url = URI.parse(api_url)
+    req = Net::HTTP::Post.new(url.path, headers)
+    http = Net::HTTP.new(url.host, url.port)


It's better to extract http client configuration out. Rubocop already complaints about this method.

Envek · 2017-11-06T10:13:44Z

lib/omniauth/strategies/ebay.rb

+
+      def raw_info
+        @user_info ||= get_user_info
+        @user_info


This line is useless ✂️

Envek · 2017-11-06T10:14:59Z

lib/omniauth/strategies/ebay.rb

+      end
+
+      def raw_info
+        @user_info ||= get_user_info


Naming is weird. Both for instance variable (it is astonishing that it doesn't match method name) and for get_user_info method.

Envek

EbayAPI module is confusing me. It is a module but it can only be included in the classes that are inherited from OmniAuth::Strategies::OAuth2 class. This is makes tests somewhat easier to write, but blows mind away when someone tries to understand what is what in code. Many people have said “prefer composition over inheritance” mantra many times and it's probably worth to make EbayAPI standalone class with clear dependencies that we must to have to instantiate it and with clear API. So we can explicitly use it. I believe this will make things better.

But anyway these are just complaints about style, mostly. Your PR is working well.

Envek · 2017-11-08T19:51:51Z

lib/omniauth/strategies/ebay.rb

+
+      credentials do
+        {
+          :refresh_token_expires_in => access_token['refresh_token_expires_in'].to_i


oauth2 gem actually converts access token lifetime duration from expires_in field to unix timestamp and exposes it in expires_at field (see OAuth2::AccessToken#initialize) and omniauth-oauth2 uses only this field. I think it is better to do the same with refresh token for the sake of consistency.

Please change it to refresh_token_expires_at (returning Unix timestamp of token expiration).

Envek · 2017-11-08T20:15:51Z

README.md

+
+    ```ruby
+    Rails.application.config.middleware.use OmniAuth::Builder do
+      provider :ebay, ENV['EBAY_KEY'], ENV['EBAY_SECRET'], ENV['EBAY_RUNAME'], scope: "sell.marketing, sell.account.readonly"


Why not just allow to specify an array of scopes?

Envek · 2017-11-08T20:19:16Z

lib/omniauth/strategies/ebay.rb

+
+      extra do
+        {
+          :raw_info => raw_info


Why do you have changed to old hash syntax?

paderinandrey · 2017-11-09T09:22:59Z

I fixed notes. My first gem turned out really cool. Thanks)

Envek · 2017-11-09T12:34:03Z

lib/ebay_api.rb

-    attr_accessor :request, :response
+module OmniAuth
+  module Strategies
+    class EbayAPI


OmniAuth::Strategies is not appropriate namespace, actually. Because this is not strategy itself and OmniAuth should not try to load it. It is better to keep it somewhere in gem namespace. Something like OmniAuth::Ebay::EbayAPI (scary, I know!)

Envek · 2017-11-09T12:45:13Z

Gem namespace is weird. All stuff (except OmniAuth strategy) should be placed somewhere inside OmniAuth::Ebay::OAuth module or something like it. To avoid screwing up with constants from omniauth-ebay gem (not sure if anyone will need to use them simultaneously), or from ebay_api gem (and we are absolutely sure that we will use them together) in one app.

Except that everything is fine. Thank you for patience — resolving reviews is hard.

Envek · 2017-11-13T19:31:18Z

I'm closing this PR in favor of #2. I'm very grateful for your work and patience in resolving issues.

This wasn't an easy decision as all three submitted pull requests were fully functional solutions, differing in some subjective things.

This project is now alive and if you notice that it lacks some useful features, have bugs or needs polishing — pull requests are always welcome!

I learned something about OAuth while reviewing this PR (even after working with this protocol before) and hope that you have learned a lot too.

paderinandrey added 2 commits October 25, 2017 17:21

Added gem skeleton

b5c370f

basic implementation

1ac13ea

Envek requested changes Nov 6, 2017

View reviewed changes

paderinandrey added 7 commits November 7, 2017 11:00

move get user request to file

137be07

extracting http client configuration out

5e28080

modify scope

951e278

fix README

c626de9

fixed evilmartians#1

4047a55

add Travis configuration file

f462ecd

fix travis config

e09fca8

Envek requested changes Nov 8, 2017

View reviewed changes

paderinandrey added 2 commits November 9, 2017 07:59

fix scopes

04f54e0

converted ebay api module to class

9792e6c

fixed evilmartians#2

1ebbbee

Envek reviewed Nov 9, 2017

View reviewed changes

fixed evilmartians#3

1c8d285

Envek approved these changes Nov 9, 2017

View reviewed changes

Envek closed this Nov 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation Ebay OAuth2 Strategy for OmniAuth #1

Implementation Ebay OAuth2 Strategy for OmniAuth #1

paderinandrey commented Nov 3, 2017

Envek left a comment

Envek Nov 6, 2017

Envek Nov 6, 2017

Envek Nov 6, 2017

Envek Nov 6, 2017

Envek Nov 6, 2017

Envek Nov 6, 2017

Envek Nov 6, 2017

Envek Nov 6, 2017

Envek Nov 6, 2017

Envek left a comment

Envek Nov 8, 2017

Envek Nov 8, 2017

Envek Nov 8, 2017

paderinandrey commented Nov 9, 2017

Envek Nov 9, 2017

Envek commented Nov 9, 2017

Envek commented Nov 13, 2017

Implementation Ebay OAuth2 Strategy for OmniAuth #1

Implementation Ebay OAuth2 Strategy for OmniAuth #1

Conversation

paderinandrey commented Nov 3, 2017

Envek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Envek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paderinandrey commented Nov 9, 2017

Choose a reason for hiding this comment

Envek commented Nov 9, 2017

Envek commented Nov 13, 2017