Fix folder credential usage #541

sirstrahd · 2019-03-15T11:26:36Z

This allows for folder credentials to be used, additionally to the current global ones. Tested it with both freestyle and pipeline (workflow) jobs, as well as the freestyle configuration dialog.
Should address #500 .

timja · 2019-03-15T11:34:00Z

src/main/java/jenkins/plugins/slack/StandardSlackService.java


-    public StandardSlackService(String baseUrl, String teamDomain, String authTokenCredentialId, boolean botUser, String roomId) {
-        this(baseUrl, teamDomain, null, authTokenCredentialId, botUser, roomId, false);
+    public StandardSlackService(String baseUrl, String teamDomain, String authTokenCredentialId, boolean botUser, String roomId, Item item) {


We can't modify existing constructors as this is part of the public API unless we are sure they aren't in use, normally you would deprecate first

I don't think we should be passing the Item down to this level, I haven't looked closely enough at the API but I would like to see a new constructor that passes a token that is guaranteed to be not null.
The legacy constructors will still need to be able to find a credential but that will be deprecated and removed at a later date

Hopefully I addressed the 2 issues. I deprecated the other constructors, but maybe it's too early for that?

jetersen

Just some suggestion, when reviewing the code

jetersen · 2019-03-15T16:22:09Z

src/main/java/jenkins/plugins/slack/CredentialsObtainer.java

+public class CredentialsObtainer {
+
+    public static StringCredentials lookupCredentials(String credentialId, Item item) {
+        List<StringCredentials> credentials = com.cloudbees.plugins.credentials.CredentialsProvider.lookupCredentials(StringCredentials.class, item, ACL.SYSTEM, Collections.emptyList());


Please import, 187 characters hurts readability 😄

jetersen · 2019-03-15T16:24:59Z

src/main/java/jenkins/plugins/slack/SlackNotifier.java

@@ -428,7 +430,14 @@ public SlackService newSlackService(AbstractBuild r, BuildListener listener) {
        authToken = env.expand(authToken);
        authTokenCredentialId = env.expand(authTokenCredentialId);
        room = env.expand(room);
-        return new StandardSlackService(baseUrl, teamDomain, authToken, authTokenCredentialId, botUser, room, false, r.getProject());
+        final String populatedToken = CredentialsObtainer.getTokenToUse(authTokenCredentialId, r.getProject(), authToken);


r.getParent() is more correct but alas this is an AbstractBuild why is this named r 😭
Would you mind renaming the variable?

jetersen · 2019-03-15T16:26:03Z

src/main/java/jenkins/plugins/slack/StandardSlackService.java

@@ -45,41 +45,70 @@
    private String host = "slack.com";
    private String baseUrl;
    private String teamDomain;
-    private String token;
-    private String authTokenCredentialId;
+    private String token = null;


null is default 😕

Removed all useless null initializations.

timja · 2019-03-15T16:28:51Z

src/main/java/jenkins/plugins/slack/CredentialsObtainer.java

+        if (credentials != null) {
+            response = credentials.getSecret().getPlainText();
+        } else {
+            response = token;


can this throw an exception if token is null please?
We get lots of issues falling through to the bottom because token and credential are both null

same on line 29

All code paths on StandardSlackService should now throw an IllegalArgumentException if token is null.

timja · 2019-03-15T16:30:52Z

src/main/java/jenkins/plugins/slack/SlackNotifier.java

+        if (populatedToken != null) {
+            return new StandardSlackService(baseUrl, teamDomain, botUser, room, false, populatedToken);
+        } else {
+            logger.log(Level.INFO, "No explicit token specified and could not obtain credentials for id: " + authTokenCredentialId);


this should throw an exception depending, or at least log a message to the build log as well
It shouldn't return a slack service as its not configured correctly

Since now getTokenToUse throws an exception (as requested in another comment) this code is now unreachable so I'm removing it.

... however, this will cause current misconfigured jobs to crash on notification calls, is that desirable? Should I surround the calls to slackFactory (which ends up calling this) with try catch blocks?

Yes given that the existing code does this we should keep doing it for now.

slack-plugin/src/main/java/jenkins/plugins/slack/StandardSlackService.java

Line 178 in b3a9261

} catch (Exception e) {

src/main/java/jenkins/plugins/slack/SlackNotifier.java

src/main/java/jenkins/plugins/slack/StandardSlackService.java

timja · 2019-03-15T16:36:03Z

src/main/java/jenkins/plugins/slack/StandardSlackService.java


+            logger.fine("Using Integration Token.");


this doesn't throw an exception if its not found,
Could this be moved to the legacy constructors instead?
and add the throwing of the exception?

timja · 2019-03-15T16:37:34Z

src/main/java/jenkins/plugins/slack/workflow/SlackSendStep.java

+            final String populatedToken = CredentialsObtainer.getTokenToUse(tokenCredentialId, item, token);
+            if (populatedToken == null) {
+                listener.error(Messages
+                        .NotificationFailedWithException(new IllegalArgumentException("the token with the provided ID could not be found and no token was specified")));


Suggested change

.NotificationFailedWithException(new IllegalArgumentException("the token with the provided ID could not be found and no token was specified")));

.NotificationFailedWithException(new IllegalArgumentException(String.format("The credential with ID %s could not be found and no token was specified", tokenCredentialId))));

timja · 2019-03-15T16:38:54Z

src/main/java/jenkins/plugins/slack/StandardSlackService.java

        this.botUser = botUser;
        this.roomIds = roomId.split("[,; ]+");
        this.replyBroadcast = replyBroadcast;
    }

+


remove extra line break?

timja · 2019-03-15T16:39:41Z

src/main/java/jenkins/plugins/slack/StandardSlackService.java

+    /**
+     * @deprecated use {@link #StandardSlackService(String, String, boolean, String, boolean, String)} instead}
+     */
+    @Deprecated


I'll check this when I test the code out, but can you make sure that none of these constructors are called anymore internally to the codebase?

just had a look, StandardSlackServiceTest are using the old constructors, could you update that please?
and any other places

They're only used from tests. I'll adapt them.

Update IllegalArgumentException message Co-Authored-By: sirstrahd <sirstrahd@gmail.com>

timja · 2019-03-15T19:02:39Z

src/main/java/jenkins/plugins/slack/StandardSlackService.java

-            StringCredentials credentials = lookupCredentials(authTokenCredentialId);
+    private String getTokenToUse(String authTokenCredentialId, String token) {
+        if (!StringUtils.isEmpty(authTokenCredentialId)) {
+            StringCredentials credentials = CredentialsObtainer.lookupCredentials(authTokenCredentialId, null);


can this have an overloaded method please so we don't have to pass null around?

Suggested change

StringCredentials credentials = CredentialsObtainer.lookupCredentials(authTokenCredentialId, null);

StringCredentials credentials = CredentialsObtainer.lookupCredentials(authTokenCredentialId);

timja · 2019-03-15T19:03:01Z

src/main/java/jenkins/plugins/slack/workflow/SlackSendStep.java

@@ -59,6 +64,7 @@
    private boolean replyBroadcast;


+


revert?

Suggested change

timja · 2019-03-15T19:04:01Z

src/main/java/jenkins/plugins/slack/workflow/SlackSendStep.java

-        	else {
-        		jsonString = JsonOutput.toJson(step.attachments);
-        	}
+            final String jsonString;


what changed in this block? tabs to spaces or something? can't tell from github

it seems something got merged into master that had a mix of tabs and spaces and my IDE "fixed" it...

timja

Thanks for this, I'll do some manual testing and merge this within the next few days

timja · 2019-03-16T18:40:38Z

Just done some testing,
Found one issue:

Pipeline syntax can't find credentials in a folder currently
Url something like:
http://127.0.0.1:8070/jenkins/job/folder/job/folder-pipeline/pipeline-syntax/

The project is null here:

slack-plugin/src/main/java/jenkins/plugins/slack/workflow/SlackSendStep.java

Line 197 in 4e071dc

    
           public ListBoxModel doFillTokenCredentialIdItems(@AncestorInPath Project project) {

timja · 2019-03-16T18:53:54Z

I've pushed a commit to fix it

jsoref · 2019-06-10T23:08:24Z

Fwiw, whatever you did here resulted in totally unhelpful debugging.

The global config UI has a nice test button, and that apparently is happy w/ system scoped credentials, but the pipeline editor ui isn't happy w/ system scoped credentials.

This, amongst other things, resulted in significant confusion and pain when debugging.

timja · 2019-06-11T06:00:57Z

Unfortunately that's how jenkins works =/
The credentials are scoped to the nearest Item or if none then the Jenkins system scope.
I'm not sure if there's a way to stop system credentials working in the global test configuration

Edit: I guess you can probably check the scope in the validate function

Marc Salles added 2 commits March 15, 2019 11:36

Fixes folder credentials not working for Slack

d35f206

Adapt code so pipelines work too

8d05af3

timja requested changes Mar 15, 2019

View reviewed changes

Marc Salles added 2 commits March 15, 2019 13:13

undoing modification of previous constructors for StandardSlackService

1c8d88e

modified service constructor so it takes a String instead of an Item

7d3c7e9

jetersen reviewed Mar 15, 2019

View reviewed changes

timja reviewed Mar 15, 2019

View reviewed changes

src/main/java/jenkins/plugins/slack/SlackNotifier.java Show resolved Hide resolved

timja reviewed Mar 15, 2019

View reviewed changes

src/main/java/jenkins/plugins/slack/StandardSlackService.java Outdated Show resolved Hide resolved

timja reviewed Mar 15, 2019

View reviewed changes

timja and others added 2 commits March 15, 2019 18:28

Update src/main/java/jenkins/plugins/slack/StandardSlackService.java

a01335b

Update IllegalArgumentException message Co-Authored-By: sirstrahd <sirstrahd@gmail.com>

addresses PR comments

94811cd

timja reviewed Mar 15, 2019

View reviewed changes

sirstrahd added 2 commits March 15, 2019 20:21

fix additional PR comments

4e071dc

Address PR comments + exception handling for factory calls

8217e7c

timja approved these changes Mar 15, 2019

View reviewed changes

Rerun CI

2edbd9d

Fix pipeline syntax not retrieving folder creds

c63a66c

timja merged commit 966a3a1 into jenkinsci:master Mar 16, 2019

This was referenced Mar 16, 2019

NPE in 2.18 (Jenkins ver. 2.161) #528

Closed

No logs: "ERROR: Slack notification failed. See Jenkins logs for details." #534

Closed

Credential ID lookup does not include credentials in Cloudbees folders #500

Closed

timja mentioned this pull request Mar 16, 2019

Validate token isn't null before sending to slack #527

Closed

tracom1 mentioned this pull request Mar 26, 2019

Slack-API Notifications no longer working #545

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix folder credential usage #541

Fix folder credential usage #541

sirstrahd commented Mar 15, 2019

timja Mar 15, 2019

sirstrahd Mar 15, 2019

jetersen left a comment

jetersen Mar 15, 2019

jetersen Mar 15, 2019

jetersen Mar 15, 2019

sirstrahd Mar 15, 2019

timja Mar 15, 2019

sirstrahd Mar 15, 2019

timja Mar 15, 2019

sirstrahd Mar 15, 2019 •

edited

Loading

sirstrahd Mar 15, 2019

timja Mar 15, 2019

timja Mar 15, 2019

timja Mar 15, 2019

timja Mar 15, 2019

timja Mar 15, 2019

timja Mar 15, 2019

sirstrahd Mar 15, 2019 •

edited

Loading

timja Mar 15, 2019

timja Mar 15, 2019

timja Mar 15, 2019

sirstrahd Mar 15, 2019

timja left a comment

timja commented Mar 16, 2019 •

edited

Loading

timja commented Mar 16, 2019

jsoref commented Jun 10, 2019

timja commented Jun 11, 2019 •

edited

Loading

	.NotificationFailedWithException(new IllegalArgumentException("the token with the provided ID could not be found and no token was specified")));
	.NotificationFailedWithException(new IllegalArgumentException(String.format("The credential with ID %s could not be found and no token was specified", tokenCredentialId))));

	StringCredentials credentials = CredentialsObtainer.lookupCredentials(authTokenCredentialId, null);
	StringCredentials credentials = CredentialsObtainer.lookupCredentials(authTokenCredentialId);

Fix folder credential usage #541

Fix folder credential usage #541

Conversation

sirstrahd commented Mar 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

sirstrahd Mar 15, 2019 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirstrahd Mar 15, 2019 • 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

timja left a comment

Choose a reason for hiding this comment

timja commented Mar 16, 2019 • edited Loading

timja commented Mar 16, 2019

jsoref commented Jun 10, 2019

timja commented Jun 11, 2019 • edited Loading

sirstrahd Mar 15, 2019 •

edited

Loading

sirstrahd Mar 15, 2019 •

edited

Loading

timja commented Mar 16, 2019 •

edited

Loading

timja commented Jun 11, 2019 •

edited

Loading