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

Connections and Disconnect calls started to return 404 #103

Closed
saboter opened this issue Dec 21, 2020 · 22 comments
Closed

Connections and Disconnect calls started to return 404 #103

saboter opened this issue Dec 21, 2020 · 22 comments

Comments

@saboter
Copy link

saboter commented Dec 21, 2020

Hi,

some of the changes in 2.5 release had nasty side effects in our implementation (main suspect d022f37#diff-2b72dffc27d2b59e684003eedd40ff55294d501dc0b4efdb0a90e4cb60b7febb)

After upgrade strange "XeroRuby::ApiError: Error message: the server returns an error HTTP status code: 404" exceptions started to pop up.

We are able to reproduce them by simply connecting and disconnecting organizations. Downgrade to the 2.4.1 fixes the issue.

Our authorization simplified flow (callback part):

        token_set = @client.get_token_set_from_callback(params)

        credentials = {
          access_token: token_set["access_token"],
          refresh_token: token_set["refresh_token"],
          id_token: token_set["id_token"]
        }

        token_hash = decoded_token(token_set["access_token"])
        auth_event_id = token_hash["authentication_event_id"]

        # 404 api error is raised at this point
        connections = @client.connections

        connection = connections.find { |t| t["authEventId"] == auth_event_id }
 

Any ideas what could be the root cause?

Thank you.

@SerKnight
Copy link
Contributor

Hey @saboter - the methods should still be returning the JSON in the exact same format so

Screen Shot 2020-12-21 at 12 14 03 PM

However they will now throw XeroRuby::ApiError properly like the rest of the project, unlike before when a failure was just surfacing back the string but not proper HTTP response.

@simbiont123 are you experiencing the same?

@SerKnight
Copy link
Contributor

@saboter can you post the whole 404 stack trace? I'd maybe expect to see 401 but if SDK is producing "Not Found" i'd like to get that sorted asap..

@saboter
Copy link
Author

saboter commented Dec 22, 2020

@SerKnight

Exception (body is empty):

XeroRuby::ApiError:
Error message: the server returns an error
HTTP status code: 404
Response headers: {"server"=>"nginx", "xero-correlation-id"=>"12a3674c-350a-4c44-b842-67691897e41a", "x-appminlimit-remaining"=>"9998", "content-length"=>"0", "expires"=>"Mon, 21 Dec 2020 12:24:56 GMT", "cache-control"=>"max-age=0, no-cache, no-store", "pragma"=>"no-cache", "date"=>"Mon, 21 Dec 2020 12:24:56 GMT", "connection"=>"keep-alive", "x-client-tls-ver"=>"tls1.3"}
Response body: 

Relevant stack trace lines:

.../bundle/ruby/2.7.0/gems/xero-ruby-2.5.0/lib/xero-ruby/api_client.rb line 200 in return_error
.../bundle/ruby/2.7.0/gems/xero-ruby-2.5.0/lib/xero-ruby/api_client.rb line 178 in call_api
.../bundle/ruby/2.7.0/gems/xero-ruby-2.5.0/lib/xero-ruby/api_client.rb line 137 in connections

The reason why our flow is broken is most probably because before without proper exception we handled the 404 simply like blank connection (no match to the auth event id). But why 404?

OT:

There is a typo in connections method comment.

    # Connection heplers
    def connections
      opts = { :header_params => {'Content-Type': 'application/json'}, :auth_names => ['OAuth2'] }
      response = call_api(:GET, 'https://api.xero.com/connections/', nil, opts)
      response[0]
    end

@saboter
Copy link
Author

saboter commented Dec 22, 2020

"The reason why our flow is broken is most probably because before without proper exception we handled the 404 simply like blank connection (no match to the auth event id)."

Taking back this part. When I rescued the 404 it still fails on missing "state" param which is not forwarded in case of 404.

@SerKnight
Copy link
Contributor

SerKnight commented Dec 22, 2020

@saboter - Not sure I understand.

I just released 2.5.1 which includes an optional state param to be passed through the authorization URL and subsequent callback.

https://github.com/XeroAPI/xero-ruby#creating-a-client


A few more questions:

  • Can you show the code you wrote that triggers the client.connections call?
  • Are you able to do the callback token exchange still?
  • Able to make other API calls?

This might be a better case for [email protected] as we might just need to look through your specific app logs. What is your client ID?

@simbiont123 are you also experiencing this?

P.S. Thanks - I will fix heplers later :)

@saboter
Copy link
Author

saboter commented Dec 23, 2020

@SerKnight Before the optional state param support we simply add it to at the end of authorization url string. Will try the new feature, but it should not change much in our case.

The code which triggers the connection is equivalent of this:

    token_set = @client.get_token_set_from_callback(params)
    connections = @client.connections

It happens in callback handler where we need to get connections for freshly authorized client. The broken version was couple of hours in production and other API calls were working only ".connections" and ".disconnect" have problems.

Will do some more debugging and then I will contact Xero Support or [email protected] directly. No public client ID disclosing :). Thank you.

@saboter
Copy link
Author

saboter commented Dec 23, 2020

@SerKnight I have examined the request differences between 2.4.1 and 2.5.1 into detail and the reason for the 404 is malformed url:

path="https://api.xero.com/api.xro/2.0/https:/api.xero.com/connections/"

During refactoring was omitted the fact that the call_api takes "path" not full url, it prefixes standard api url:

data, status_code, headers = call_api(:GET, 'https://api.xero.com/connections/', nil, opts)

the full url added in path param is processed in this method, where it is also malformed

   def build_request_url(path)
      if @config.base_url
        # Add leading and trailing slashes to path
        path = "/#{path}".gsub(/\/+/, '/')
        @config.base_url + path
      else
        path
      end
   end

Some Xero Api endpoints are special, with different base url, which most probably the initial reason why they were implemented without call_api method.

@SerKnight
Copy link
Contributor

SerKnight commented Dec 23, 2020

So the base_url should only be set if you are using one of the actual api sets:

def accounting_api
  @config.base_url = @config.accounting_url
  XeroRuby::AccountingApi.new(self)
end

The else => path was meant to handle the disconnect and connections route.

Are you using the client.connections after you call an accounting_api method?

I might need to add another test around this. Can you log what your @config.base_url is set to when this fails?

@saboter
Copy link
Author

saboter commented Dec 23, 2020

@SerKnight Later in our code is accounting_api.get_organisations call. Need to test if it is the reason why the url is malformed.

In general it is not very fortunate when the same instance of client can have the config altered with forwarded accounting_api call.

@SerKnight
Copy link
Contributor

SerKnight commented Dec 24, 2020

Yea agreed. Let me know reality of test + I’ll recreate and get that fixed and well tested.

@whatthewhat
Copy link
Contributor

We've just updated xero-ruby on our OAuth2 migration branch and we're also getting this issue, the rspec output demonstrates it best:

 ...
     Failure/Error: connections = xero_client.connections

     WebMock::NetConnectNotAllowedError:
       Real HTTP connections are disabled. Unregistered request: GET https://api.xero.com/api.xro/2.0/https:/api.xero.com/connections/ with headers {'Accept'=>'*/*', 'Accept-Encoding'=>'gzip;q=1.0,deflate;q=0.6,identity;q=0.3', 'Authorization'=>'Bearer eyJhbGciOiJub25lIn0.eyJ4ZXJvX3VzZXJpZCI6Inhlcm8tdXNlci1pZCJ9.', 'Content-Type'=>'application/json', 'User-Agent'=>'xero-ruby-2.5.1'}

       You can stub this request with the following snippet:

       stub_request(:get, "https://api.xero.com/api.xro/2.0/https:/api.xero.com/connections/").
 
...

       registered request stubs:
 
       stub_request(:get, "https://api.xero.com/connections/")

The code that triggers it:

xero_client = XeroRuby::ApiClient.new(...)
token_set = xero_client.get_token_set_from_callback('code' => @code)
xero_client.set_token_set(token_set)
connections = xero_client.connections

@whatthewhat
Copy link
Contributor

It's a little tricky to follow the implementation, but I think one of the issues might be that the config is actually shared between api client instances here. So in our case a different test might have mutated the global config (and changed the base_url), even though it was using a different client instance.

@SerKnight could you comment if that is accurate/done intentionally? Thanks!

@saboter saboter closed this as completed Dec 28, 2020
@saboter saboter reopened this Dec 28, 2020
@saboter
Copy link
Author

saboter commented Dec 28, 2020

Sorry, miss-clicked and closed the issue by accident :).

@SerKnight
Copy link
Contributor

This is related to the base url fix in the next release

@SerKnight
Copy link
Contributor

@raouldevil
Copy link

@SerKnight Thanks for this fix, we experienced this issue as well so are grateful for the fix, will upgrade to 2.6.0 shortly.

@matthieua
Copy link

Thanks @SerKnight but I'm still getting the same issue with 2.6.0

@SerKnight
Copy link
Contributor

Hmm @matthieua - can you share an example of how you triggered the issue using 2.6.0?

There might be some situations that we can improve in the specs for this.

@matthieua
Copy link

matthieua commented Jan 28, 2021

image

@SerKnight Sorry for the slow reply, this is what I get. I just tested it with 2.6.1 and still the same so I'm reverting back to 2.4.1 which works great.

@matthieua
Copy link

@SerKnight sorry to bother you but we're still getting this issue even with the latest release 2.7.0 :(

@SerKnight
Copy link
Contributor

SerKnight commented Feb 19, 2021

Hey @matthieua not a bother at all.

Can you share the whole method stack that is triggering this and your gemfile lock?

Seeing the client = setup might be helpful.

I can re-open this issue or we can start a new one. I wasn't able to reproduce really want to make sure you get it solved.

@SerKnight
Copy link
Contributor

Actually sorry to trouble but can you create a new issue with everything summarized?

In addition the fastest way to get a bug triaged is if we can replicate it in the sample app - https://github.com/XeroAPI/xero-ruby-oauth2-app

There are obviously different dependencies / setup in every system but that makes it clear in a stripped down app how to replicate.

I've just tried with the recent version calling two separate API namespace that have a different base_urls and could not get SDK to trip.

Screen Shot 2021-02-18 at 6 17 55 PM

Screen Shot 2021-02-18 at 6 14 11 PM

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

No branches or pull requests

5 participants