JENKINS-45789 Use Credentials Plugin for JIRA Global Configuration User #140

gmshake · 2018-02-01T12:13:39Z

Issue link: https://issues.jenkins-ci.org/browse/JENKINS-45789

TODO:

~~Better document on credentialsId field.~~ move out from this PR
Manually merge ~~Partial fix for JENKINS-48357, reverting commit 420f69076b8606c74cf24f301b9446778d88658f #139~~ JENKINS-48357 Fix by bumping version of Jenkins Apache HttpComponents Client 4.x API Plugin #147 into local repository to test this PR.
~~Rebase to latest master.~~

coveralls · 2018-02-01T12:22:13Z

Coverage increased (+1.08%) to 53.273% when pulling d063bbc on gmshake:JENKINS-45789 into bed1307 on jenkinsci:master.

artkoshelev · 2018-04-20T04:26:08Z

src/main/java/hudson/plugins/jira/JiraSite.java

+
+        // HACK, update final fields via reflection, see jls 17.5.3, https://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.5.3
+        if (StringUtils.isNotBlank(credentialsId)) {
+            StandardUsernamePasswordCredentials credentials = CredentialsHelper.lookupSystemCredentials(credentialsId, url != null ? url.toExternalForm() : null);


could we move url null check inside lookupSystemCredentials?

Good catch!

artkoshelev · 2018-04-20T04:30:19Z

src/test/java/hudson/plugins/jira/JiraSite2Test.java

@@ -14,7 +14,7 @@
    @Test
    public void testClientInitialization() throws Exception {
        JiraSite site = new JiraSite(new URL("https://nonexistent.url"), null,
-                "user", "password",
+                (String)null,


why cast to String?

Emm, I found this change make the test not functional as expected, I'll fix it :(

artkoshelev · 2018-04-20T04:32:54Z

src/main/java/hudson/plugins/jira/JiraSite.java

+                updateJiraIssueForAllStatus, groupVisibility, roleVisibility, useHTTPAuth);
+    }
+
+    @Deprecated


please specify method to use instead of deprecated one and the reason for deprecation

Deprecate the previous constructor but leave it in place for Java-level compatibility. See https://jenkins.io/doc/developer/plugin-development/pipeline-integration/ , section "Constructor vs. setters"

artkoshelev · 2018-04-20T04:36:31Z

src/main/java/hudson/plugins/jira/CredentialsHelper.java

+ *
+ * @author Zhenlei Huang
+ */
+public class CredentialsHelper {


please add tests for this class

OK, I'll do it :)

Tests for CredentialsHelper added. Commit: 0966980

gmshake · 2018-04-20T10:14:07Z

src/main/resources/hudson/plugins/jira/JiraSite/config_de.properties

artkoshelev · 2018-04-22T23:30:58Z

src/main/java/hudson/plugins/jira/CredentialsHelper.java

+		for (StandardUsernamePasswordCredentials c : credentials) {
+			if (StringUtils.equals(password, Secret.toString(c.getPassword()))) {
+				cred = c;
+				break; // found


we could return c here and drop null check below

artkoshelev · 2018-04-22T23:33:43Z

src/main/java/hudson/plugins/jira/JiraSite.java

+        // HACK, update final fields via reflection, see jls 17.5.3, https://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.5.3
+        if (StringUtils.isNotBlank(credentialsId)) {
+            StandardUsernamePasswordCredentials credentials = CredentialsHelper.lookupSystemCredentials(credentialsId, url);
+            setCredentials(credentials);


shouldn't we call setCredentialsId here as well?

There is a condition that credentialsId is not blank but no matched credentials. setCredentialsId should be called under null check with credentials :(

artkoshelev · 2018-04-22T23:35:23Z

src/main/java/hudson/plugins/jira/JiraSite.java

        return this;
    }

+    private void setCredentialsId(String credentialsId) {


i would move this method and one below to CredentialsHelper

gmshake · 2018-04-29T04:40:19Z

Ping @artkoshelev

artkoshelev · 2018-04-24T02:46:42Z

pom.xml

@@ -323,6 +323,12 @@
        </exclusion>
      </exclusions>
    </dependency>
+    <dependency>
+      <groupId>org.powermock</groupId>


why do we need powermock? (we already have mockito)

Testcase DescriptorImplTest#testValidateConnectionError()

jira-plugin/src/test/java/hudson/plugins/jira/DescriptorImplTest.java

Line 46 in 07e3440

FormValidation validation = descriptor.doValidate("user", "http://localhost:8080", "pass", null, null, false, " ", JiraSite.DEFAULT_TIMEOUT);

made false positive IIUC.

@Test public void testValidateConnectionError() throws Exception { when(jiraSession.getMyPermissions()).thenThrow(RestClientException.class); when(jiraSite.createSession()).thenReturn(jiraSession); FormValidation validation = descriptor.doValidate("user", "http://localhost:8080", "pass", null, null, false, " ", JiraSite.DEFAULT_TIMEOUT); assertEquals(validation.kind, FormValidation.Kind.ERROR); }

The condition passed to descriptor.doValidate() indeed test alternativeURL.
Since mockito does not support mock new operations, I imported powermock to correct this test.

How about using builder pattern for JiraSite instead? It would help fixing "million-paramaters-constructor" calls for JiraSite as well as allow properly inject mocked JiraSite implementation in tests.

Builder pattern seems not helpful with mock, mock JiraSite.createSession() in this case. Can you explain more about how to mock with builder pattern? Thanks!

In order to properly inject JiraSite object in DescriptorImpl you might define getJiraSite() method and mock it later, but it would require to pass all the parameters you have for JiraSite constructor. Instead, you define getJiraSiteBuilder() returning just new builder, then you configure builder in your impl code and call build() to get actual JiraSite constructed. Later in tests, you mock getJiraSiteBuilder() to return mocked/spy'ed builder, which would return mocked site on build() call.

I'm not sure whether I implement the builder pattern right or not. It looks ugly 👎
I'll include this if it satisfies. See https://github.com/gmshake/jira-plugin/pull/1/files

artkoshelev · 2018-04-24T02:49:13Z

src/main/java/hudson/plugins/jira/CredentialsHelper.java

+		SystemCredentialsProvider.getInstance().getCredentials().add(newCredentials);
+		try {
+			SystemCredentialsProvider.getInstance().save();
+			LOGGER.log(Level.INFO, "Migrated credentials");


I would prefer to log something more meaningful, e.g. Provided username and password were successfully migrated and stored as {credentialsId}

Good catch:)

artkoshelev · 2018-04-24T02:54:34Z

src/main/java/hudson/plugins/jira/CredentialsHelper.java

+
+	public static void setCredentialsId(JiraSite site, String credentialsId) {
+		try {
+			Field f = site.getClass().getDeclaredField("credentialsId");


why do we need to use reflection?

JiraSite is designed to use final field credentialsId and credentials, during the progress of deserialization, the instance of JiraSite is created before JiraSite#readResolve() is called,

jira-plugin/src/main/java/hudson/plugins/jira/JiraSite.java

Line 253 in 07e3440

protected Object readResolve() {

hence we need use reflection to update relevant fields.
There's another solution, that return a new constructed JiraSite with all need value from the temp deserialization instance from with JiraSite#readResolve(), what do you think?

gmshake · 2018-05-01T09:36:00Z

@artkoshelev Thanks very much for your help with this PR, I did not notice there is an acceptance test for this plugin at the time this PR created, https://github.com/jenkinsci/acceptance-test-harness/blob/master/src/test/java/plugins/JiraPluginTest.java
Do I need to update the test, since this PR introduced incompatible UI change ? Then there would be another issue that new version of Jira plugin will break the acceptance test before relevant tests updated. Any suggestion?

artkoshelev · 2018-05-02T01:54:30Z

I'm not sure how it handled, better to ask someone supporting acceptance-test-harness. my guess would be:

made PR with fix for acceptance tests, wait for builds to be red
merge this change, release plugin
re-run build on PR above, merge PR for acceptance tests

tonivdv · 2018-06-01T09:41:23Z

Any news on this? Need help?

gmshake · 2018-06-01T16:44:33Z

@tonivdv Sorry but I was busy, I'm planning to get this feature done within next few days :)

gmshake · 2018-06-03T12:17:58Z

Rebased to latest master, and resolved conflicts.

gmshake · 2018-06-04T09:33:16Z

It's weird that the coverage decreased :(

Related to powermock / jacoco compatibility, see https://github.com/powermock/powermock/wiki/Code-coverage-with-JaCoCo#on-the-fly-instrumentation

gmshake · 2018-06-05T15:21:52Z

Ping @artkoshelev

artkoshelev · 2018-06-07T00:21:35Z

@gmshake sorry, won't be able to review it properly till next week

gmshake · 2018-06-17T04:06:24Z

Rebased to master and resolved conflicts.

gmshake · 2018-07-02T01:47:25Z

Ping @artkoshelev

artkoshelev · 2018-07-02T04:17:23Z

looking, sorry for the delay

samuelmasuy · 2018-07-04T23:04:33Z

Really looking forward this merge! Any news on this?

artkoshelev · 2018-07-02T06:22:28Z

pom.xml

@@ -323,6 +323,12 @@
        </exclusion>
      </exclusions>
    </dependency>
+    <dependency>
+      <groupId>org.powermock</groupId>


How about using builder pattern for JiraSite instead? It would help fixing "million-paramaters-constructor" calls for JiraSite as well as allow properly inject mocked JiraSite implementation in tests.

artkoshelev · 2018-07-02T06:30:01Z

src/main/java/hudson/plugins/jira/JiraSite.java

@@ -251,10 +289,12 @@ public boolean isAppendChangeTimestamp() {
    }

    protected Object readResolve() {


this method is not used as far as i can see, let's delete it

We need readResolve() for backward compatibility, and it is required during deserialization, see https://wiki.jenkins.io/display/JENKINS/Hint+on+retaining+backward+compatibility

Could you please put "ignore unused" annotation with this link?

artkoshelev · 2018-07-02T06:42:02Z

src/main/java/hudson/plugins/jira/CredentialsHelper.java

+	private static final Logger LOGGER = Logger.getLogger(CredentialsHelper.class.getName());
+
+	@CheckForNull
+	public static StandardUsernamePasswordCredentials lookupSystemCredentials(@CheckForNull String credentialsId, @CheckForNull URL url) {


Static methods are usually anti-pattern: you couldn't mock them when you test dependent components. Let's implement it as a normal component, say CredentialsManager?

As the logic is simple enough, only integration tests are written for it. I'm not seeing any value writing unit test here. What do you think?

artkoshelev · 2018-07-02T06:44:43Z

src/main/resources/hudson/plugins/jira/JiraSite/config.jelly

@@ -1,5 +1,5 @@
 <?jelly escape-by-default='true'?>


What would be the user experience when plugin with this changes installed? Would existing username/password be lost?

Existing username/password will migrated to credentials and be stored. The config will not be touched anyway, i.e. require end user re-config jira-plugin global settings. It will be better that no re-config required. I'm struggling on this yet. I'm not sure how to trigger jenkins to auto save jira-plugin settings.

@warden @olamy @oleg-nenashev any ideas on this?

artkoshelev · 2018-07-02T06:48:41Z

src/test/java/hudson/plugins/jira/DescriptorImplTest.java

+        as.grant(Item.READ).onItems(dummy).to("alice");
+        r.jenkins.setAuthorizationStrategy(as);
+
+        try (ACLContext context = ACL.as(User.get("admin"))) {


IDEA suggests to rename unused variable to ignored

artkoshelev · 2018-07-02T06:49:50Z

src/test/java/hudson/plugins/jira/DescriptorImplTest.java

-    JiraSite jiraSite = mock(JiraSite.class);
-
-    JiraSession jiraSession = mock(JiraSession.class);
-
    @Rule
    public final ExpectedException exception = ExpectedException.none();


not needed, since we don't expect any exceptions anyway

artkoshelev · 2018-07-05T00:05:11Z

@samuelmasuy once conflicts fixed and PR comments addressed, i'm happy to merge it

gmshake · 2018-07-05T03:18:27Z

Rebased to latest master and resolved conflicts. Polished as @artkoshelev suggested.

…Impl

gmshake · 2018-07-09T01:03:26Z

Thanks @artkoshelev :)

tonivdv · 2018-07-09T07:24:16Z

Thanks. Will you release a new version soon with this improvement?

artkoshelev · 2018-07-10T06:59:52Z

@tonivdv please ask @olamy or @warden to release new version =)

artkoshelev requested changes Apr 20, 2018

View reviewed changes

gmshake commented Apr 20, 2018

View reviewed changes

artkoshelev reviewed Apr 22, 2018

View reviewed changes

artkoshelev reviewed Apr 30, 2018

View reviewed changes

gmshake mentioned this pull request May 2, 2018

JENKINS-48357 Fix by bumping version of Jenkins Apache HttpComponents Client 4.x API Plugin #147

Closed

gmshake force-pushed the JENKINS-45789 branch from 7bad8e4 to e2dd20e Compare June 3, 2018 12:16

gmshake force-pushed the JENKINS-45789 branch from d0a504e to 51bb028 Compare June 17, 2018 04:05

artkoshelev requested changes Jul 4, 2018

View reviewed changes

gmshake added 7 commits July 5, 2018 09:26

JENKINS-45789 Use Credentials Plugin for JIRA Configuration

915ed5a

JENKINS-45789 Restrict credentials domain

853392f

JENKINS-45789 Add unit test

76ceb61

JENKINS-45789 Minor refactor

38b90c4

JENKINS-45789 Auto migrate credentials

a08242f

JENKINS-45789 Minor refactor

c598984

JENKINS-45789 Fix testcase JiraSite2Test

492192b

gmshake added 9 commits July 5, 2018 09:41

JENKINS-45789 Add tests for CredentialsHelper

d3f44d0

JENKINS-45789 Fix German translation

c69f4fe

JENKINS-45789 Minor refactor

0d68768

JENKINS-45789 More meaningful log

3f0f74e

JENKINS-45789 Minor refactor

295145f

JENKINS-45789 Split DescriptorImplTest to restore JaCoCo code coverage

ba0ee91

JENKINS-45789 Use correct permission to retrieve credentials

bdea096

JENKINS-45789 Add more tests for JiraSite.DescriptorImpl

fc23513

JENKINS-45789 Polish

bd5f6ee

gmshake force-pushed the JENKINS-45789 branch from 51bb028 to bd5f6ee Compare July 5, 2018 03:16

JENKINS-45789 Use builder pattern to help testing JiraSite.Descriptor…

d063bbc

…Impl

artkoshelev approved these changes Jul 8, 2018

View reviewed changes

artkoshelev merged commit 0beb22c into jenkinsci:master Jul 8, 2018

		@@ -251,10 +289,12 @@ public boolean isAppendChangeTimestamp() {
		}

		protected Object readResolve() {

JENKINS-45789 Use Credentials Plugin for JIRA Global Configuration User #140

JENKINS-45789 Use Credentials Plugin for JIRA Global Configuration User #140

Conversation

gmshake commented Feb 1, 2018 • edited

coveralls commented Feb 1, 2018 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmshake Apr 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmshake commented Apr 29, 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

Choose a reason for hiding this comment

gmshake Jul 5, 2018 • edited

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

gmshake commented May 1, 2018

artkoshelev commented May 2, 2018

tonivdv commented Jun 1, 2018

gmshake commented Jun 1, 2018

gmshake commented Jun 3, 2018

gmshake commented Jun 4, 2018 • edited

gmshake commented Jun 5, 2018

artkoshelev commented Jun 7, 2018

gmshake commented Jun 17, 2018

gmshake commented Jul 2, 2018

artkoshelev commented Jul 2, 2018

samuelmasuy commented Jul 4, 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

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

artkoshelev commented Jul 5, 2018

gmshake commented Jul 5, 2018

gmshake commented Jul 9, 2018

tonivdv commented Jul 9, 2018

artkoshelev commented Jul 10, 2018

gmshake commented Feb 1, 2018 •

edited

coveralls commented Feb 1, 2018 •

edited

gmshake Apr 23, 2018 •

edited

gmshake Jul 5, 2018 •

edited

gmshake commented Jun 4, 2018 •

edited