Unverified Commit 29e26739 authored by Martin Lowe's avatar Martin Lowe 🇨🇦 Committed by GitHub
Browse files

Revert the revert for bot checking logic and fix null pointer issue (#50)

* Revert "Revert "Update to bot checking logic to check for project and any matching email""

This reverts commit c1bd01af.

Revert "Revert "Fix tests associated with changes to bot detection logic""

This reverts commit 9480b8fc.

Revert "Revert "Maintain support for bots committing to non-project repositories""

This reverts commit 1bf96e82

.

* Add fix for null pointer when handling emails

There was a production issue with some commits generating null pointer
exceptions. Added extra object safety around mail strings.
Signed-off-by: Martin Lowe's avatarMartin Lowe <martin.lowe@eclipse-foundation.org>

* Fix merge commit for validation resource
parent 329621fe
......@@ -16,7 +16,8 @@ import javax.ws.rs.Path;
import javax.ws.rs.Produces;
import org.eclipse.microprofile.rest.client.inject.RegisterRestClient;
import org.eclipsefoundation.git.eca.model.BotUser;
import com.fasterxml.jackson.databind.JsonNode;
/**
* Interface for interacting with the Eclipse Foundation Bots API.
......@@ -35,5 +36,5 @@ public interface BotsAPI {
*/
@GET
@Produces("application/json")
List<BotUser> getBots();
List<JsonNode> getBots();
}
/**
* Copyright (C) 2020 Eclipse Foundation
*
* <p>This program and the accompanying materials are made available under the terms of the Eclipse
* Public License 2.0 which is available at https://www.eclipse.org/legal/epl-2.0/
*
* <p>SPDX-License-Identifier: EPL-2.0
*/
package org.eclipsefoundation.git.eca.model;
import com.fasterxml.jackson.annotation.JsonProperty;
/**
* Represents a bot user in the Eclipse API.
*
* @author Martin Lowe
*/
public class BotUser {
private String id;
private String projectId;
private String username;
private String email;
@JsonProperty("github.com")
private SiteSpecificBot githubBot;
@JsonProperty("gitlab.eclipse.org")
private SiteSpecificBot gitlabBot;
/** @return the id */
public String getId() {
return id;
}
/** @param id the id to set */
public void setId(String id) {
this.id = id;
}
/** @return the projectId */
public String getProjectId() {
return projectId;
}
/** @param projectId the projectId to set */
public void setProjectId(String projectId) {
this.projectId = projectId;
}
/** @return the username */
public String getUsername() {
return username;
}
/** @param username the username to set */
public void setUsername(String username) {
this.username = username;
}
/** @return the email */
public String getEmail() {
return email;
}
/** @return the githubBot */
public SiteSpecificBot getGithubBot() {
return githubBot;
}
/** @param githubBot the githubBot to set */
public void setGithubBot(SiteSpecificBot githubBot) {
this.githubBot = githubBot;
}
/** @return the gitlabBot */
public SiteSpecificBot getGitlabBot() {
return gitlabBot;
}
/** @param gitlabBot the gitlabBot to set */
public void setGitlabBot(SiteSpecificBot gitlabBot) {
this.gitlabBot = gitlabBot;
}
/** @param email the email to set */
public void setEmail(String email) {
this.email = email;
}
@Override
public String toString() {
StringBuilder builder = new StringBuilder();
builder.append("BotUser [id=");
builder.append(id);
builder.append(", projectId=");
builder.append(projectId);
builder.append(", username=");
builder.append(username);
builder.append(", email=");
builder.append(email);
builder.append(", githubBot=");
builder.append(githubBot);
builder.append(", gitlabBot=");
builder.append(gitlabBot);
builder.append("]");
return builder.toString();
}
public static class SiteSpecificBot {
private String username;
private String email;
/** @return the username */
public String getUsername() {
return username;
}
/** @param username the username to set */
public void setUsername(String username) {
this.username = username;
}
/** @return the email */
public String getEmail() {
return email;
}
/** @param email the email to set */
public void setEmail(String email) {
this.email = email;
}
}
}
......@@ -10,7 +10,9 @@ package org.eclipsefoundation.git.eca.resource;
import java.net.MalformedURLException;
import java.util.Collections;
import java.util.Iterator;
import java.util.List;
import java.util.Map.Entry;
import java.util.Optional;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
......@@ -30,7 +32,6 @@ import org.eclipse.microprofile.rest.client.inject.RestClient;
import org.eclipsefoundation.git.eca.api.AccountsAPI;
import org.eclipsefoundation.git.eca.api.BotsAPI;
import org.eclipsefoundation.git.eca.helper.CommitHelper;
import org.eclipsefoundation.git.eca.model.BotUser;
import org.eclipsefoundation.git.eca.model.Commit;
import org.eclipsefoundation.git.eca.model.EclipseUser;
import org.eclipsefoundation.git.eca.model.GitUser;
......@@ -45,6 +46,8 @@ import org.eclipsefoundation.git.eca.service.ProjectsService;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import com.fasterxml.jackson.databind.JsonNode;
/**
* ECA validation endpoint for Git commits. Will use information from the bots, projects, and
* accounts API to validate commits passed to this endpoint. Should be as system agnostic as
......@@ -182,7 +185,7 @@ public class ValidationResource {
EclipseUser eclipseAuthor = getIdentifiedUser(author);
if (eclipseAuthor == null) {
// if the user is a bot, generate a stubbed user
if (!userIsABot(author.getMail(), provider)) {
if (!userIsABot(author.getMail(), filteredProjects)) {
addMessage(
response,
String.format(
......@@ -208,7 +211,7 @@ public class ValidationResource {
committer.getMail(), c.getHash()),
c.getHash());
eclipseCommitter = EclipseUser.createBotStub(committer);
} else if (!userIsABot(committer.getMail(), provider)) {
} else if (!userIsABot(committer.getMail(), filteredProjects)) {
addMessage(
response,
String.format(
......@@ -320,33 +323,66 @@ public class ValidationResource {
}
}
// check if user is a bot, either through early detection or through on-demand check
if (user.isBot() || userIsABot(user.getMail(), provider)) {
if (user.isBot() || userIsABot(user.getMail(), filteredProjects)) {
LOGGER.debug("User '{} <{}>' was found to be a bot", user.getName(), user.getMail());
return true;
}
return false;
}
private boolean userIsABot(String mail, ProviderType provider) {
if (mail == null || "".equals(mail.trim())) {
return false;
private boolean userIsABot(String mail, List<Project> filteredProjects) {
if (mail == null || "".equals(mail.trim())) {
return false;
}
List<JsonNode> botObjs = getBots();
// if there are no matching projects, then check against all bots, not just project bots
if (filteredProjects == null || filteredProjects.isEmpty()) {
return botObjs.stream().anyMatch(bot -> checkFieldsForMatchingMail(bot, mail));
}
// for each of the matched projects, check the bot for the matching project ID
for (Project p : filteredProjects) {
LOGGER.debug("Checking project {} for matching bots", p.getProjectId());
for (JsonNode bot : botObjs) {
// if the project ID match, and one of the email fields matches, then user is bot
if (p.getProjectId().equalsIgnoreCase(bot.get("projectId").asText())
&& checkFieldsForMatchingMail(bot, mail)) {
return true;
}
}
}
return false;
}
return getBots()
.stream()
.anyMatch(
bot -> {
if (ProviderType.GITHUB.equals(provider)) {
return bot.getGithubBot() != null
&& mail.equalsIgnoreCase(bot.getGithubBot().getEmail());
} else if (ProviderType.GITLAB.equals(provider)) {
return bot.getGitlabBot() != null
&& mail.equalsIgnoreCase(bot.getGitlabBot().getEmail());
} else {
return mail.equalsIgnoreCase(bot.getEmail());
}
});
}
/**
* Checks JSON node to look for email fields, both at the root, and nested email fields.
*
* @param bot the bots JSON object representation
* @param mail the email to match against
* @return true if the bot has a matching email value, otherwise false
*/
private boolean checkFieldsForMatchingMail(JsonNode bot, String mail) {
// check the root email for the bot for match
JsonNode botmail = bot.get("email");
if (mail != null && botmail != null && mail.equalsIgnoreCase(botmail.asText(""))) {
return true;
}
Iterator<Entry<String, JsonNode>> i = bot.fields();
while (i.hasNext()) {
Entry<String, JsonNode> e = i.next();
// check that our field is an object with fields
JsonNode node = e.getValue();
if (node.isObject()) {
LOGGER.debug("Checking {} for bot email", e.getKey());
// if the mail matches (ignoring case) user is bot
JsonNode botAliasMail = node.get("email");
if (mail != null && botAliasMail != null && mail.equalsIgnoreCase(botAliasMail.asText(""))) {
LOGGER.debug("Found match for bot email {}", mail);
return true;
}
}
}
return false;
}
private boolean isAllowedUser(String mail) {
return allowListUsers.indexOf(mail) != -1;
}
......@@ -484,13 +520,13 @@ public class ValidationResource {
}
@SuppressWarnings("unchecked")
private List<BotUser> getBots() {
Optional<List<BotUser>> allBots =
cache.get("allBots", () -> bots.getBots(), (Class<List<BotUser>>) (Object) List.class);
if (!allBots.isPresent()) {
return Collections.emptyList();
}
return allBots.get();
private List<JsonNode> getBots() {
Optional<List<JsonNode>> allBots = cache.get("allBots", () -> bots.getBots(),
(Class<List<JsonNode>>) (Object) List.class);
if (!allBots.isPresent()) {
return Collections.emptyList();
}
return allBots.get();
}
private void addMessage(ValidationResponse r, String message, String hash) {
......
......@@ -16,8 +16,11 @@ import javax.annotation.PostConstruct;
import javax.enterprise.context.ApplicationScoped;
import org.eclipse.microprofile.rest.client.inject.RestClient;
import org.eclipsefoundation.git.eca.model.BotUser;
import org.eclipsefoundation.git.eca.model.BotUser.SiteSpecificBot;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.node.JsonNodeFactory;
import com.fasterxml.jackson.databind.node.ObjectNode;
import com.fasterxml.jackson.databind.node.TextNode;
import io.quarkus.test.Mock;
......@@ -26,43 +29,43 @@ import io.quarkus.test.Mock;
@ApplicationScoped
public class MockBotsAPI implements BotsAPI {
private List<BotUser> src;
private List<JsonNode> src;
@PostConstruct
public void build() {
this.src = new ArrayList<>();
BotUser b1 = new BotUser();
b1.setId("1");
b1.setEmail("1.bot@eclipse.org");
b1.setProjectId("sample.proj");
b1.setUsername("projbot");
ObjectNode b1 = new ObjectNode(JsonNodeFactory.instance);
b1.set("id", new TextNode("1"));
b1.set("email", new TextNode("1.bot@eclipse.org"));
b1.set("projectId", new TextNode("sample.proj"));
b1.set("username", new TextNode("projbot"));
src.add(b1);
BotUser b2 = new BotUser();
b2.setId("10");
b2.setEmail("2.bot@eclipse.org");
b2.setProjectId("sample.proto");
b2.setUsername("protobot");
SiteSpecificBot ssbGH = new SiteSpecificBot();
ssbGH.setEmail("2.bot-github@eclipse.org");
ssbGH.setUsername("protobot-gh");
b2.setGithubBot(ssbGH);
ObjectNode b2 = new ObjectNode(JsonNodeFactory.instance);
b2.set("id", new TextNode("10"));
b2.set("email", new TextNode("2.bot@eclipse.org"));
b2.set("projectId", new TextNode("sample.proto"));
b2.set("username", new TextNode("protobot"));
ObjectNode ssbGH = new ObjectNode(JsonNodeFactory.instance);
ssbGH.set("email", new TextNode("2.bot-github@eclipse.org"));
ssbGH.set("username", new TextNode("protobot-gh"));
b2.set("github.com", ssbGH);
src.add(b2);
BotUser b3 = new BotUser();
b3.setId("11");
b3.setEmail("3.bot@eclipse.org");
b3.setProjectId("spec.proj");
b3.setUsername("specbot");
SiteSpecificBot ssbGL = new SiteSpecificBot();
ssbGL.setEmail("3.bot-gitlab@eclipse.org");
ssbGL.setUsername("protobot-gl");
b3.setGitlabBot(ssbGL);
ObjectNode b3 = new ObjectNode(JsonNodeFactory.instance);
b3.set("id", new TextNode("11"));
b3.set("email", new TextNode("3.bot@eclipse.org"));
b3.set("projectId", new TextNode("spec.proj"));
b3.set("username", new TextNode("specbot"));
ObjectNode ssbGL = new ObjectNode(JsonNodeFactory.instance);
ssbGL.set("email", new TextNode("3.bot-gitlab@eclipse.org"));
ssbGL.set("username", new TextNode("protobot-gl"));
b3.set("gitlab.eclipse.org",ssbGL);
src.add(b3);
}
@Override
public List<BotUser> getBots() {
public List<JsonNode> getBots() {
return new ArrayList<>(src);
}
}
......@@ -77,8 +77,8 @@ public class MockProjectsAPI implements ProjectsAPI {
p2.setProjectId("sample.proto");
p2.setSpecWorkingGroup(null);
p2.setGithubRepos(Arrays.asList(r3));
p1.setGerritRepos(Arrays.asList(r6));
p1.setGitlabRepos(Arrays.asList(r8));
p2.setGerritRepos(Arrays.asList(r6));
p2.setGitlabRepos(Arrays.asList(r8));
p2.setCommitters(Arrays.asList(u2));
src.add(p2);
......@@ -87,7 +87,7 @@ public class MockProjectsAPI implements ProjectsAPI {
p3.setProjectId("spec.proj");
p3.setSpecWorkingGroup("proj1");
p3.setGithubRepos(Arrays.asList(r4));
p1.setGitlabRepos(Arrays.asList(r7));
p3.setGitlabRepos(Arrays.asList(r7));
p3.setCommitters(Arrays.asList(u1, u2));
src.add(p3);
}
......
......@@ -671,6 +671,58 @@ class ValidationResourceTest {
@Test
void validateBotCommiterAccessGithub() throws URISyntaxException {
// set up test users
GitUser g1 = new GitUser();
g1.setName("projbot");
g1.setMail("1.bot@eclipse.org");
List<Commit> commits = new ArrayList<>();
// create sample commits
Commit c1 = new Commit();
c1.setAuthor(g1);
c1.setCommitter(g1);
c1.setHash("123456789abcdefghijklmnop");
c1.setSubject("All of the things");
c1.setParents(Arrays.asList("46bb69bf6aa4ed26b2bf8c322ae05bef0bcc5c10"));
commits.add(c1);
ValidationRequest vr = new ValidationRequest();
vr.setProvider(ProviderType.GITHUB);
vr.setRepoUrl(new URI("http://www.github.com/eclipsefdn/sample"));
vr.setCommits(commits);
// test output w/ assertions
// Should be valid as bots should only commit on their own projects (including aliases)
given().body(vr).contentType(ContentType.JSON).when().post("/eca").then().statusCode(200);
}
@Test
void validateBotCommiterAccessGithub_untracked() throws URISyntaxException {
// set up test users
GitUser g1 = new GitUser();
g1.setName("projbot");
g1.setMail("1.bot@eclipse.org");
List<Commit> commits = new ArrayList<>();
// create sample commits
Commit c1 = new Commit();
c1.setAuthor(g1);
c1.setCommitter(g1);
c1.setHash("123456789abcdefghijklmnop");
c1.setSubject("All of the things");
c1.setParents(Arrays.asList("46bb69bf6aa4ed26b2bf8c322ae05bef0bcc5c10"));
commits.add(c1);
ValidationRequest vr = new ValidationRequest();
vr.setProvider(ProviderType.GITHUB);
vr.setRepoUrl(new URI("http://www.github.com/eclipsefdn/sample-untracked"));
vr.setCommits(commits);
// test output w/ assertions
// Should be valid as bots can commit on any untracked project (legacy support)
given().body(vr).contentType(ContentType.JSON).when().post("/eca").then().statusCode(200);
}
@Test
void validateBotCommiterAccessGithub_invalidBot() throws URISyntaxException {
// set up test users
GitUser g1 = new GitUser();
g1.setName("protobot-gh");
......@@ -691,8 +743,8 @@ class ValidationResourceTest {
vr.setRepoUrl(new URI("http://www.github.com/eclipsefdn/sample"));
vr.setCommits(commits);
// test output w/ assertions
// Should be valid as bots don't need sign off and should be valid on any proj
given().body(vr).contentType(ContentType.JSON).when().post("/eca").then().statusCode(200);
// Should be invalid as bots should only commit on their own projects (including aliases)
given().body(vr).contentType(ContentType.JSON).when().post("/eca").then().statusCode(403);
}
@Test
......@@ -723,10 +775,62 @@ class ValidationResourceTest {
@Test
void validateBotCommiterAccessGitlab() throws URISyntaxException {
// set up test users
// set up test users
GitUser g1 = new GitUser();
g1.setName("protobot-gh");
g1.setMail("2.bot-github@eclipse.org");
List<Commit> commits = new ArrayList<>();
// create sample commits
Commit c1 = new Commit();
c1.setAuthor(g1);
c1.setCommitter(g1);
c1.setHash("123456789abcdefghijklmnop");
c1.setSubject("All of the things");
c1.setParents(Arrays.asList("46bb69bf6aa4ed26b2bf8c322ae05bef0bcc5c10"));
commits.add(c1);
ValidationRequest vr = new ValidationRequest();
vr.setProvider(ProviderType.GITLAB);
vr.setRepoUrl(new URI("https://gitlab.eclipse.org/eclipse/dash/dash.handbook.test"));
vr.setCommits(commits);
// test output w/ assertions
// Should be valid as bots should only commit on their own projects (including aliases)
given().body(vr).contentType(ContentType.JSON).when().post("/eca").then().statusCode(200);
}
@Test
void validateBotCommiterAccessGitlab_untracked() throws URISyntaxException {
// set up test users
GitUser g1 = new GitUser();
g1.setName("protobot-gh");
g1.setMail("2.bot-github@eclipse.org");
List<Commit> commits = new ArrayList<>();
// create sample commits
Commit c1 = new Commit();
c1.setAuthor(g1);
c1.setCommitter(g1);
c1.setHash("123456789abcdefghijklmnop");
c1.setSubject("All of the things");
c1.setParents(Arrays.asList("46bb69bf6aa4ed26b2bf8c322ae05bef0bcc5c10"));
commits.add(c1);
ValidationRequest vr = new ValidationRequest();
vr.setProvider(ProviderType.GITLAB);
vr.setRepoUrl(new URI("https://gitlab.eclipse.org/eclipse/dash/dash.handbook.untracked"));
vr.setCommits(commits);
// test output w/ assertions
// Should be valid as bots can commit on any untracked project (legacy support)
given().body(vr).contentType(ContentType.JSON).when().post("/eca").then().statusCode(200);
}
@Test
void validateBotCommiterAccessGitlab_invalidBot() throws URISyntaxException {
// set up test users (wrong bot for project)
GitUser g1 = new GitUser();
g1.setName("specbot-gh");
g1.setMail("3.bot-gitlab@eclipse.org");
g1.setName("specbot");
g1.setMail("3.bot@eclipse.org");
List<Commit> commits = new ArrayList<>();
// create sample commits
......@@ -743,13 +847,13 @@ class ValidationResourceTest {
vr.setRepoUrl(new URI("https://gitlab.eclipse.org/eclipse/dash/dash.handbook.test"));
vr.setCommits(commits);
// test output w/ assertions
// Should be valid as bots don't need sign off and should be valid on any proj
given().body(vr).contentType(ContentType.JSON).when().post("/eca").then().statusCode(200);
// Should be invalid as bots should only commit on their own projects
given().body(vr).contentType(ContentType.JSON).when().post("/eca").then().statusCode(403);
}
@Test
void validateBotCommiterAccessGitlab_wrongEmail() throws URISyntaxException {
// set up test users - uses Gerrit/LDAP email (wrong for case)
// set up test users - uses Gerrit/LDAP email (expects Gitlab email)
GitUser g1 = new GitUser();
g1.setName("specbot");
g1.setMail("3.bot@eclipse.org");
......@@ -769,8 +873,8 @@ class ValidationResourceTest {
vr.setRepoUrl(new URI("https://gitlab.eclipse.org/eclipse/dash/dash.git"));
vr.setCommits(commits);
// test output w/ assertions
// Should be invalid as wrong email was used for bot (uses Gerrit bot email)
given().body(vr).contentType(ContentType.JSON).when().post("/eca").then().statusCode(403);
// Should be valid as wrong email was used, but is still bot email alias (uses Gerrit bot email)
given().body(vr).contentType(ContentType.JSON).when().post("/eca").then().statusCode(200);
}