Checkout V2: Retain and refresh OAuth access token #5098

sinourain · 2024-04-16T22:10:13Z

This changes contains:

Retain OAuth access token to be used on subsequent transactions and refreshed when it expires

Spreedly reference:
ECS-2996

Unit:
66 tests, 403 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed
2902.89 tests/s, 17725.19 assertions/s

Remote:
103 tests, 254 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed
0.63 tests/s, 1.56 assertions/s

aenand · 2024-04-17T15:05:18Z

lib/active_merchant/billing/gateways/checkout_v2.rb

-        auth_token = @access_token ? "Bearer #{@access_token}" : @options[:secret_key]
+        auth_token = "Bearer #{@options[:access_token] || @options[:secret_key]}"


This could be a breaking change as this is altering the authentication from Basic to Bearer auth for gateway tokens that only have a secret_key. Why are we making this change? Is it compatible to use Bearer auth with a secret_key?

This change is because the following guide and this ticket that described it, to continue supporting the secret_key in current merchants, I could add a condition that when the secret_key includes sk_sbox_ add Bearer and otherwise do not include the Bearer way

I have an assumption that the previous secret_key ends with a uuid v4, so I am going to upload my changes so that all the tests in CORE work and once I have confirmation if my assumption is correct or not I will update the code (if it is necessary) and mention it to continue with the review :)

I removed these changes from this PR, they caused a lot of confusion and they will be in a new PR :)

almalee24 · 2024-04-18T15:19:43Z

lib/active_merchant/billing/gateways/checkout_v2.rb

+        response = parse(ssl_post(access_token_url, 'grant_type=client_credentials', access_token_header))
+        @options[:access_token] = response['access_token']
+      rescue ResponseError => e
+        @multiresponse = MultiResponse.new


Why add a MultiResponse here?

The initial idea was to obtain the response when the access_token is created so that we can debug better in case something fails and know what is related to the access_token, I moved this logic elsewhere and I think it looks better :)

almalee24 · 2024-04-18T15:22:14Z

test/remote/gateways/remote_checkout_v2_test.rb

@@ -1,11 +1,13 @@
+require 'pry'


This can probably be removed now

Yes, thanks!

almalee24 · 2024-04-18T16:56:55Z

lib/active_merchant/billing/gateways/checkout_v2.rb

@@ -508,8 +510,17 @@ def response(action, succeeded, response, options = {}, source_id = nil)
      end

      def headers(action, options)
-        auth_token = @access_token ? "Bearer #{@access_token}" : @options[:secret_key]
-        auth_token = @options[:public_key] if action == :tokens
+        auth_token =


I would prefer if these was it's own PR like it was before. It would make it a lot easier to diagnose a problem if something goes wrong when it's deployed.

Sure, I will

Please don't forget to make this into it's on PR

I did it, it's just that in this same PR I added the changes from the ticket: ECS-3487 about Update Authorization from Basic to Bearer for Checkout gateway

I know but it's better if these are in two separate PRs. One for retaining and refreshing the access_token and another for updating the authorization. Keeping this two changes separate will make it easier to diagnose if something goes wrong when we merge it.

I removed these changes from this PR, they caused a lot of confusion and they will be in a new PR

almalee24 · 2024-04-18T17:18:13Z

lib/active_merchant/billing/gateways/checkout_v2.rb

        rescue ResponseError => e
+          if e.response.to_s.match?(/Unauthorized/) && @options[:access_token] && action != :tokens
+            @options[:access_token] = nil
+            return commit(action, post, options, authorization, method)


I'm a bit worried this could cause a constant loop if a transaction fails and it going through commit again and again. I'm not sure if that possible but wanted to bring it up.

I also don't think Unauthorized is a good error message to retry since I think this is a clear indication that the credentials are incorrect. The error that brought this ticket is Failed with 429 Too Many Requests so I'm not sure if this is a better one

I changed the implementation of this part, to do this I added a new field in the gateway options called expires, now we prevent the loop and we know better what the error is in case it occurs

almalee24 · 2024-04-22T13:53:47Z

lib/active_merchant/billing/gateways/checkout_v2.rb

      end

-      def commit(action, post, options, authorization = nil, method = :post)
+      def access_token_valid?
+        @options[:access_token] && @options.fetch(:expires, 0) > DateTime.now.strftime('%Q').to_i


I would suggest making sure the the default value for expires is 0. Currently @options.fetch(:expires, 0) could be nil which cause a NoMethodError when checking if it's less than

Oh it think that's what you are trying to do with fetch and make 0 the default but for some reason it's not working

I will change it to:

@options[:access_token] && @options[:expires] && @options[:expires] > DateTime.now.strftime('%Q').to_i

almalee24 · 2024-04-22T14:28:39Z

lib/active_merchant/billing/gateways/checkout_v2.rb

-          response = parse(raw_response)
+        response = parse(ssl_post(access_token_url, 'grant_type=client_credentials', access_token_header))
+        @options[:access_token] = response['access_token']
+        @options[:expires] = (DateTime.now + response.fetch('expires_in', 0).seconds).strftime('%Q').to_i


Some thing for this fetch it needs to be updated since it's not going to default to 0

almalee24 · 2024-04-22T14:31:06Z

lib/active_merchant/billing/gateways/checkout_v2.rb

+          error_code: error_code_from(false, response, {})
+        )
+      rescue ResponseError => e
+        Response.new(false, e.response.body.to_s, {}, test: test?)


Considering the way that y'all wrote this PR think this could be an OAuthResponseError instead

Sure, I will

lib/active_merchant/billing/gateways/checkout_v2.rb

curiousepic · 2024-04-22T19:20:41Z

@almalee24 @sinourain I'd like to explore options other than tracking an expiration date before we merge this - will chat offline.

curiousepic · 2024-04-22T20:08:10Z

lib/active_merchant/billing/gateways/checkout_v2.rb

-      def response(action, succeeded, response, options = {}, source_id = nil)
+      def commit(action, post, options, authorization = nil, method = :post)
+        if @options[:secret_key] || access_token_valid?
+          peform_payin(action, post, options, authorization, method)


What will happen if the token is valid when this is executed, but due to communications lag, it expires before the gateway receives it? It will not attempt to refresh the token, and will just fail with an auth failure, right?

The token is valid for 60mins so if there's a communication lag it would be a few minutes at most but yes if it expires it would fail with an auth failure. But if the customer is retries the transaction we would refresh the token.

To prevent transactions from being made very close to the expiration and therefore failing due to lag, I was thinking that we could, for example, subtract a minute or two from the expiration time (expires) and with this we prevent there from being a lower failure rate. What do you think?

almalee24 · 2024-04-26T16:01:42Z

lib/active_merchant/billing/gateways/checkout_v2.rb

+        @options[:access_token].present? && @options[:expires] && @options[:expires].to_i > DateTime.now.strftime('%Q').to_i
+      end
+
+      def peform_payin(action, post, options, authorization = nil, method = :post)


I think this is a small typo but it should be perform_payin

almalee24 · 2024-04-26T16:01:59Z

lib/active_merchant/billing/gateways/checkout_v2.rb

+      def commit(action, post, options, authorization = nil, method = :post)
+        MultiResponse.run do |r|
+          r.process { setup_access_token } unless @options[:secret_key] || access_token_valid?
+          r.process { peform_payin(action, post, options, authorization, method) }


Same typo here

almalee24 · 2024-04-26T16:03:14Z

lib/active_merchant/billing/gateways/checkout_v2.rb

      end

-      def commit(action, post, options, authorization = nil, method = :post)
+      def access_token_valid?
+        @options[:access_token].present? && @options[:expires] && @options[:expires].to_i > DateTime.now.strftime('%Q').to_i


Since you've defaulted expires to be 0 i don't think the extract @options[:expires] is necessary

almalee24 · 2024-04-26T16:18:30Z

lib/active_merchant/billing/gateways/checkout_v2.rb

        rescue ResponseError => e
+          @options[:access_token] = '' if e.response.code == '401'


I would suggest just to added check here for access_token in case the the transaction fails when using the secret_key

good catch!

@almalee24 I'm adding a test for this but it doesn't make sense since the access_token takes precedence over secret_key according to this line of code

Yeah I guess it's not a big issue but if secret_key transaction fails with 401 then we do @options[:access_token] = '' which we don't need to do because we didn't use access_token. It's not breaking anything so it's not something you need to change but a nice improvement I guess.

oh, ok, i got it

almalee24 · 2024-04-26T17:46:45Z

lib/active_merchant/billing/gateways/checkout_v2.rb

-        end
+        Response.new(
+          response['access_token'].present?,
+          message_from(true, response, {}),


Just another small issue is that this should only be true if response['access_token'].present?

Will I use access_token_valid? as a verifier since we assign parts of the response that allow us to validate the presence of the access_token and the expires

Ok, you just also have to update message_from(true, response, {}) to be message_from(access_token_valid?, response, {})

almalee24 · 2024-04-26T18:40:35Z

lib/active_merchant/billing/gateways/checkout_v2.rb

+          message_from(true, response, {}),
+          response.merge({ expires: @options[:expires] }),
+          test: test?,
+          error_code: error_code_from(false, response, {})


It would be the same here, it should be error_code_from(access_token_valid?, response, {})

almalee24

Looks good to me. Thanks for all the hard work!

curiousepic · 2024-04-26T20:33:52Z

lib/active_merchant/billing/gateways/checkout_v2.rb

-          response = parse(raw_response)
+        response = parse(ssl_post(access_token_url, 'grant_type=client_credentials', access_token_header))
+        @options[:access_token] = response['access_token']
+        @options[:expires] = response['expires_in'] && response['expires_in'] > 0 && (DateTime.now + (response['expires_in'] - 120).seconds).strftime('%Q').to_i


This is confusing to me with the && behavior with integers mixed with booleans. Could you add a comment that explains the purpose? Or maybe extract to named helper methods?

Yes, I'm going to create a comment and a new method

curiousepic · 2024-04-26T20:39:28Z

lib/active_merchant/billing/gateways/checkout_v2.rb

+        @options[:access_token].present? && @options[:expires].to_i > DateTime.now.strftime('%Q').to_i
+      end
+
+      def perform_payin(action, post, options, authorization = nil, method = :post)


payin is an unusual term for AM - I'd suggest just perform_request

ok, thanks!

curiousepic

Thanks for the tweaks!

Retain OAuth access token to be used on subsequent transactions and refreshed when it expires Spreedly reference: [ECS-2996](https://spreedly.atlassian.net/browse/ECS-2996) Unit: 66 tests, 403 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed 2902.89 tests/s, 17725.19 assertions/s Remote: 103 tests, 254 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications 100% passed 0.63 tests/s, 1.56 assertions/s

aenand reviewed Apr 17, 2024

View reviewed changes

almalee24 requested a review from a team April 17, 2024 20:51

sinourain force-pushed the ECS-2996_Checkout_v2_Retain_refresh_access_token branch from 11f1e79 to 4189241 Compare April 18, 2024 13:18

almalee24 reviewed Apr 18, 2024

View reviewed changes

sinourain force-pushed the ECS-2996_Checkout_v2_Retain_refresh_access_token branch from 4189241 to d9ae367 Compare April 19, 2024 14:23

almalee24 reviewed Apr 22, 2024

View reviewed changes

sinourain changed the title ~~Checkout V2: Retain and refresh OAuth access token~~ Checkout V2: Retain and refresh OAuth access token & Update Authorization from Basic to Bearer for Checkout gateway Apr 22, 2024

almalee24 reviewed Apr 22, 2024

View reviewed changes

lib/active_merchant/billing/gateways/checkout_v2.rb Outdated Show resolved Hide resolved

curiousepic reviewed Apr 22, 2024

View reviewed changes

sinourain force-pushed the ECS-2996_Checkout_v2_Retain_refresh_access_token branch from d9ae367 to 006d44e Compare April 25, 2024 15:25

sinourain changed the title ~~Checkout V2: Retain and refresh OAuth access token & Update Authorization from Basic to Bearer for Checkout gateway~~ Checkout V2: Retain and refresh OAuth access token Apr 25, 2024

sinourain force-pushed the ECS-2996_Checkout_v2_Retain_refresh_access_token branch 6 times, most recently from f32fabf to 372ecc2 Compare April 26, 2024 03:03

almalee24 reviewed Apr 26, 2024

View reviewed changes

sinourain force-pushed the ECS-2996_Checkout_v2_Retain_refresh_access_token branch from 372ecc2 to 086c17c Compare April 26, 2024 17:03

almalee24 reviewed Apr 26, 2024

View reviewed changes

sinourain force-pushed the ECS-2996_Checkout_v2_Retain_refresh_access_token branch from 086c17c to bd03d37 Compare April 26, 2024 18:26

almalee24 reviewed Apr 26, 2024

View reviewed changes

sinourain force-pushed the ECS-2996_Checkout_v2_Retain_refresh_access_token branch from bd03d37 to e8d04a1 Compare April 26, 2024 18:54

almalee24 approved these changes Apr 26, 2024

View reviewed changes

curiousepic reviewed Apr 26, 2024

View reviewed changes

sinourain force-pushed the ECS-2996_Checkout_v2_Retain_refresh_access_token branch from e8d04a1 to 09ce962 Compare April 29, 2024 14:32

curiousepic approved these changes Apr 30, 2024

View reviewed changes

almalee24 force-pushed the ECS-2996_Checkout_v2_Retain_refresh_access_token branch from 09ce962 to 311d29a Compare May 1, 2024 14:36

almalee24 merged commit 311d29a into master May 1, 2024
0 of 5 checks passed

almalee24 deleted the ECS-2996_Checkout_v2_Retain_refresh_access_token branch May 1, 2024 14:44

		auth_token = @access_token ? "Bearer #{@access_token}" : @options[:secret_key]
		auth_token = "Bearer #{@options[:access_token] \|\| @options[:secret_key]}"

		rescue ResponseError => e
		@options[:access_token] = '' if e.response.code == '401'

Checkout V2: Retain and refresh OAuth access token #5098

Checkout V2: Retain and refresh OAuth access token #5098

Conversation

sinourain commented Apr 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sinourain Apr 25, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

almalee24 Apr 18, 2024 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curiousepic commented Apr 22, 2024

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

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

almalee24 left a comment

Choose a reason for hiding this comment

curiousepic Apr 26, 2024 • edited

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

curiousepic left a comment

Choose a reason for hiding this comment

sinourain commented Apr 16, 2024 •

edited

sinourain Apr 25, 2024 •

edited

almalee24 Apr 18, 2024 •

edited

curiousepic Apr 26, 2024 •

edited