Use AuthenticationTokens #101

vault · 2018-01-03T16:22:38Z

Opening for initial feedback.

This is based on #92 and moves to using the AuthenticationTokens plugin as was suggested in a couple of other pull-requests (#69, #68, and #94). This also includes support for client certificates like #94.

jetersen · 2018-01-03T16:30:09Z

@vault think you need to rebase onto PR #92 again and resolve conflicts

stephenc

A good start

stephenc · 2018-01-03T21:25:22Z

...dbees/jenkins/plugins/bitbucket/api/credentials/BitbucketClientCertificateAuthenticator.java

+        try {
+            builder.setSSLContext(buildSSLContext());
+        } catch (NoSuchAlgorithmException | UnrecoverableKeyException | KeyStoreException | KeyManagementException ignored) {
+


I think we'll need to either pass a TaskListener to these methods or log the exceptions. Certainly unacceptable to ignore and continue.

May even need to wrap as an IOException and throw

stephenc · 2018-01-03T21:25:46Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/api/BitbucketAuthenticator.java

+import org.apache.http.client.protocol.HttpClientContext;
+import org.apache.http.impl.client.HttpClientBuilder;
+
+public abstract class BitbucketAuthenticator<T extends Credentials> {


On the plus side, you are implementing a strategy

stephenc · 2018-01-03T21:26:46Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/api/BitbucketAuthenticator.java

+
+public abstract class BitbucketAuthenticator<T extends Credentials> {
+
+    public final T credentials;


On the minus side you are basically wrapping a Credentials... please don't do that, extract what you need from the Credential and move on, that way the BitbucketAuthenticator can be used from remote agents rather than only from the master JVM

Yes that means you need a subclass for each type of credential that is supported... but you needed that anyway. Also... just say no to public fields

stephenc · 2018-01-03T21:28:27Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/client/BitbucketCloudApiClient.java

 import static com.cloudbees.jenkins.plugins.bitbucket.Utils.encodePath;

 public class BitbucketCloudApiClient implements BitbucketApi {
    private static final Logger LOGGER = Logger.getLogger(BitbucketCloudApiClient.class.getName());
+    private static final HttpHost API_HOST = HttpHost.create("https://api.bitbucket.org");


I strongly recommend waiting until after #80 is merged (or else rebasing this PR on top of #80)

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/endpoints/AbstractBitbucketEndpoint.java

stephenc · 2018-01-03T21:35:00Z

...a/com/cloudbees/jenkins/plugins/bitbucket/endpoints/AbstractBitbucketEndpointDescriptor.java

                URIRequirementBuilder.fromUri(serverUrl).build(),
-                CredentialsMatchers.instanceOf(StandardUsernamePasswordCredentials.class)
+                CredentialsMatchers.anyOf(


No, you should be using AuthenticationTokens.matcher(BitbucketAuthenticator.class) here. No need to test for instanceOf any more. In fact this switch is the key reason to move to AuthenticationTokens... as it means an extension plugin can register more authenticator sources and then the support wires right through

stephenc · 2018-01-03T21:35:30Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/endpoints/BitbucketCloudEndpoint.java

 import org.kohsuke.stapler.DataBoundConstructor;

+import javax.annotation.Nonnull;


There is no published JSR 305. Please use edu.umd.cs.findbugs.annotations.NonNull

stephenc · 2018-01-03T21:36:41Z

...ain/java/com/cloudbees/jenkins/plugins/bitbucket/server/client/BitbucketServerAPIClient.java

+import org.apache.http.message.BasicNameValuePair;
+import org.apache.http.util.EntityUtils;
+import org.codehaus.jackson.type.TypeReference;
+


all imports in one section sorted alphabetically: https://github.com/jenkinsci/scm-api-plugin/blob/master/CONTRIBUTING.md details the code style

stephenc · 2018-01-03T21:37:29Z

src/test/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketMockApiFactory.java

 import edu.umd.cs.findbugs.annotations.CheckForNull;
 import edu.umd.cs.findbugs.annotations.NonNull;
 import edu.umd.cs.findbugs.annotations.Nullable;
 import hudson.Extension;
 import hudson.ExtensionList;
+import org.apache.commons.lang.StringUtils;
+


Check your IDE settings for import sorting... it's all of them not just the two I have tagged

jetersen · 2018-01-18T13:38:16Z

@vault this should now be in a good state to rebase 👍

stephenc · 2018-01-18T13:43:14Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketSCMNavigator.java

                    URIRequirementBuilder.fromUri(bitbucketServerUrl).build(),
-                    CredentialsMatchers.anyOf(CredentialsMatchers.instanceOf(StandardUsernamePasswordCredentials.class))
+                    CredentialsMatchers.anyOf(


AuthenticationTokens.matcher(BitbucketCredentials.class) should suffice instead of the credentials matchers

NOTE: you will need to use an AuthenticationTokenContext to handle the different types of auth for http vs https urls, so at that point you will change to AuthenticationTokens.matcher(BitbucketCredentials.class, authContext)

NOTE2: This comment applies to everywhere you are using CredentialsMatchers.instanceOf

stephenc · 2018-01-18T13:44:39Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketSCMSource.java

+                        CredentialsProvider.lookupCredentials(StandardCertificateCredentials.class,
+                                context,
+                                context instanceof Queue.Task ? Tasks.getDefaultAuthenticationOf((Queue.Task) context) : ACL.SYSTEM,
+                                URIRequirementBuilder.fromUri(bitbucketServerUrl).build()), CredentialsMatchers.withId(value)) != null) {


You will need both withId and AuthenticationTokens.matcher(BitbucketCredentials.class) to handle the case where there are two different types of credentials with the same id at different layers in the folder hierarchy.

The point being that the drop-down list will be showing only those that AuthenticationTokens.matcher(BitbucketCredentials.class) so may hide higher up ones that happen to have the same id but don't match the type we want...

say at the root there is a userpass with id foo and one folder down there is a certificate with id foo... if the url is http we should be able to select foo and mean the userpass...

and such a config could actually be desired, e.g. against https we want to use the stronger client cert... against http we have no choice... and we set up everything to always have id of foo so that the choice is easy... not a config I'd recommend... but it is logical... and i think credentials-api would support it

vault · 2018-01-26T02:56:59Z

I think I've addressed most of the requests here. The main omissions being I'm not sure what else to do in the client-cert exception case other than log something. What other changes do you think this needs? Any thoughts on how to handle that case correctly? I looked into TaskListener a bit but I wasn't able to figure out the right way to use it here.

jetersen

LGTM, though a unit test for testing the convert different credential would be nice.

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/api/BitbucketAuthenticator.java

jetersen · 2018-01-26T03:10:19Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/api/BitbucketAuthenticator.java

+     */
+    public void configureBuilder(HttpClientBuilder builder) { }
+
+


👻 whitespace

jetersen · 2018-01-26T03:10:52Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/api/BitbucketAuthenticator.java

+     */
+    public void configureContext(HttpClientContext context, HttpHost host) { }
+
+


👻 whitespace

jetersen · 2018-01-26T03:11:00Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/api/BitbucketAuthenticator.java

+     */
+    public void configureRequest(HttpRequest request) { }
+
+


👻 whitespace

stephenc

Looking good... I may use this PR as an example of how to do things... once it's ready

Some changes / improvements I have suggested

I have not had time to give a full review today

stephenc · 2018-01-26T17:36:52Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/api/BitbucketAuthenticator.java

+    /**
+     * The URL protocol as reported in an {@link AuthenticationTokenContext}
+     */
+    public static final String PROTOCOL_PURPOSE = "PROTOCOL";


I would call this Scheme

For the constant name, or its string value? Also, <blah>_PURPOSE reads kind of weird. Is there a naming scheme that would be more preferred than that?

I’d use SCHEME = “scheme” or SERVER_URI_SCHEME = “bitbucket.server.uri.scheme”

The length of the constant doesn’t use up more space as it will still be an 8 byte reference to the constant pool anyway, so more descriptive is better

stephenc · 2018-01-26T17:38:30Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/api/BitbucketAuthenticator.java

+    /**
+     * The purpose value for HTTP instances
+     */
+    public static final String PROTOCOL_HTTP = "HTTP";


just use the URI scheme rather than a constant (i.e. parse the URL, or just substring up to the first : and lowercase the scheme part)... if you want to have constants for HTTP and HTTPS their values should be the same as an URL (i.e. lowercase)

stephenc · 2018-01-26T17:39:29Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/api/BitbucketAuthenticator.java

+    /**
+     * The Bitbucket instance type as reported in an {@link AuthenticationTokenContext}
+     */
+    public static final String INSTANCE_TYPE_PURPOSE = "INSTANCE_TYPE";


Make it clear that this is bitbucket, a better value would be Bitbucket.Type

stephenc · 2018-01-26T17:39:42Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/api/BitbucketAuthenticator.java

+     * @param serverUrl The URL being authenticated against
+     * @return an {@link AuthenticationTokenContext} for use with the AuthenticationTokens APIs
+     */
+    public static AuthenticationTokenContext<BitbucketAuthenticator> authenticationContext(String serverUrl) {


stephenc · 2018-01-26T17:41:43Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/api/BitbucketAuthenticator.java

+        boolean isCloud = serverUrl == null || serverUrl.equals(BitbucketCloudEndpoint.SERVER_URL);
+
+        return AuthenticationTokenContext.builder(BitbucketAuthenticator.class)
+                .with(PROTOCOL_PURPOSE, isHttps ? PROTOCOL_HTTPS : PROTOCOL_HTTP)


I think you could be perfectly fine not to parse the URL and just put the URL in the token context as url and be done... let the impls decide if the url is cloud or server...

fine to do it here, but I would recommend adding the URL to the token context also in that case, as you don't know what will need it further down the road

(for example a credential may only be valid against a specific server)

[JENKINS-44784] Ensure workspace cleanup for deleted branch projects proceeds asynchronously, and will time out if the agent is stuck

rsandell · 2018-04-24T10:42:56Z

I'm not sure what @stephenc 's philosophy has been on this, but there are signature changes in a couple of unrestricted public classes and extension points (constructor changes, methods removed etc.)
In general we make an effort to keep old signatures (but deprecated) and make them work with the new code.

vault · 2018-05-11T21:55:39Z

I should be able to fix that. I'll try to annotate changes that are problematic and try to get them fixed in the next day or so.

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/api/BitbucketApiFactory.java

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/client/BitbucketCloudApiClient.java

...ain/java/com/cloudbees/jenkins/plugins/bitbucket/server/client/BitbucketServerAPIClient.java

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketSCMNavigator.java

vault · 2018-05-15T16:28:07Z

Rebased and added compatibility methods for what I think are all the important places this branch wasn't backwards compatible.

Let me know if it seems like there's something I missed.

michelzanini · 2018-07-04T11:39:02Z

Apart from username/password and client certificates, does this PR also supports using a OAuth token to connect to the REST API of Bitbucket?

vault · 2018-07-09T18:10:05Z

This PR doesn't add support for OAuth tokens, but it should make adding that support easier.

andrewspiers · 2018-07-30T05:59:19Z

Is the only thing blocking this @stephenc 's review of 4th January?

stephenc · 2018-07-30T06:29:16Z

When I moved projects, bobby was primarily assigned responsibilities for this plugin. If there are significant api changes, then he’s supposed to call me in, but I haven’t seen same.

(Also I don’t know if bobby’s responsibilities have been reassigned since)

When I looked at this pr last it seemed headed in the right direction, but I haven’t had the time to re-review and I haven’t been called in by bobby, so my current understanding is bobby needs to review... though that may be wrong

hoegertn · 2018-08-30T06:24:07Z

Are there plans to merge this? As I see it, currently it is not possible to use this plugin with Bitbucket Cloud.

jetersen · 2018-09-22T05:06:24Z

@vault please rebase 😇

jetersen

LGTM except for the recent unresolved conflict 😇

Stale

jetersen

LGTM I would like @stephenc to do another review pass if you have the time to spare 😅
There is gonna be a merge conflict with #138, but will manage

...dbees/jenkins/plugins/bitbucket/api/credentials/BitbucketClientCertificateAuthenticator.java

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/api/BitbucketAuthenticator.java

...udbees/jenkins/plugins/bitbucket/api/credentials/BitbucketUsernamePasswordAuthenticator.java

...jenkins/plugins/bitbucket/api/credentials/BitbucketClientCertificateAuthenticatorSource.java

.../jenkins/plugins/bitbucket/api/credentials/BitbucketUsernamePasswordAuthenticatorSource.java

jetersen · 2018-10-27T19:22:02Z

Could you please rebase 😇

stephenc

From an API and direction Point Of View: This looks great.

Assuming you:

rebase
fix the files missing license headers
address other code review comments

You get my vote

stephenc · 2018-10-27T20:55:40Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/client/BitbucketCloudApiClient.java

+        cacheKey.append(owner);
+
+        if (authenticator != null) {
+            cacheKey.append("::").append(authenticator.getId());


So.... the change from username to credentials id may mean that we lose the cache where there are two credentials instances with the same username... OTOH that might have been a way to capture information if we had been caching more than repositories... so likely not a bad thing... and this cache is internal, so if needs be could be refactored to a different key later.

jetersen · 2018-10-27T21:32:30Z

Just to cover @hoegertn comment about being unable to use for Bitbucket Cloud is not true.
Create an atlassian user account using username & password credentials with username being the email of the user seems to be working just fine.

* Introduces a BitbucketAuthenticator class that is responsible for configuring the http client used to access the bitbucket rest API. * Adds 2 concrete subclasses of BitbucketAuthenticator capable of authentication with normal username/password and client certificates * Uses the authentication-tokens plugin to register these authentication methods and acquire the correct instance type for the supplied credentials. * Adds a message instructiong the user to configure SSH checkout if a certificate credentials have been selected.

* Don't just wrap Credentials in BitbucketAuthenticators * Use AuthenticationTokens.matcher * Log a message in exception handler

vault · 2018-10-31T20:17:12Z

Rebased and added license headers which I think addresses all outstanding issues.

vault force-pushed the authentication-token-plugin branch from d337625 to e4130d2 Compare January 3, 2018 16:31

stephenc previously requested changes Jan 3, 2018

View reviewed changes

stephenc mentioned this pull request Jan 3, 2018

JENKINS-48517 Use client certificates #94

Closed

vault force-pushed the authentication-token-plugin branch 2 times, most recently from 5d4a3ff to 93b56e2 Compare January 9, 2018 16:17

stephenc reviewed Jan 18, 2018

View reviewed changes

vault force-pushed the authentication-token-plugin branch 2 times, most recently from 8c4a90c to 16859cb Compare January 26, 2018 02:40

jetersen approved these changes Jan 26, 2018

View reviewed changes

stephenc reviewed Jan 26, 2018

View reviewed changes

vault commented May 11, 2018

View reviewed changes

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/api/BitbucketApiFactory.java Show resolved Hide resolved

vault commented May 11, 2018

View reviewed changes

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/api/BitbucketApiFactory.java Show resolved Hide resolved

vault commented May 11, 2018

View reviewed changes

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/client/BitbucketCloudApiClient.java Outdated Show resolved Hide resolved

vault commented May 11, 2018

View reviewed changes

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/client/BitbucketCloudApiClient.java Show resolved Hide resolved

vault commented May 11, 2018

View reviewed changes

...ain/java/com/cloudbees/jenkins/plugins/bitbucket/server/client/BitbucketServerAPIClient.java Show resolved Hide resolved

vault commented May 11, 2018

View reviewed changes

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/BitbucketSCMNavigator.java Show resolved Hide resolved

vault force-pushed the authentication-token-plugin branch from a9997a4 to bfcbbe0 Compare May 15, 2018 15:34

jetersen requested changes Sep 22, 2018

View reviewed changes

This was referenced Sep 22, 2018

Added JWT support for authentication to bitbucket connect #68

Closed

JENKINS-34262 OAuth implemented, building, skipping tests #69

Closed

vault force-pushed the authentication-token-plugin branch from fd46cae to 0ebfc16 Compare September 26, 2018 16:31

jetersen approved these changes Oct 7, 2018

View reviewed changes

stephenc approved these changes Oct 27, 2018

View reviewed changes

vault added 14 commits October 31, 2018 15:07

Changes from initial feedback:

77c9df7

* Don't just wrap Credentials in BitbucketAuthenticators * Use AuthenticationTokens.matcher * Log a message in exception handler

Fix problems introduced by recent PR

b300b3f

Use AuthenticationTokenContext api

26f86cc

Fix query parameter naming

069ec99

Add some JavaDoc

5643c30

Fix find-bugs complaints

09b5a64

Cleanup whitespace

5378250

Tests for BitbucketAuthenticator

d76b91d

Include server url in context. Improve naming.

a89ec27

Try not to break API compatibility

b78824a

null checks for findbugs

ebd7a81

fix ambiguous constructor calls

91e8b83

add license comment

fa8ec07

vault force-pushed the authentication-token-plugin branch from 0ebfc16 to fa8ec07 Compare October 31, 2018 19:36

jetersen approved these changes Oct 31, 2018

View reviewed changes

jetersen merged commit 85bc804 into jenkinsci:master Oct 31, 2018

kurddt mentioned this pull request Dec 4, 2019

add OAuth authentication #262

Merged

6 tasks


		public abstract class BitbucketAuthenticator<T extends Credentials> {

		public final T credentials;

		import org.kohsuke.stapler.DataBoundConstructor;

		import javax.annotation.Nonnull;

		*/
		public void configureBuilder(HttpClientBuilder builder) { }

		*/
		public void configureContext(HttpClientContext context, HttpHost host) { }

Use AuthenticationTokens #101

Use AuthenticationTokens #101

Conversation

vault commented Jan 3, 2018

jetersen commented Jan 3, 2018

stephenc left a 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

jetersen commented Jan 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vault commented Jan 26, 2018

jetersen left a 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

stephenc left a 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

rsandell commented Apr 24, 2018

vault commented May 11, 2018

vault commented May 15, 2018

michelzanini commented Jul 4, 2018

vault commented Jul 9, 2018

andrewspiers commented Jul 30, 2018

stephenc commented Jul 30, 2018

hoegertn commented Aug 30, 2018

jetersen commented Sep 22, 2018

jetersen left a comment

Choose a reason for hiding this comment

jetersen left a comment • edited

Choose a reason for hiding this comment

jetersen commented Oct 27, 2018

stephenc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jetersen commented Oct 27, 2018

vault commented Oct 31, 2018

jetersen left a comment •

edited