intercom / intercom-dotnet

Intercom API client library for .NET
https://developers.intercom.io/reference
Apache License 2.0
63 stars 53 forks source link

Client.cs uses HttpBasicAuthenticator which appends invalid header #107

Open ladotonia opened 6 years ago

ladotonia commented 6 years ago

HttpBasicAuthenticator appends the header:

        public HttpBasicAuthenticator(string username, string password)
        {
            var token = Convert.ToBase64String(Encoding.UTF8.GetBytes(string.Format("{0}:{1}", username, password)));

            authHeader = string.Format("Basic {0}", token);
        }

while all the api docs suggest using the header -H 'Authorization:Bearer '

Are access tokens supported in this SDK? or does the authenticator need to be updated to support the updated api?

kmossco commented 6 years ago

Hey @ladotonia! This library already supports Access Tokens through this part of the code:

https://github.com/intercom/intercom-dotnet/blob/14f09901e63ca3a9dd3dbd34cc39379bae814f7e/src/Intercom/Core/Client.cs#L296-L306

It works correctly if you send the access token as the app_id with the password as an empty string, which is what this library is doing for now. We have plans to make sure it matches what the API docs refer but considering we had API keys until recently, it was postponed until we deprecate API keys for good.

Are you having problems sending requests?

ladotonia commented 6 years ago

Hi @kmossco, I did try using the single argument constructor for Intercom.core.Authentication using only my access token.

I am unable to create a new Contact or User through the .net SDK, however when I directly hit the rest api the same command is successful. The main difference I was noticing, was that the api mentions to send the access key in plain text as Bearer <access-key> where the HttpBasicAuthenticator converts <accesskey>: to a base64 string and sends it as Basic <base64-string>.

Here is my sdk code snippet:

ContactsClient intercomClient = new ContactsClient(new Intercom.Core.Authentication(accessToken));
Contact user = intercomClient.Create(new Contact() { user_id = id, email = email});

user is always null, as is the resturn from intercomClient.List();

kmossco commented 6 years ago

Hey again @ladotonia! Just to be sure, is Intercom.Core.Authentication a method you created? That doesn't seem to be how you instantiate the authentication in our library. Here's the correct code:

ContactsClient intercomClient = new ContactsClient(new Authentication("MyPersonalAccessToken"));
Contact user = intercomClient.Create(new Contact() { user_id = id, email = email });

Is it possible that you are using some other library than ours? I also can't find the <accesskey>: in our codebase.

ladotonia commented 6 years ago

Intercom.Core.Authentication is just the full namespace path to the same class Authentication that your code is using, so my snippet is functionally the same as yours.

The "<access-key>:" bit is just the result of what the HttpBasicAuthenticator Base64 encodes into the auth header if it is constructed as new HttpBasicAuthenticator("<access-key>",String.Empty); This is a result of the String.Format() call that's in my original issue comment.

kmossco commented 6 years ago

Gotcha, the reason I asked is that there is an unofficial client called Intercom.Core.Client and I was wondering if that was related. You are right in that we are not appending the best headers right now as the initial change made to accept access tokens was simply passing it as the app_id identifier and keeping the api_key field empty. This is fully supported and we have no plans to deprecate it but I agree that we should make sure we send the right header.

I'll add this to the roadmap of things to get fixed, but if you have the time/resources feel free to send us a PR to address this. Happy to review it and thank you for reporting this!

Just to set the proper expectations, we are currently looking to push the PR #97 forward and then we will focus on bringing pagination to the SDK. So I will probably only have time to address this after that is implemented. 👍