Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix folder credential usage #541

Merged
merged 10 commits into from
Mar 16, 2019
41 changes: 41 additions & 0 deletions src/main/java/jenkins/plugins/slack/CredentialsObtainer.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package jenkins.plugins.slack;

import com.cloudbees.plugins.credentials.CredentialsMatcher;
import com.cloudbees.plugins.credentials.CredentialsMatchers;
import hudson.model.Item;
import hudson.security.ACL;
import org.apache.commons.lang.StringUtils;
import org.jenkinsci.plugins.plaincredentials.StringCredentials;

import java.util.Collections;
import java.util.List;

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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please import, 187 characters hurts readability 😄

CredentialsMatcher matcher = CredentialsMatchers.withId(credentialId);
return CredentialsMatchers.firstOrNull(credentials, matcher);
}

/**
* Attempts to obtain the credential with the providedId from the item's credential context, otherwise returns token
* @param credentialId the id from the credential to be used
* @param item the item with the context to obtain the credential from.
* @param token the fallback token
* @return the obtained token
*/
public static String getTokenToUse(String credentialId, Item item, String token) {
if (StringUtils.isEmpty(credentialId)) {
return token;
}
StringCredentials credentials = lookupCredentials(StringUtils.trim(credentialId), item);
final String response;
if (credentials != null) {
response = credentials.getSecret().getPlainText();
} else {
response = token;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
return response;
}
}
18 changes: 16 additions & 2 deletions src/main/java/jenkins/plugins/slack/SlackNotifier.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package jenkins.plugins.slack;

import com.cloudbees.plugins.credentials.Credentials;
import com.cloudbees.plugins.credentials.common.StandardListBoxModel;
import com.cloudbees.plugins.credentials.domains.HostnameRequirement;
import hudson.EnvVars;
Expand Down Expand Up @@ -34,6 +35,7 @@
import org.kohsuke.stapler.QueryParameter;
import org.kohsuke.stapler.StaplerRequest;

import java.util.NoSuchElementException;
import java.util.function.Function;
import java.util.logging.Level;
import java.util.logging.Logger;
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@sirstrahd sirstrahd Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... 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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return new StandardSlackService(baseUrl, teamDomain, authToken, authTokenCredentialId, botUser, room, false);
}


}

Expand Down Expand Up @@ -606,7 +615,12 @@ public boolean configure(StaplerRequest req, JSONObject formData) {
}

SlackService getSlackService(final String baseUrl, final String teamDomain, final String authTokenCredentialId, final boolean botUser, final String roomId, final Item item) {
timja marked this conversation as resolved.
Show resolved Hide resolved
return new StandardSlackService(baseUrl, teamDomain, null, authTokenCredentialId, botUser, roomId, false, item);
final String populatedToken = CredentialsObtainer.getTokenToUse(authTokenCredentialId, item,null );
if (populatedToken != null) {
return new StandardSlackService(baseUrl, teamDomain, botUser, roomId, false, populatedToken);
} else {
throw new NoSuchElementException("Could not obtain credentials with credential id: " + authTokenCredentialId);
}
}

@Nonnull
Expand Down
74 changes: 50 additions & 24 deletions src/main/java/jenkins/plugins/slack/StandardSlackService.java
Original file line number Diff line number Diff line change
Expand Up @@ -45,41 +45,70 @@ public class StandardSlackService implements SlackService {
private String host = "slack.com";
private String baseUrl;
private String teamDomain;
private String token;
private String authTokenCredentialId;
private String token = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null is default 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed all useless null initializations.

private String authTokenCredentialId = null;
private boolean botUser;
private String[] roomIds;
private boolean replyBroadcast;
private String responseString = null;
private Item item;
private String populatedToken = null;

/**
* @deprecated use {@link #StandardSlackService(String, String, boolean, String, boolean, String)} instead}
*/
@Deprecated
public StandardSlackService(String baseUrl, String teamDomain, String authTokenCredentialId, boolean botUser, String roomId) {
this(baseUrl, teamDomain, null, authTokenCredentialId, botUser, roomId, false, null);
this(baseUrl, teamDomain, null, authTokenCredentialId, botUser, roomId, false);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@sirstrahd sirstrahd Mar 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

public StandardSlackService(String baseUrl, String teamDomain, String token, String authTokenCredentialId, boolean botUser, String roomId) {
this(baseUrl, teamDomain, token, authTokenCredentialId, botUser, roomId, false, null);
this(baseUrl, teamDomain, token, authTokenCredentialId, botUser, roomId, false);
}

/**
* @deprecated use {@link #StandardSlackService(String, String, boolean, String, boolean, String)} instead}
*/
@Deprecated
public StandardSlackService(String baseUrl, String teamDomain, String token, String authTokenCredentialId, boolean botUser, String roomId, boolean replyBroadcast) {
this(baseUrl, teamDomain, token, authTokenCredentialId, botUser, roomId, replyBroadcast, null);
this(baseUrl, teamDomain, botUser, roomId, replyBroadcast);
this.token = token;
this.authTokenCredentialId = StringUtils.trim(authTokenCredentialId);
}

/**
* @param baseUrl the full url to use, this is an alternative to specifying teamDomain
* @param teamDomain the teamDomain inside slack.com to use
* @param botUser
* @param roomId a semicolon separated list of rooms to notify
* @param replyBroadcast
* @param populatedToken a non-null token to use for authentication
*/
public StandardSlackService(String baseUrl, String teamDomain, boolean botUser, String roomId, boolean replyBroadcast, String populatedToken) {
this(baseUrl, teamDomain, botUser, roomId, replyBroadcast);
if (populatedToken == null) {
throw new IllegalArgumentException("populatedToken cannot be null");
sirstrahd marked this conversation as resolved.
Show resolved Hide resolved
}
this.populatedToken = populatedToken;
}

public StandardSlackService(String baseUrl, String teamDomain, String token, String authTokenCredentialId, boolean botUser, String roomId, boolean replyBroadcast, Item item) {
private StandardSlackService(String baseUrl, String teamDomain, boolean botUser, String roomId, boolean replyBroadcast) {
super();
this.baseUrl = baseUrl;
if(this.baseUrl != null && !this.baseUrl.isEmpty() && !this.baseUrl.endsWith("/")) {
this.baseUrl += "/";
}
this.teamDomain = teamDomain;
this.token = token;
this.authTokenCredentialId = StringUtils.trim(authTokenCredentialId);
this.botUser = botUser;
this.roomIds = roomId.split("[,; ]+");
this.replyBroadcast = replyBroadcast;
this.item = item;
}


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove extra line break?


public String getResponseString() {
return responseString;
}
Expand Down Expand Up @@ -193,25 +222,22 @@ public boolean publish(String message, JSONArray attachments, String color) {
}

private String getTokenToUse() {
if (authTokenCredentialId != null && !authTokenCredentialId.isEmpty()) {
StringCredentials credentials = lookupCredentials(authTokenCredentialId);
if (credentials != null) {
logger.fine("Using Integration Token Credential ID.");
return credentials.getSecret().getPlainText();
if (populatedToken != null) {
return populatedToken;
} else {
if (!StringUtils.isEmpty(authTokenCredentialId)) {
StringCredentials credentials = CredentialsObtainer.lookupCredentials(authTokenCredentialId, null);
if (credentials != null) {
logger.fine("Using Integration Token Credential ID.");
return credentials.getSecret().getPlainText();
}
}
}

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

logger.fine("Using Integration Token.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
return token;
}

private StringCredentials lookupCredentials(String credentialId) {
List<StringCredentials> credentials = com.cloudbees.plugins.credentials.CredentialsProvider.lookupCredentials(StringCredentials.class, item, ACL.SYSTEM, Collections.emptyList());
CredentialsMatcher matcher = CredentialsMatchers.withId(credentialId);
return CredentialsMatchers.firstOrNull(credentials, matcher);
}

protected CloseableHttpClient getHttpClient() {
final HttpClientBuilder clientBuilder = HttpClients.custom();
final CredentialsProvider credentialsProvider = new BasicCredentialsProvider();
Expand Down
16 changes: 11 additions & 5 deletions src/main/java/jenkins/plugins/slack/workflow/SlackSendStep.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import hudson.util.FormValidation;
import hudson.util.ListBoxModel;
import jenkins.model.Jenkins;
import jenkins.plugins.slack.CredentialsObtainer;
import jenkins.plugins.slack.Messages;
import jenkins.plugins.slack.SlackNotifier;
import jenkins.plugins.slack.SlackService;
Expand Down Expand Up @@ -63,6 +64,7 @@ public class SlackSendStep extends Step {
private boolean replyBroadcast;



Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revert?

Suggested change

@Nonnull
public String getMessage() {
return message;
Expand Down Expand Up @@ -255,10 +257,14 @@ protected SlackResponse run() throws Exception {
defaultIfEmpty(baseUrl), defaultIfEmpty(teamDomain), channel, defaultIfEmpty(color), botUser,
defaultIfEmpty(tokenCredentialId))
);

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")));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))));

return null;
}
SlackService slackService = getSlackService(
baseUrl, teamDomain, token, tokenCredentialId, botUser, channel, step.replyBroadcast, item
);
baseUrl, teamDomain, botUser, channel, step.replyBroadcast, populatedToken);
final boolean publishSuccess;
if (step.attachments != null) {
JSONArray jsonArray = getAttachmentsAsJSONArray();
Expand Down Expand Up @@ -357,8 +363,8 @@ private String defaultIfEmpty(String value) {
}

//streamline unit testing
SlackService getSlackService(String baseUrl, String team, String token, String tokenCredentialId, boolean botUser, String channel, boolean replyBroadcast, Item item) {
return new StandardSlackService(baseUrl, team, token, tokenCredentialId, botUser, channel, replyBroadcast, item);
SlackService getSlackService(String baseUrl, String team, boolean botUser, String channel, boolean replyBroadcast, String populatedToken) {
return new StandardSlackService(baseUrl, team, botUser, channel, replyBroadcast, populatedToken);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import com.cloudbees.plugins.credentials.CredentialsProvider;
import hudson.ExtensionList;
import hudson.model.*;
import hudson.security.ACL;
import hudson.util.Secret;
import jenkins.model.Jenkins;
Expand All @@ -21,7 +20,6 @@
import java.util.Collections;
import java.util.List;

import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.powermock.api.mockito.PowerMockito.mock;
import static org.powermock.api.mockito.PowerMockito.when;
Expand All @@ -38,24 +36,22 @@ public void setUp() {
}

@Test
public void providedJobCredentialsAreUsed() {
final String id = "cred-id";
final String secretText = "secret-text";
final Job job = createJobWithAvailableCredentialId(id, secretText);
final StandardSlackServiceStub service = new StandardSlackServiceStub("", "domain", null, id, true, "#room1:1528317530", job);
public void populatedTokenIsUsed() {
final String populatedToken = "secret-text";
final StandardSlackServiceStub service = new StandardSlackServiceStub("", "domain", true, "#room1:1528317530", populatedToken);
final CloseableHttpClientStub httpClientStub = new CloseableHttpClientStub();
httpClientStub.setHttpStatus(HttpStatus.SC_OK);
service.setHttpClient(httpClientStub);
service.publish("message");
assertTrue(httpClientStub.getLastRequest().getURI().toString().contains(secretText));
assertTrue(httpClientStub.getLastRequest().getURI().toString().contains(populatedToken));
}

@Test
public void globalCredentialsCanBeUsed() {
public void globalCredentialByIdUsed() {
final String id = "cred-2id";
final String secretText = "secret-2text";
setupGlobalAvailableCredentialId(id, secretText);
final StandardSlackServiceStub service = new StandardSlackServiceStub("", "domain", null, id, true, "#room1:1528317530", null);
final StandardSlackServiceStub service = new StandardSlackServiceStub("", "domain", null, id, true, "#room1:1528317530");
final CloseableHttpClientStub httpClientStub = new CloseableHttpClientStub();
httpClientStub.setHttpStatus(HttpStatus.SC_OK);
service.setHttpClient(httpClientStub);
Expand All @@ -64,50 +60,18 @@ public void globalCredentialsCanBeUsed() {
}

@Test
public void otherJobCredentialsAreNotObtained() {
final String id = "cred-id";
final String secretText = "secret-text";
final Job job = createJobWithAvailableCredentialId(id, secretText);
final StandardSlackServiceStub service = new StandardSlackServiceStub("", "domain", null, "wrongid", true, "#room1:1528317530", job);
final CloseableHttpClientStub httpClientStub = new CloseableHttpClientStub();
httpClientStub.setHttpStatus(HttpStatus.SC_OK);
service.setHttpClient(httpClientStub);
service.publish("message");
assertFalse(httpClientStub.getLastRequest().getURI().toString().contains(secretText));
}

@Test
public void ifNoJobProvidedTokenIsUsed() {
public void tokenIsUsed() {
final String token = "explicittoken";
final StandardSlackServiceStub service = new StandardSlackServiceStub("", "domain", token, null, true, "#room1:1528317530", null);
final StandardSlackServiceStub service = new StandardSlackServiceStub("", "domain", token, null, true, "#room1:1528317530");
final CloseableHttpClientStub httpClientStub = new CloseableHttpClientStub();
httpClientStub.setHttpStatus(HttpStatus.SC_OK);
service.setHttpClient(httpClientStub);
service.publish("message");
assertTrue(httpClientStub.getLastRequest().getURI().toString().contains(token));
}

/**
* Creates a mocked job and sets up the mocking mechanism so the provided credential is obtainable.
* @param id the id of the credential to use.
* @param secretText the secret text of the credential.
* @return a mocked job set up so it can use the provided credentials
*/
private Job createJobWithAvailableCredentialId(final String id, final String secretText) {
final Job job = mock(Job.class);
final CredentialsProvider credentialsProvider = mock(CredentialsProvider.class);
final List<StringCredentials> credentialList = setupMocks(id, secretText, credentialsProvider);
when(credentialsProvider.getCredentials(Matchers.eq(StringCredentials.class), Matchers.eq(job), Matchers.eq(ACL.SYSTEM), Matchers.eq(Collections.emptyList()))).thenReturn(credentialList);
return job;
}

private void setupGlobalAvailableCredentialId(final String id, final String secretText) {
final CredentialsProvider credentialsProvider = mock(CredentialsProvider.class);
final List<StringCredentials> credentialList = setupMocks(id, secretText, credentialsProvider);
when(credentialsProvider.getCredentials(Matchers.eq(StringCredentials.class), Matchers.eq(jenkins), Matchers.eq(ACL.SYSTEM), Matchers.eq(Collections.emptyList()))).thenReturn(credentialList);
}

private List<StringCredentials> setupMocks(String id, String secretText, CredentialsProvider credentialsProvider) {
final ExtensionList<CredentialsProvider> extensionList = mock(ExtensionList.class);
final List<CredentialsProvider> listProviders = new ArrayList<>(1);
final List<StringCredentials> credentialList = new ArrayList<>();
Expand All @@ -121,7 +85,7 @@ private List<StringCredentials> setupMocks(String id, String secretText, Credent
when(secret.getPlainText()).thenReturn(secretText);
when(credentials.getSecret()).thenReturn(secret);
credentialList.add(credentials);
return credentialList;
when(credentialsProvider.getCredentials(Matchers.eq(StringCredentials.class), Matchers.eq(jenkins), Matchers.eq(ACL.SYSTEM), Matchers.eq(Collections.emptyList()))).thenReturn(credentialList);
}

}
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
package jenkins.plugins.slack;

import hudson.model.Item;

public class StandardSlackServiceStub extends StandardSlackService {

private CloseableHttpClientStub httpClientStub;

public StandardSlackServiceStub(String baseUrl, String teamDomain, String token, String tokenCredentialId, boolean botUser, String roomId, Item item) {
super(baseUrl, teamDomain, token, tokenCredentialId, botUser, roomId, false, item);
public StandardSlackServiceStub(String baseUrl, String teamDomain, String token, String tokenCredentialId, boolean botUser, String roomId) {
super(baseUrl, teamDomain, token, tokenCredentialId, botUser, roomId, false);
}

public StandardSlackServiceStub(String baseUrl, String teamDomain, boolean botUser, String roomId, String populatedToken) {
super(baseUrl, teamDomain, botUser, roomId, false, populatedToken);
}

@Override
Expand Down
Loading