[JENKINS-50216] - Make RemotableGoogleCredentials compatible with JEP-200 in Jenkins 2.102+ #15

oleg-nenashev · 2018-03-20T11:03:38Z

I have just did some facelifting work during the investigation of https://issues.jenkins-ci.org/browse/JENKINS-50216 . This patch enables PCT runs for the recent versions

I have not signed the Google CLA yet though: https://cla.developers.google.com/ , but #16 states that it's not required anymore. I assume that the copyright requirement in Checkstyle is also outdated then.

Facelift the plugin
Add Jenkinsfile
Add custom serializer logic for JodaTime's DateTime class, which replaces the format by a JEP-200-free layout and also adds support of old object deserialization

@reviewbybees @jglick

jglick

No Jenkinsfile?

jglick · 2018-03-20T18:08:54Z

pom.xml

@@ -18,7 +18,7 @@
  <parent>
    <groupId>org.jenkins-ci.plugins</groupId>
    <artifactId>plugin</artifactId>
-    <version>2.5</version>
+    <version>3.5</version>


jglick · 2018-03-20T18:09:10Z

pom.xml

@@ -112,7 +94,7 @@
      <plugin>
        <groupId>org.codehaus.mojo</groupId>
        <artifactId>findbugs-maven-plugin</artifactId>
-        <version>3.0.1</version>
+        <version>3.0.5</version>
        <configuration>
          <effort>Max</effort>
          <threshold>Medium</threshold>


TODO comment

jglick · 2018-03-20T18:09:17Z

pom.xml

@@ -200,7 +183,6 @@
    <dependency>
      <groupId>junit</groupId>
      <artifactId>junit</artifactId>


just delete

oleg-nenashev · 2018-03-20T18:16:39Z

Forgot to submit Jenkinsfile. Will do it tomorrow along with the comments cleanup

oleg-nenashev · 2018-04-02T12:08:45Z

Needs #16 to be merged before we can proceed

…eTime

jglick · 2018-04-16T18:39:23Z

Jenkinsfile

@@ -0,0 +1,2 @@
+// Build the plugin using https://github.com/jenkins-infra/pipeline-library
+buildPlugin(jenkinsVersions: [null, '2.107.2'], failFast: false)


Windows test failures

jglick · 2018-04-16T18:45:18Z

src/main/java/com/google/jenkins/plugins/credentials/oauth/RemotableGoogleCredentials.java

@@ -70,7 +79,7 @@ public RemotableGoogleCredentials(
      throw new GeneralSecurityException(
          Messages.RemotableGoogleCredentials_NoAccessToken(), e);
    }
-    this.accessToken = checkNotNull(credential.getAccessToken());
+    this.accessToken = mock ? "MOCKED" : checkNotNull(credential.getAccessToken());
    this.expiration = new DateTime().plusSeconds(


I would rather just see this field converted to a simple long (msec since epoch), in which case JodaDateTimeConverter would be purely for compatibility with old settings and its marshal could throw UnsupportedOperationException.

oleg-nenashev · 2018-04-17T07:05:03Z

"src/test/java/com/google/jenkins/plugins/credentials/oauth/P12ServiceAccountConfigTestUtil.java:[39,36] package org.bouncycastle.cert.jcajce does not exist" this is the biggest issue for me. Since we are not using PCT (needs jenkins-infra/pipeline-library#35), something is going on with BouncyCastle APIs. Likely I will need to add dependency on https://github.com/jenkinsci/bouncycastle-api-plugin or modify tests a bit

oleg-nenashev · 2018-04-17T07:08:32Z

.../com/google/jenkins/plugins/credentials/oauth/RemotableGoogleCredentialsIntegrationTest.java

@@ -0,0 +1,115 @@
+/*
+ * Copyright 2018 CloudBees, Inc.


@astroilov I would appreciate guidance regarding it. Your current checkstyle configuration requires Google to be a copyright owner. AFAICT it is no longer required after #16, right?

I believe that to be correct, but I'll check with our opensource compliance team just in case.

@astroilov 👍 Note that the current title in Google licenses may also need clarification with this team ("Copyright 2013 Google Inc. All Rights Reserved."). "All Rights Reserved" IMHO contradicts with Apache v.2 license which is used in the plugin, but IANAL

Confirmed that this is fine.

…inst 2.107.2

…ired in the plugin

oleg-nenashev · 2018-04-17T08:34:58Z

@jglick

I would rather just see this field converted to a simple long (msec since epoch), in which case JodaDateTimeConverter would be purely for compatibility with old settings and its marshal could throw UnsupportedOperationException.

Currently it converts to a 1-liner string:

<expiration millis="1523953488567" timezone="Europe/Zurich"/>

IMHO it's good enough. Of course I could convert the field to long in readResolve() after that, but I do not see immediate need to do that as a part of the fix.

…n JEP-200 blacklist

…o need to tweak converter priorities

jglick · 2018-04-17T16:24:18Z

Currently it converts to a 1-liner string

Besides the timezone being irrelevant (AFAICT), this continues to persist a Joda type to XML, which is what I would like to avoid. So I am -0 on this approach. Rather, RemotableGoogleCredentials should persist a simple long, there should be no whitelist addition, and if the expiration field actually needs to be restored from old settings (since this is inherently a time-sensitive datum, it is not clear to me?), then RemotableGoogleCredentials can get a custom ConverterImpl to process the historical XML format.

astroilov · 2018-04-17T22:12:57Z

src/main/java/com/google/jenkins/plugins/credentials/oauth/RemotableGoogleCredentials.java

+          GoogleRobotCredentials credentials,
+          GoogleOAuth2ScopeRequirement requirement,
+          GoogleRobotCredentialsModule module,
+          boolean mock) throws GeneralSecurityException {


I don't like having explicit mock or testing options in the core code; the code should ideally be constructed in such a way as to make this unnecessary, and having mock options in security-related code in particular is concerning.

It appears that the only effect of this is to ignore the access token's expiration and the token string. It seems like that could be achieved by having test code construct its GoogleRobotCredentials object with appropriate expiration and string data. The tests are already using a FakeGoogleCredentials class, so they should be able to control that - is there a reason not to take that approach?

jglick

See #17.

Update Parent POM to the recent version

a878b91

oleg-nenashev requested a review from jglick March 20, 2018 11:03

jglick approved these changes Mar 20, 2018

View reviewed changes

oleg-nenashev added 2 commits April 16, 2018 20:16

[JENKINS-50216] - Introduce a custom serializer for org.joda.time.Dat…

c47f472

…eTime

[JENKINS-50216] - Add Jenkinsfile to verify JEP-200 regressions

5e0448e

oleg-nenashev changed the title ~~Update Parent POM to the recent version~~ [JENKINS-50216] - Make the plugin compatible with JEP-200 in Jenkins 2.102+ Apr 16, 2018

oleg-nenashev changed the title ~~[JENKINS-50216] - Make the plugin compatible with JEP-200 in Jenkins 2.102+~~ [JENKINS-50216] - Make RemotableGoogleCredentials compatible with JEP-200 in Jenkins 2.102+ Apr 16, 2018

oleg-nenashev requested a review from jglick April 16, 2018 18:25

jglick reviewed Apr 16, 2018

View reviewed changes

oleg-nenashev commented Apr 17, 2018

View reviewed changes

oleg-nenashev added 4 commits April 17, 2018 09:34

[JENKINS-50216] - Add BouncyCastle API dependency to enable tests aga…

51fb71a

…inst 2.107.2

Checkstyle: Relax copyright header requirements

902af17

[JENKINS-50216] - Cleanup codestyle and re-enable Checkstyle

bb9c2cc

Plugin POM 3.8: FindBugs and JUnit configurations are not longer requ…

9099cb1

…ired in the plugin

oleg-nenashev added 3 commits April 17, 2018 15:08

[JENKINS-50216] - Joda DateTime converter now has higher priority tha…

dbdef9b

…n JEP-200 blacklist

[JENKINS-50216] - Whitelist org.joda.time.DateTime so that there is n…

498b54b

…o need to tweak converter priorities

Jenkinsfile: Disable Windows tests for now

3267f6d

astroilov reviewed Apr 17, 2018

View reviewed changes

jglick mentioned this pull request Apr 28, 2018

[JENKINS-50216] - RemotableGoogleCredentials.expiration fix #17

Merged

jglick suggested changes Apr 28, 2018

View reviewed changes

astroilov merged commit 3267f6d into jenkinsci:master May 1, 2018

craigdbarber mentioned this pull request Feb 28, 2019

Recover roundtrip tests from PR #15 #32

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-50216] - Make RemotableGoogleCredentials compatible with JEP-200 in Jenkins 2.102+ #15

[JENKINS-50216] - Make RemotableGoogleCredentials compatible with JEP-200 in Jenkins 2.102+ #15

oleg-nenashev commented Mar 20, 2018 •

edited

Loading

jglick left a comment

jglick Mar 20, 2018

jglick Mar 20, 2018

jglick Mar 20, 2018

oleg-nenashev commented Mar 20, 2018

oleg-nenashev commented Apr 2, 2018

jglick Apr 16, 2018

jglick Apr 16, 2018

oleg-nenashev commented Apr 17, 2018

oleg-nenashev Apr 17, 2018

astroilov Apr 17, 2018

oleg-nenashev Apr 17, 2018

astroilov Apr 17, 2018

oleg-nenashev commented Apr 17, 2018

jglick commented Apr 17, 2018

astroilov Apr 17, 2018

jglick left a comment

		@@ -0,0 +1,2 @@
		// Build the plugin using https://github.com/jenkins-infra/pipeline-library
		buildPlugin(jenkinsVersions: [null, '2.107.2'], failFast: false)

[JENKINS-50216] - Make RemotableGoogleCredentials compatible with JEP-200 in Jenkins 2.102+ #15

[JENKINS-50216] - Make RemotableGoogleCredentials compatible with JEP-200 in Jenkins 2.102+ #15

Conversation

oleg-nenashev commented Mar 20, 2018 • edited Loading

jglick 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

oleg-nenashev commented Mar 20, 2018

oleg-nenashev commented Apr 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev commented Apr 17, 2018

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

oleg-nenashev commented Apr 17, 2018

jglick commented Apr 17, 2018

Choose a reason for hiding this comment

jglick left a comment

Choose a reason for hiding this comment

oleg-nenashev commented Mar 20, 2018 •

edited

Loading