Add personal access token authentication

[martinda]

My first attempt to fix issue #121.

Bitbucket recently added personal access token authorization for accessing the REST API.

Please guide me on how to add test for this feature, because I am not sure the basic and base64 authorization methods were explicitly tested. Specifically, I am not sure how to assign a value to the username in src/test/java/com/cdancy/bitbucket/rest/TestUtilities.java because the Bearer authorization does not need it.

I tested this change against a local bitbucket instance with the following code:

BitbucketClient client = BitbucketClient.builder()
    .endPoint("https://bitbucket.domain.com/")
    .credentials("Bearer ODg0Nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx")
    .build();
Version version = client.api().systemApi().version();

[cdancy]

@martinda not a bad first go but we can simplify things some more. Taking into account your snippet above we should be able to do something like this:

BitbucketClient client = BitbucketClient.builder()
    .endPoint("https://bitbucket.domain.com/")
    .credentials("ODg0Nxxxxxxxxxxx") // notice we don't pass in "Bearer"
    .build();
Version version = client.api().systemApi().version();

The link I posted in the ISSUE you opened shows how you can implement this here. I'm thinking of copying quite literally that impl and use it here so that we can do something like:

      boolean useBasicAuth = isbase64 ? true : isBase64Encoded(foundCredential);
      String authHeader = useBasicAuth ? "Basic " : "Bearer "
      authHeader += foundCredential
      return request.toBuilder().addHeader(HttpHeaders.AUTHORIZATION, authHeader);

You could probably simplify and optimize that even more using a StringBuilder if you're up for it.

The big difference this solution provides over the current is that instead of throwing an exception if things are not base64 encoded we will instead assume they want to pass in a token and go with that.

As for testing you can do:

./gradlew integTest -PtestBitbucketCredential=<generated-bearer-token>

[martinda]

It bothers me too, but without the "Bearer " prefix, the personal access token matches the regex.

[cdancy]

I haven't looked too much into Bearer but can it be base64 encoded (maybe it's supposed to be)?

[martinda]

The Bearer token is passed as is to Bitbucket. When I decode it, it looks like garbage.

[cdancy]

@martinda is there anything descriptive about the format/structure of the String that we could use to detect if it's a "Bearer" value and without having to bring in external libraries? I see for JWT style formatting it goes something like cat.dog.mouse (meaning 3 parts separated by a period) but not sure if that is standard.

[martinda]

@cdancy I found the RFC documentation for Basic and Bearer Authorization. The bearer tokens I get from Bitbucket match both the basic expression AND the bearer expression. Decoding the Bitbucket bearer token sometimes results in a string with a colon, so decoding does make them distinguishable.

It is interesting to note that in both RFCs, the Authorization header does specify the type of authorization explicitly (the "Basic" and "Bearer" strings). Being explicit about the header type is probably the only way to distinguish them.

[cdancy]

@martinda yeah I was reading up on that a bit more last night and walked away with the same conclusions. However, I did have a potential epiphany that could work here and keep things sane without the user having to pass in the respective header.

When a user is using "Basic" auth they are passing along a username and password.

When a user is using "Bearer" auth they are passing along a key or token which I suppose you could say is analogous to a password but doesn't contain a username (hopefully that came out right).

Here's what I'm thinking: If the user wants "Basic" auth they can continue to do so in the normal way but if they want to use "Bearer" they can still pass in credentials in the normal way but omit the username. So instead of doing:

username:password

They could do:

:<token>

In the case where things are already encoded we'd have to find a way to tell if it's in the format of username:password or just :password. Maybe decode it and if it starts with a : assume "Bearer"?

[cdancy]

Or ... we could optionally provide the credentials to be passed in like:

basic@<optionally-encoded-creds>

OR

bearer@<optionally-encoded-creds>

To not break the existing api we will default to the former if no <auth-type>@ is present. This would allow things to work in the normal way whilst not complicating the passed credential process. We could do things in a more programmatic way but this library is heavily used in CI environments where the credentials are set as system properties or environment variables and so I want to keep that process the same and as clean as possible. This would give us a hint as to which auth-type to use and as I said above if none is found we default back to Basic.

Let me know what you think.

[martinda]

@cdancy

Unlike basic authorization, it is not possible to distinguish a non-encoded string from an encoded string when it comes to bearer authorization. Thus, I would amend your suggestion to remove the reference to encoding:

basic@<optionally-encoded-creds>
bearer@<personal-access-token>

This gives the following ways of specifying the various authorizations, which is backwards compatible for existing users:

• Colon delimited username and password: username:password, or basic@username:password
• Base64 encoded username and password: YWRtaW46cGFzc3dvcmQ= or basic@YWRtaW46cGFzc3dvcmQ=
• Personal Access Token: bearer@Mzg0NTxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx

If you think this works, I can complete the pull-request, but I am still not sure how to modify the block of code starting at src/test/java/com/cdancy/bitbucket/rest/TestUtilities.java.

[martinda]

@cdancy I ran the integration tests on this PR, and they work as well as on master branch a573276

I am ready for your review.

[cdancy]

@martinda away on vacation at the moment but will get back to this sometime this weekend to run some local tests here. However, and looking through this PR briefly on my phone, everything looks OK.

[cdancy]

@martinda been looking over this PR the past couple days and am going to send in some more commits to your branch sometime today. I found a potentially "easier" way to track and define the type of authentication being used after spending some time with your work. Stay tuned and let me know what you think.

[cdancy]

@martinda I refactored things quite a bit. Have a look over and let me know what you think. Some highlights include:

• No longer prepend system/env properties with basic or bearer but instead create a separate set of properties for Bearer (e.g. token) auth.
• When building a client user can now use the token(String value) method to pass along a token if desired over credentials(String value).

[cdancy]

FYI: as this is a breaking change I'm mulling over bumping minor or even the major version so as not to interrupt downstream use-cases.

[martinda]

@cdancy I like your implementation better. I have not tried it yet, but will probably do so before Monday.

You could use the @Deprecated annotation, maybe that would help with backward compatibility?

What should be done about src/test/java/com/cdancy/bitbucket/rest/TestUtilities.java ?

[cdancy]

@martinda good catch and need to address that. As base64 encoding, if required, will now be done within the BitbucketClient creation, instead of for every request, we should be able to largely remove all of that logic including what was there before.

[martinda]

@cdancy I ran the PR against my real world use case and it works. I am okay if you bump the major number since it is a breaking change.

[cdancy]

@martinda thanks for the additional check and confirmation. I'm going to do another round of cleanups today and will merge sometime at the beginning of the week and do a new release.

[martinda]

@cdancy Thank you. I also want to say that I learned a lot from reading your code, specifically with regards to the builder pattern and in the way externalities like credentials and endpoint are provided to the application.

[cdancy]

@martinda after playing with this for a while I decided to do things the "right" way and more/less do a complete refactor of how authentication is handled. The end-user/client shouldn't see a difference one way or another. Feel free to give things a go if you get a minute. Have a few more things I'd like to add before doing another release.

[martinda]

@cdancy Thanks again. I ran my tests again and everything is fine.

[vanlooverenkoen]

Any idea when you will release 1.0.6? I am writing a gradle plugin where we will include your library. But in order to get it working we need to use the personal access tokens.

[cdancy]

By the end of this week. I planned on releasing this past weekend but we had a feature request here to be able to pass in jclouds overrides properties, both through the client and via system properties or environment variables, to configure things in a very devops/CI sort of way much like I do with setting the endpoint and credentials.

[cdancy]

Version 2.0.0 has been released.