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.

Sign up for GitHub

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
Merged

Fix folder credential usage #541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d35f206
Fixes folder credentials not working for Slack
Mar 13, 2019
8d05af3
Adapt code so pipelines work too
Mar 14, 2019
1c8d88e
undoing modification of previous constructors for StandardSlackService
Mar 15, 2019
7d3c7e9
modified service constructor so it takes a String instead of an Item
Mar 15, 2019
a01335b
Update src/main/java/jenkins/plugins/slack/StandardSlackService.java
timja Mar 15, 2019
94811cd
addresses PR comments
sirstrahd Mar 15, 2019
4e071dc
fix additional PR comments
sirstrahd Mar 15, 2019
8217e7c
Address PR comments + exception handling for factory calls
sirstrahd Mar 15, 2019
2edbd9d
Rerun CI
timja Mar 16, 2019
c63a66c
Fix pipeline syntax not retrieving folder creds
timja Mar 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 57 additions & 0 deletions src/main/java/jenkins/plugins/slack/CredentialsObtainer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
package jenkins.plugins.slack;

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

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

public class CredentialsObtainer {

public static StringCredentials lookupCredentials(String credentialId) {
List<StringCredentials> credentials = CredentialsProvider.lookupCredentials(StringCredentials.class, Jenkins.get(), ACL.SYSTEM, Collections.emptyList());
return getCredentialWithId(credentialId, credentials);
}

public static StringCredentials lookupCredentials(String credentialId, Item item) {
List<StringCredentials> credentials = CredentialsProvider.lookupCredentials(StringCredentials.class, item, ACL.SYSTEM, Collections.emptyList());
return getCredentialWithId(credentialId, credentials);
}

/**
* 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) {
String response;
if (StringUtils.isEmpty(credentialId)) {
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.

} else {
StringCredentials credentials = lookupCredentials(StringUtils.trim(credentialId), item);
if (credentials != null) {
response = credentials.getSecret().getPlainText();
} else {
response = token;
}
}
if (StringUtils.isEmpty(response)) {
throw new IllegalArgumentException("the token with the provided ID could not be found and no token was specified");
}
return response;
}

private static StringCredentials getCredentialWithId(String credentialId, List<StringCredentials> credentials) {
CredentialsMatcher matcher = CredentialsMatchers.withId(credentialId);
return CredentialsMatchers.firstOrNull(credentials, matcher);
}
}
58 changes: 42 additions & 16 deletions src/main/java/jenkins/plugins/slack/SlackNotifier.java
View file
Open in desktop
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 All @@ -10,6 +11,7 @@
import hudson.model.AbstractProject;
import hudson.model.BuildListener;
import hudson.model.Item;
import hudson.model.Project;
import hudson.security.ACL;
import hudson.tasks.BuildStepDescriptor;
import hudson.tasks.BuildStepMonitor;
Expand All @@ -33,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 @@ -405,7 +408,7 @@ public BuildStepMonitor getRequiredMonitorService() {
return BuildStepMonitor.NONE;
}

public SlackService newSlackService(AbstractBuild r, BuildListener listener) {
public SlackService newSlackService(AbstractBuild abstractBuild, BuildListener listener) {
DescriptorImpl descriptor = getDescriptor();
String teamDomain = Util.fixEmpty(this.teamDomain) != null ? this.teamDomain : descriptor.getTeamDomain();
String baseUrl = Util.fixEmpty(this.baseUrl) != null ? this.baseUrl : descriptor.getBaseUrl();
Expand All @@ -417,7 +420,7 @@ public SlackService newSlackService(AbstractBuild r, BuildListener listener) {

EnvVars env;
try {
env = r.getEnvironment(listener);
env = abstractBuild.getEnvironment(listener);
} catch (Exception e) {
listener.getLogger().println("Error retrieving environment vars: " + e.getMessage());
env = new EnvVars();
Expand All @@ -427,8 +430,8 @@ 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);
final String populatedToken = CredentialsObtainer.getTokenToUse(authTokenCredentialId, abstractBuild.getParent(), authToken);
return new StandardSlackService(baseUrl, teamDomain, botUser, room, false, populatedToken);
}

@Override
Expand All @@ -442,20 +445,29 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
BuildAwareLogger log = createLogger(listener);
log.debug(buildKey, "Performing complete notifications");
JenkinsTokenExpander tokenExpander = new JenkinsTokenExpander(listener);
new ActiveNotifier(this, slackFactory(listener), log, tokenExpander).completed(build);
if (notifyRegression) {
log.debug(buildKey, "Performing finalize notifications");
new ActiveNotifier(this, slackFactory(listener), log, tokenExpander).finalized(build);
try {
new ActiveNotifier(this, slackFactory(listener), log, tokenExpander).completed(build);
if (notifyRegression) {
log.debug(buildKey, "Performing finalize notifications");
new ActiveNotifier(this, slackFactory(listener), log, tokenExpander).finalized(build);
}
} catch (Exception e) {
log.info(buildKey,"Exception attempting Slack notification: " + e.getMessage());
}
return true;
}

@Override
public boolean prebuild(AbstractBuild<?, ?> build, BuildListener listener) {
if (startNotification) {
BuildAwareLogger log = createLogger(listener);
log.debug(BuildKey.format(build), "Performing start notifications");
new ActiveNotifier(this, slackFactory(listener), log, new JenkinsTokenExpander(listener)).started(build);
String buildKey = BuildKey.format(build);
BuildAwareLogger log = createLogger(listener);
try {
if (startNotification) {
log.debug(buildKey, "Performing start notifications");
new ActiveNotifier(this, slackFactory(listener), log, new JenkinsTokenExpander(listener)).started(build);
}
} catch (Exception e) {
log.info(buildKey,"Exception attempting Slack notification: " + e.getMessage());
}
return super.prebuild(build, listener);
}
Expand Down Expand Up @@ -604,8 +616,21 @@ public boolean configure(StaplerRequest req, JSONObject formData) {
return true;
}

SlackService getSlackService(final String baseUrl, final String teamDomain, final String authTokenCredentialId, final boolean botUser, final String room) {
return new StandardSlackService(baseUrl, teamDomain, authTokenCredentialId, botUser, room);
/**
* @deprecated use {@link #getSlackService(String, String, String, boolean, String, Item)} instead}
*/
@Deprecated
SlackService getSlackService(final String baseUrl, final String teamDomain, final String authTokenCredentialId, final boolean botUser, final String roomId) {
return getSlackService(baseUrl, teamDomain, authTokenCredentialId, botUser, roomId, null);
}

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
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 All @@ -618,7 +643,8 @@ public FormValidation doTestConnection(@QueryParameter("baseUrl") final String b
@QueryParameter("teamDomain") final String teamDomain,
@QueryParameter("tokenCredentialId") final String tokenCredentialId,
@QueryParameter("botUser") final boolean botUser,
@QueryParameter("room") final String room) {
@QueryParameter("room") final String room,
@AncestorInPath Project project) {

try {
String targetUrl = baseUrl;
Expand All @@ -636,7 +662,7 @@ public FormValidation doTestConnection(@QueryParameter("baseUrl") final String b
this.tokenCredentialId;
String targetRoom = Util.fixEmpty(room) != null ? room : this.room;

SlackService testSlackService = getSlackService(targetUrl, targetDomain, targetTokenCredentialId, targetBotUser, targetRoom);
SlackService testSlackService = getSlackService(targetUrl, targetDomain, targetTokenCredentialId, targetBotUser, targetRoom, project);
String message = "Slack/Jenkins plugin: you're all set on " + DisplayURLProvider.get().getRoot();
boolean success = testSlackService.publish(message, "good");
return success ? FormValidation.ok("Success") : FormValidation.error("Failure");
Expand Down
67 changes: 48 additions & 19 deletions src/main/java/jenkins/plugins/slack/StandardSlackService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.cloudbees.plugins.credentials.CredentialsMatcher;
import com.cloudbees.plugins.credentials.CredentialsMatchers;
import hudson.ProxyConfiguration;
import hudson.model.Item;
import hudson.security.ACL;
import jenkins.model.Jenkins;
import jenkins.plugins.slack.logging.BuildKey;
Expand Down Expand Up @@ -44,31 +45,67 @@ public class StandardSlackService implements SlackService {
private String host = "slack.com";
private String baseUrl;
private String teamDomain;
private String token;
private String authTokenCredentialId;
private boolean botUser;
private String[] roomIds;
private boolean replyBroadcast;
private String responseString = null;
private String responseString;
private String populatedToken;

/**
* @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);
}

/**
* @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 •
edited
Loading

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

/**
* @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, botUser, roomId, replyBroadcast);
this.populatedToken = getTokenToUse(authTokenCredentialId, token);
if (this.populatedToken == null) {
throw new IllegalArgumentException("No slack token found, setup a secret text credential and configure it to be used");
}
}

/**
* @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("No slack token found, setup a secret text credential and configure it to be used");
}
this.populatedToken = populatedToken;
}

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;
if (roomId == null) {
throw new IllegalArgumentException("Project Channel or Slack User ID must be specified.");
}
this.roomIds = roomId.split("[,; ]+");
this.replyBroadcast = replyBroadcast;
}
Expand Down Expand Up @@ -120,12 +157,11 @@ public boolean publish(String message, JSONArray attachments, String color) {
roomId = splitThread[0];
threadTs = splitThread[1];
}

//prepare post methods for both requests types
if (!botUser || !StringUtils.isEmpty(baseUrl)) {
url = "https://" + teamDomain + "." + host + "/services/hooks/jenkins-ci?token=" + getTokenToUse();
url = "https://" + teamDomain + "." + host + "/services/hooks/jenkins-ci?token=" + populatedToken;
if (!StringUtils.isEmpty(baseUrl)) {
url = baseUrl + getTokenToUse();
url = baseUrl + populatedToken;
}
post = new HttpPost(url);
JSONObject json = new JSONObject();
Expand All @@ -139,7 +175,7 @@ public boolean publish(String message, JSONArray attachments, String color) {

nvps.add(new BasicNameValuePair("payload", json.toString()));
} else {
url = "https://slack.com/api/chat.postMessage?token=" + getTokenToUse() +
url = "https://slack.com/api/chat.postMessage?token=" + populatedToken +
"&channel=" + roomId.replace("#", "") +
"&link_names=1" +
"&as_user=true";
Expand Down Expand Up @@ -185,26 +221,19 @@ public boolean publish(String message, JSONArray attachments, String color) {
return result;
}

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

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

return token;
}

private StringCredentials lookupCredentials(String credentialId) {
List<StringCredentials> credentials = com.cloudbees.plugins.credentials.CredentialsProvider.lookupCredentials(StringCredentials.class, Jenkins.get(), 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
Loading