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 2 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
13 changes: 7 additions & 6 deletions src/main/java/jenkins/plugins/slack/SlackNotifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,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 Down Expand Up @@ -427,8 +428,7 @@ 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);
return new StandardSlackService(baseUrl, teamDomain, authToken, authTokenCredentialId, botUser, room, r.getProject());
}

@Override
Expand Down Expand Up @@ -604,8 +604,8 @@ 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);
SlackService getSlackService(final String baseUrl, final String teamDomain, final String authTokenCredentialId, final boolean botUser, final String room, final Item item) {
return new StandardSlackService(baseUrl, teamDomain, authTokenCredentialId, botUser, room, item);
}

@Nonnull
Expand All @@ -618,7 +618,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 +637,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
23 changes: 13 additions & 10 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 @@ -50,16 +51,17 @@ public class StandardSlackService implements SlackService {
private String[] roomIds;
private boolean replyBroadcast;
private String responseString = null;
private Item item;

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

Choose a reason for hiding this comment

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

  1. 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
  2. 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

this(baseUrl, teamDomain, null, authTokenCredentialId, botUser, roomId, false, item);
}

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

public StandardSlackService(String baseUrl, String teamDomain, String token, String authTokenCredentialId, boolean botUser, String roomId, boolean replyBroadcast) {
public StandardSlackService(String baseUrl, String teamDomain, String token, String authTokenCredentialId, boolean botUser, String roomId, boolean replyBroadcast, Item item) {
super();
this.baseUrl = baseUrl;
if(this.baseUrl != null && !this.baseUrl.isEmpty() && !this.baseUrl.endsWith("/")) {
Expand All @@ -71,6 +73,7 @@ public StandardSlackService(String baseUrl, String teamDomain, String token, Str
this.botUser = botUser;
this.roomIds = roomId.split("[,; ]+");
this.replyBroadcast = replyBroadcast;
this.item = item;
}

public String getResponseString() {
Expand Down Expand Up @@ -120,12 +123,12 @@ public boolean publish(String message, JSONArray attachments, String color) {
roomId = splitThread[0];
threadTs = splitThread[1];
}

final String token = getTokenToUse();
//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=" + token;
if (!StringUtils.isEmpty(baseUrl)) {
url = baseUrl + getTokenToUse();
url = baseUrl + token;
}
post = new HttpPost(url);
JSONObject json = new JSONObject();
Expand All @@ -139,7 +142,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=" + token +
"&channel=" + roomId.replace("#", "") +
"&link_names=1" +
"&as_user=true";
Expand Down Expand Up @@ -200,7 +203,7 @@ private String getTokenToUse() {
}

private StringCredentials lookupCredentials(String credentialId) {
List<StringCredentials> credentials = com.cloudbees.plugins.credentials.CredentialsProvider.lookupCredentials(StringCredentials.class, Jenkins.get(), ACL.SYSTEM, Collections.emptyList());
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);
}
Expand Down
53 changes: 41 additions & 12 deletions src/main/java/jenkins/plugins/slack/workflow/SlackSendStep.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,11 @@
import org.kohsuke.stapler.DataBoundConstructor;
import org.kohsuke.stapler.DataBoundSetter;
import org.kohsuke.stapler.QueryParameter;
import java.util.logging.Level;

import java.util.Objects;
import java.util.Set;
import java.util.logging.Logger;
import javax.annotation.Nonnull;

import static com.cloudbees.plugins.credentials.CredentialsProvider.lookupCredentials;
Expand All @@ -46,6 +48,8 @@
*/
public class SlackSendStep extends Step {

private static final Logger logger = Logger.getLogger(SlackSendStep.class.getName());

private String message;
private String color;
private String token;
Expand Down Expand Up @@ -232,7 +236,7 @@ public static class SlackSendStepExecution extends SynchronousNonBlockingStepExe
protected SlackResponse run() throws Exception {

Jenkins jenkins = Jenkins.get();

Item item = getItemForCredentials();
SlackNotifier.DescriptorImpl slackDesc = jenkins.getDescriptorByType(SlackNotifier.DescriptorImpl.class);

String baseUrl = step.baseUrl != null ? step.baseUrl : slackDesc.getBaseUrl();
Expand All @@ -253,7 +257,7 @@ protected SlackResponse run() throws Exception {
);

SlackService slackService = getSlackService(
baseUrl, teamDomain, token, tokenCredentialId, botUser, channel, step.replyBroadcast
baseUrl, teamDomain, token, tokenCredentialId, botUser, channel, step.replyBroadcast, item
);
final boolean publishSuccess;
if (step.attachments != null) {
Expand Down Expand Up @@ -300,13 +304,12 @@ protected SlackResponse run() throws Exception {

JSONArray getAttachmentsAsJSONArray() throws Exception {
final TaskListener listener = getContext().get(TaskListener.class);
final String jsonString;
if (step.attachments instanceof String) {
jsonString = (String) step.attachments;
}
else {
jsonString = JsonOutput.toJson(step.attachments);
}
final String jsonString;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

if (step.attachments instanceof String) {
jsonString = (String) step.attachments;
} else {
jsonString = JsonOutput.toJson(step.attachments);
}

JsonSlurper jsonSlurper = new JsonSlurper();
JSON json = null;
Expand All @@ -316,20 +319,46 @@ JSONArray getAttachmentsAsJSONArray() throws Exception {
listener.error(Messages.NotificationFailedWithException(e));
return null;
}
if(!(json instanceof JSONArray)){
if (!(json instanceof JSONArray)) {
listener.error(Messages.NotificationFailedWithException(new IllegalArgumentException("Attachments must be JSONArray")));
return null;
}
return (JSONArray) json;
}

/**
* Tries to obtain the proper Item object to provide to CredentialsProvider.
* Project works for freestyle jobs, the parent of the Run works for pipelines.
* In case the proper item cannot be found, null is returned, since when null is provided to CredentialsProvider,
* it will internally use Jenkins.getInstance() which effectively only allows global credentials.
*
* @return the item to use for CredentialsProvider credential lookup
*/
private Item getItemForCredentials() {
Item item = null;
try {
item = getContext().get(Project.class);
if (item == null) {
Run run = getContext().get(Run.class);
if (run != null) {
item = run.getParent();
} else {
item = null;
}
}
} catch (Exception e) {
logger.log(Level.INFO, "Exception obtaining item for credentials lookup. Only global credentials will be available", e);
}
return item;
}

private String defaultIfEmpty(String value) {
return Util.fixEmpty(value) != null ? value : Messages.SlackSendStepValuesEmptyMessage();
}

//streamline unit testing
SlackService getSlackService(String baseUrl, String team, String token, String tokenCredentialId, boolean botUser, String channel, boolean replyBroadcast) {
return new StandardSlackService(baseUrl, team, token, tokenCredentialId, botUser, channel, replyBroadcast);
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);
}
}
}
6 changes: 6 additions & 0 deletions src/test/java/jenkins/plugins/slack/CloseableHttpClientStub.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@ public class CloseableHttpClientStub extends CloseableHttpClient {
private int numberOfCallsToExecuteMethod;
private int httpStatus;
private boolean failAlternateResponses = false;
private HttpUriRequest lastRequest = null;

public CloseableHttpResponse execute(HttpUriRequest post) {
lastRequest = post;
numberOfCallsToExecuteMethod++;
if (failAlternateResponses && (numberOfCallsToExecuteMethod % 2 == 0)) {
return new CloseableHttpResponseStub(HttpStatus.SC_NOT_FOUND);
Expand Down Expand Up @@ -57,4 +59,8 @@ public void setHttpStatus(int httpStatus) {
public void setFailAlternateResponses(boolean failAlternateResponses) {
this.failAlternateResponses = failAlternateResponses;
}

public HttpUriRequest getLastRequest() {
return lastRequest;
}
}
4 changes: 3 additions & 1 deletion src/test/java/jenkins/plugins/slack/SlackNotifierStub.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package jenkins.plugins.slack;

import hudson.model.Item;

public class SlackNotifierStub extends SlackNotifier {

public SlackNotifierStub(String baseUrl, String teamDomain, String authToken, boolean botUser, String room, String authTokenCredentialId,
Expand All @@ -23,7 +25,7 @@ public synchronized void load() {
}

@Override
SlackService getSlackService(final String baseUrl, final String teamDomain, final String authTokenCredentialId, final boolean botUser, final String room) {
SlackService getSlackService(final String baseUrl, final String teamDomain, final String authTokenCredentialId, final boolean botUser, final String room, final Item item) {
return slackService;
}

Expand Down
2 changes: 1 addition & 1 deletion src/test/java/jenkins/plugins/slack/SlackNotifierTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public void testDoTestConnection() {
}
descriptor.setSlackService(slackServiceStub);
FormValidation result = descriptor
.doTestConnection("baseUrl", "teamDomain", "authTokenCredentialId", false, "room");
.doTestConnection("baseUrl", "teamDomain", "authTokenCredentialId", false, "room", null);
assertEquals(result.kind, expectedResult);
}

Expand Down
Loading