Skip to content
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

Add support for customer login #128

Merged
merged 1 commit into from
Jul 8, 2016
Merged

Add support for customer login #128

merged 1 commit into from
Jul 8, 2016

Conversation

mattolson
Copy link
Contributor

  • Add instance method on Customer object to generate
    a login token for that customer. This only works if
    the app has the store_v2_customer_login scope and is
    installed on the store.

@mattolson
Copy link
Contributor Author

@philipmuir @pedelman

# Generate a token that can be used to log the customer into the storefront.
# This requires your app to have the store_v2_customers_login scope and to
# be installed in the store.
def login_token
Copy link
Contributor

@pedelman pedelman Jul 7, 2016

Choose a reason for hiding this comment

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

In order to protect the thread safety of this, we should allow a user to pass in the store_hash, client_id, and client_secret (AKA a config).

def login_token(config: Bigcommerce.config)
  payload = {
    'iss' => config.client_id,
    'iat' => Time.now.to_i,
    'jti' => "#{config.client_id}-#{SecureRandom.hex(16)}",
    'operation' => 'customer_login',
    'store_hash' => config.store_hash,
    'customer_id' => id
  }

  JWT.encode(payload, config.client_secret, 'HS256')
end

With that I can do this:

Bigcommerce.configure do |config|
  config.store_hash = ENV['BC_STORE_HASH']
  config.client_id = ENV['BC_CLIENT_ID']
  config.client_secret = ENV['BC_CLIENT_SECRET']
  config.access_token = ENV['BC_ACCESS_TOKEN']
end

# Not thread safe
Bigcommerce::Customer.new(id: 123).login_token

or

# Thread safe
config = Bigcommerce::Config.new(
  store_hash: ENV['BC_STORE_HASH'], 
  client_id: ENV['BC_CLIENT_ID'], 
  client_secret: ENV['BC_CLIENT_SECRET'],
  access_token: ENV['BC_ACCESS_TOKEN']
)

Bigcommerce::Customer.new(id: 123).login_token(config: config)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, will do

@pedelman
Copy link
Contributor

pedelman commented Jul 7, 2016

Mixed opinion about adding a specific JWT gem as a dependency, but its is a very useful thing to have.

What are others thoughts?

@pedelman
Copy link
Contributor

pedelman commented Jul 7, 2016

A few minor comments, but code looks very close and the tests are awesome. Do you feel this is a patch or minor version bump? I feel this is probably minor version since we are adding a new dependency.

@mattolson
Copy link
Contributor Author

@pedelman ♻️

@mattolson
Copy link
Contributor Author

@pedelman Semver speaks to the API of our library. I don't believe that adding a dependency changes that, since it is completely internal to the library. However, since we are adding functionality in a backward compatible way, it would be a minor version bump, i.e. 1.1.0


```rb
Bigcommerce.configure do |config|
config.client_secret = ENV['BC_CLIENT_SECRET']
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this example can show that you also need a store_hash and a client_id

* Add instance method on Customer object to generate
  a login token for that customer. This only works if
  the app has the store_v2_customer_login scope and is
  installed on the store.
@mattolson
Copy link
Contributor Author

@pedelman ♻️

@pedelman
Copy link
Contributor

pedelman commented Jul 8, 2016

Looks good to me! 👍

Lets sync about when we want to cut a 1.1.0 release for this -- I think its okay for it to sit in master since 1.0.0 is already released.

@pedelman pedelman merged commit 9844fa6 into bigcommerce:master Jul 8, 2016
@mattolson
Copy link
Contributor Author

Agreed, no rush to release a new version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants