Use credentials plugin for managing authentication credentials #103

ccHanibal · 2023-03-23T16:37:06Z

As discussed in #102 this PR makes the plugin use the credentials plugin for managing credentials to confluence sites.
Existing credentials are migrated automatically on jenkins launch.
Also it adds support for credentials in the form of an API token used with Bearer authentication.

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

ccHanibal · 2023-03-23T16:49:20Z

Hoping to get this merged @jetersen, although the build failed again (as with #102), so I can continue to work on the bearer authentication feature. Or should I open a PR that does both at the same time?

jetersen · 2023-03-23T17:23:02Z

I'd suggest adding the token bearer authentication in the same PR to reduce rework.

jetersen · 2023-03-23T17:21:21Z

src/main/java/com/myyearbook/hudson/plugins/confluence/ConfluenceSite.java

     */
    @DataBoundConstructor
-    public ConfluenceSite(URL url, final String username, final String password) {
-        LOGGER.log(Level.FINER, "ctor args: " + url + ", " + username + ", " + password);
+    public ConfluenceSite(URL url, final String credentialsId) {


This breaks binary compatibility.

I suggest you create a new constructor and remove @DataBoundConstructor from this one mark this one deprecated.

So create a new constructor marked with @DataBoundConstructor that takes no arguments and then create a @DataBoundSetter for url and credentialsId

See why here: https://www.jenkins.io/doc/developer/plugin-development/pipeline-integration/#constructor-vs-setters

/** * * * @deprecated use {@link #new()} instead. */ @Deprecated public ConfluenceSite(URL url, final String username, final String password) { if (!url.toExternalForm().endsWith("/")) { try { url = new URL(url.toExternalForm() + "/"); } catch (MalformedURLException e) { throw new AssertionError(e); // impossible } } this.url = url; this.username = hudson.Util.fixEmptyAndTrim(username); this.password = Secret.fromString(password); } @DataBoundConstructor public ConfluenceSite() { } /* ... you have these further below already */ @DataBoundSetter public void setUrl(@CheckForNull URL url) { this.url = Util.fixEmptyAndTrim(url); } @DataBoundSetter public void setUrl(@CheckForNull String credentialsId) { this.credentialsId = Util.fixEmptyAndTrim(credentialsId); }

Did most exactly that.

jetersen · 2023-03-23T17:23:57Z

src/main/java/com/myyearbook/hudson/plugins/confluence/ConfluenceSite.java

+    @Deprecated
+    public String getUsername(){
+        return username;
+    }
+
+    @DataBoundSetter
+    public void setUsername(String username){
+        this.username = Util.fixEmptyAndTrim(username);
+    }
+
+    @Deprecated
+    public Secret getPassword(){
+        return password;
+    }
+
+    @DataBoundSetter
+    public void setPassword(Secret password) {
+        this.password = password;
+    }
+
+    public void setPassword(String password) {
+        this.password = Secret.fromString(password);
+    }
+


I don't think you should add these as they would be considered obsolete and will be hard to remove once added.

Also they definitely should not have @DataBoundSetter when transient.

Suggested change

@Deprecated

public String getUsername(){

return username;

}

@DataBoundSetter

public void setUsername(String username){

this.username = Util.fixEmptyAndTrim(username);

}

@Deprecated

public Secret getPassword(){

return password;

}

@DataBoundSetter

public void setPassword(Secret password) {

this.password = password;

}

public void setPassword(String password) {

this.password = Secret.fromString(password);

}

You are right. I was under the impression those were needed for XML serialisation. But they are not.

jetersen · 2023-03-23T17:32:09Z

src/main/java/com/myyearbook/hudson/plugins/confluence/ConfluenceSite.java

+                            URIRequirementBuilder.create().build()),
+                    CredentialsMatchers.withId(credentialsId));
+            if (credentials != null) {
+                if (credentials instanceof UsernamePasswordCredentials) {


I think the correct type is StandardUsernamePasswordCredentials as it is the interface while UsernamePasswordCredentials is the implementation as far as I recall.

They are both interfaces.

Now using StandardUsernamePasswordCredentials anyway, to be consistent across all places where credentials are retrieved.

* Keep original constructor as depricated * Use StandardUsernamePasswordCredentials

ccHanibal · 2023-03-23T19:21:08Z

I addressed your suggestions. More to come tomorrow.

jetersen · 2023-03-24T17:39:59Z

src/main/java/com/myyearbook/hudson/plugins/confluence/credentials/ConfluenceApiTokenImpl.java

+import hudson.util.Secret;
+import org.kohsuke.stapler.DataBoundConstructor;
+
+public final class ConfluenceApiTokenImpl extends BaseStandardCredentials implements ConfluenceApiToken {


I don't think we need to implement our own credential implementation. I think we should just use StringCredentials 😓 Sorry I did not provide enough context.

Which you can get from depending on this plugin without a version since it is in the jenkins plugin BOM: https://plugins.jenkins.io/plain-credentials/ it is broadly installed.

https://github.com/search?q=plain-credentials+org%3Ajenkinsci&type=code

Hm yeah thought about that.
I got my inspiration from the GitLab plugin which does both, implement its own type and support StringCredentials. And I thought "we don't need both", but apparently I chose the "wrong" one.
Should be easy to change.

For newer implementation I would stick to StringCredentials unless there was a different need for multiple different string typed credentials.

jetersen

One minor issue is the star imports. Once we fix those, this is ready.

jetersen · 2023-03-27T16:06:25Z

src/main/java/com/myyearbook/hudson/plugins/confluence/ConfluenceSite.java

 import com.atlassian.confluence.rest.client.RestClientFactory;
 import com.atlassian.confluence.rest.client.authentication.AuthenticatedWebResourceProvider;
+import com.cloudbees.plugins.credentials.*;


I would ask if you can undo the star imports.

jetersen · 2023-03-27T16:06:32Z

src/main/java/com/myyearbook/hudson/plugins/confluence/ConfluenceSite.java

-import hudson.model.Descriptor;
-import hudson.model.Hudson;
+import hudson.Util;
+import hudson.model.*;


jetersen · 2023-03-29T08:19:14Z

@ccHanibal Did you have a chance to test the HPI and see if the token bearer worked?

https://repo.jenkins-ci.org/incrementals/org/jenkins-ci/plugins/confluence-publisher/154.vef20e6e5fdd9/confluence-publisher-154.vef20e6e5fdd9.hpi

ccHanibal · 2023-03-29T09:49:08Z

@jetersen Tested with my local build and with the v154 hpi. Also tested the automatic migration of username and password after upgrading. All looked good.

ccHanibal · 2023-03-29T10:13:34Z

Oh damn it.
Just noticed that I did not delete the resource file for rendering the custom ConfluenceApiTokenImpl (which has been removed): credentials/ConfluenceApiTokenImpl/credentials.jelly
I think we can just leave it in, but can cause potential irritations in the future.

Use credentials plugin for managing authentication credentials

97cc33b

jetersen requested changes Mar 23, 2023

View reviewed changes

Apply review suggestens

0dc8ba8

* Keep original constructor as depricated * Use StandardUsernamePasswordCredentials

Add support for Bearer Token authentication

0e5a5d3

jetersen requested changes Mar 24, 2023

View reviewed changes

Use StringCredentials instead of custom credentials type

25adb8e

This was referenced Mar 27, 2023

Maven central packages not found on build agent jenkins-infra/helpdesk#3434

Closed

Add support for bearer token authentication #102

Closed

jetersen added the enhancement label Mar 27, 2023

jetersen requested changes Mar 27, 2023

View reviewed changes

No star imports

ef20e6e

jetersen approved these changes Mar 29, 2023

View reviewed changes

jetersen merged commit 38792a2 into jenkinsci:master Mar 29, 2023

ccHanibal deleted the feature/use-credentials-plugin branch March 29, 2023 10:26

ccHanibal added a commit to ccHanibal/confluence-publisher-plugin that referenced this pull request Mar 29, 2023

Remove leftover jelly file from jenkinsci#103

29d07ba

ccHanibal mentioned this pull request Mar 29, 2023

Remove leftover jelly file #105

Merged

6 tasks

jetersen pushed a commit that referenced this pull request Mar 29, 2023

Remove leftover jelly file from #103 (#105)

b6abf4a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use credentials plugin for managing authentication credentials #103

Use credentials plugin for managing authentication credentials #103

ccHanibal commented Mar 23, 2023 •

edited

Loading

ccHanibal commented Mar 23, 2023

jetersen commented Mar 23, 2023

jetersen Mar 23, 2023

ccHanibal Mar 23, 2023

jetersen Mar 23, 2023

ccHanibal Mar 23, 2023

jetersen Mar 23, 2023

ccHanibal Mar 23, 2023

ccHanibal Mar 23, 2023 •

edited

Loading

ccHanibal commented Mar 23, 2023

jetersen Mar 24, 2023

ccHanibal Mar 27, 2023 •

edited

Loading

jetersen Mar 27, 2023

jetersen left a comment •

edited

Loading

jetersen Mar 27, 2023

ccHanibal Mar 27, 2023

jetersen Mar 27, 2023

ccHanibal Mar 27, 2023

jetersen commented Mar 29, 2023

ccHanibal commented Mar 29, 2023 •

edited

Loading

ccHanibal commented Mar 29, 2023

Use credentials plugin for managing authentication credentials #103

Use credentials plugin for managing authentication credentials #103

Conversation

ccHanibal commented Mar 23, 2023 • edited Loading

ccHanibal commented Mar 23, 2023

jetersen commented Mar 23, 2023

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

ccHanibal Mar 23, 2023 • edited Loading

Choose a reason for hiding this comment

ccHanibal commented Mar 23, 2023

Choose a reason for hiding this comment

ccHanibal Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jetersen left a comment • edited Loading

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 Mar 29, 2023

ccHanibal commented Mar 29, 2023 • edited Loading

ccHanibal commented Mar 29, 2023

ccHanibal commented Mar 23, 2023 •

edited

Loading

ccHanibal Mar 23, 2023 •

edited

Loading

ccHanibal Mar 27, 2023 •

edited

Loading

jetersen left a comment •

edited

Loading

ccHanibal commented Mar 29, 2023 •

edited

Loading