Not sending authentication headers when user and/or password is not set

tomasbjerre · Apr 10, 2015 · 2537dec · 2537dec
1 parent 737506d
commit 2537dec
Show file tree

Hide file tree

Showing 5 changed files with 82 additions and 12 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,9 @@
 
 Changelog of Pull Request Notifier for Stash.
 
+## 1.7
+* Not sending authentication headers when user and/or password is not set
+
 ## 1.6
 * Correcting design with CSS for password field in admin view
 * Removing accidently added text from admin view

diff --git a/src/main/java/se/bjurr/prnfs/listener/UrlInvoker.java b/src/main/java/se/bjurr/prnfs/listener/UrlInvoker.java
@@ -26,7 +26,7 @@ public class UrlInvoker {
  public void ivoke(String urlParam, Optional<String> user, Optional<String> password) {
   InputStreamReader ir = null;
   try {
-   logger.info("Url: \"" + urlParam + "\" user: \"" + user.or("") + "\" password: \"" + password.or("") + "\"");
+   logger.info("Url: \"" + urlParam + "\"");
    final URL url = new URL(urlParam);
    final URLConnection uc = url.openConnection();
    setAuthorization(uc, user, password);
@@ -44,11 +44,17 @@ public void ivoke(String urlParam, Optional<String> user, Optional<String> passw
  @VisibleForTesting
  void setAuthorization(URLConnection uc, Optional<String> user, Optional<String> password)
    throws UnsupportedEncodingException {
-  if (user.isPresent() && password.isPresent()) {
+  if (shouldUseBasicAuth(user, password)) {
+   logger.info("user: \"" + user.or("") + "\" password: \"" + password.or("") + "\"");
    final String userpass = user.get() + ":" + password.get();
    final String basicAuth = "Basic " + new String(printBase64Binary(userpass.getBytes(UTF_8)));
    uc.setRequestProperty(AUTHORIZATION, basicAuth);
   }
 
  }
+
+ @VisibleForTesting
+ public static boolean shouldUseBasicAuth(Optional<String> user, Optional<String> password) {
+  return user.isPresent() && password.isPresent();
+ }
 }
diff --git a/src/main/java/se/bjurr/prnfs/settings/PrnfsNotification.java b/src/main/java/se/bjurr/prnfs/settings/PrnfsNotification.java
@@ -2,6 +2,7 @@
 
 import static com.google.common.base.Optional.fromNullable;
 import static com.google.common.base.Preconditions.checkNotNull;
+import static com.google.common.base.Strings.emptyToNull;
 import static com.google.common.base.Strings.nullToEmpty;
 import static java.util.regex.Pattern.compile;
 
@@ -23,7 +24,7 @@ public class PrnfsNotification {
 
  public PrnfsNotification(List<PullRequestAction> triggers, String url, String user, String password,
    String filterString, String filterRegexp) throws ValidationException {
-  this.password = nullToEmpty(password).trim();
+  this.password = emptyToNull(nullToEmpty(password).trim());
   if (nullToEmpty(url).trim().isEmpty()) {
    throw new ValidationException(AdminFormValues.FIELDS.url.name(), "URL not set!");
   }
@@ -45,7 +46,7 @@ public PrnfsNotification(List<PullRequestAction> triggers, String url, String us
    }
   }
   this.url = url;
-  this.user = nullToEmpty(user).trim();
+  this.user = emptyToNull(nullToEmpty(user).trim());
   this.triggers = checkNotNull(triggers);
   this.filterString = filterString;
   this.filterRegexp = filterRegexp;

diff --git a/src/test/java/se/bjurr/prnfs/admin/PrnfsPullRequestEventListenerTest.java b/src/test/java/se/bjurr/prnfs/admin/PrnfsPullRequestEventListenerTest.java
@@ -164,6 +164,54 @@ public void testThatBasicAuthenticationHeaderIsSentIfThereIsAUser() {
     .invokedUser("theuser").invokedPassword("thepassword");
  }
 
+ @Test
+ public void testThatBasicAuthenticationHeaderIsNotSentIfThereIsNoUser() {
+  prnfsTestBuilder()
+    .isLoggedInAsAdmin()
+    .withNotification(
+      notificationBuilder().withFieldValue(AdminFormValues.FIELDS.url, "http://bjurr.se/")
+        .withFieldValue(AdminFormValues.FIELDS.events, OPENED.name()).withFieldValue(AdminFormValues.FIELDS.user, "")
+        .withFieldValue(AdminFormValues.FIELDS.password, "thepassword").build()).store()
+    .trigger(pullRequestEventBuilder().withPullRequestAction(OPENED).build()).invokedUrl("http://bjurr.se/")
+    .didNotUseBasicAuth();
+ }
+
+ @Test
+ public void testThatBasicAuthenticationHeaderIsNotSentIfThereIsNoPassword() {
+  prnfsTestBuilder()
+    .isLoggedInAsAdmin()
+    .withNotification(
+      notificationBuilder().withFieldValue(AdminFormValues.FIELDS.url, "http://bjurr.se/")
+        .withFieldValue(AdminFormValues.FIELDS.events, OPENED.name())
+        .withFieldValue(AdminFormValues.FIELDS.user, "theuser").withFieldValue(AdminFormValues.FIELDS.password, "")
+        .build()).store().trigger(pullRequestEventBuilder().withPullRequestAction(OPENED).build())
+    .invokedUrl("http://bjurr.se/").didNotUseBasicAuth();
+ }
+
+ @Test
+ public void testThatBasicAuthenticationHeaderIsNotSentIfTheUserContainsOnlySpace() {
+  prnfsTestBuilder()
+    .isLoggedInAsAdmin()
+    .withNotification(
+      notificationBuilder().withFieldValue(AdminFormValues.FIELDS.url, "http://bjurr.se/")
+        .withFieldValue(AdminFormValues.FIELDS.events, OPENED.name()).withFieldValue(AdminFormValues.FIELDS.user, " ")
+        .withFieldValue(AdminFormValues.FIELDS.password, "thepassword").build()).store()
+    .trigger(pullRequestEventBuilder().withPullRequestAction(OPENED).build()).invokedUrl("http://bjurr.se/")
+    .didNotUseBasicAuth();
+ }
+
+ @Test
+ public void testThatBasicAuthenticationHeaderIsNotSentIfThePasswordContainsOnlySpace() {
+  prnfsTestBuilder()
+    .isLoggedInAsAdmin()
+    .withNotification(
+      notificationBuilder().withFieldValue(AdminFormValues.FIELDS.url, "http://bjurr.se/")
+        .withFieldValue(AdminFormValues.FIELDS.events, OPENED.name())
+        .withFieldValue(AdminFormValues.FIELDS.user, "theuser").withFieldValue(AdminFormValues.FIELDS.password, " ")
+        .build()).store().trigger(pullRequestEventBuilder().withPullRequestAction(OPENED).build())
+    .invokedUrl("http://bjurr.se/").didNotUseBasicAuth();
+ }
+
  @Test
  public void testThatFieldsUsedInAdminGUIArePresentInAdminFormFields() throws IOException {
   final URL resource = getResource("admin.vm");

diff --git a/src/test/java/se/bjurr/prnfs/admin/utils/PrnfsTestBuilder.java b/src/test/java/se/bjurr/prnfs/admin/utils/PrnfsTestBuilder.java
@@ -9,12 +9,14 @@
 import static com.google.common.collect.Maps.uniqueIndex;
 import static java.lang.Boolean.TRUE;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 import static se.bjurr.prnfs.admin.AdminFormValues.NAME;
 import static se.bjurr.prnfs.admin.AdminFormValues.VALUE;
+import static se.bjurr.prnfs.listener.UrlInvoker.shouldUseBasicAuth;
 import static se.bjurr.prnfs.settings.PrnfsPredicates.predicate;
 import static se.bjurr.prnfs.settings.SettingsStorage.FORM_IDENTIFIER_NAME;
 import static se.bjurr.prnfs.settings.SettingsStorage.fakeRandom;
@@ -87,9 +89,9 @@ public static PrnfsTestBuilder prnfsTestBuilder() {
 
  private TransactionTemplate transactionTemplate;
 
- private final List<String> usedPassword = newArrayList();
+ private final List<Optional<String>> usedPassword = newArrayList();
 
- private final List<String> usedUser = newArrayList();
+ private final List<Optional<String>> usedUser = newArrayList();
 
  private final UserKey userKey;
 
@@ -123,8 +125,8 @@ public PrnfsTestBuilder delete(String id) {
 
  public void didNotUseBasicAuth() {
   for (int i = 0; i < usedUser.size(); i++) {
-   assertTrue("user" + i, usedUser.get(i).isEmpty());
-   assertTrue("password" + i, usedPassword.get(i).isEmpty());
+   assertFalse("user" + i + " \"" + usedUser.get(i).orNull() + "\" password" + i + " \"" + usedUser.get(i).orNull()
+     + "\"", shouldUseBasicAuth(usedUser.get(i), usedPassword.get(i)));
   }
  }
 
@@ -192,7 +194,12 @@ public PrnfsTestBuilder invokedOnlyUrl(String url) {
  }
 
  public PrnfsTestBuilder invokedPassword(String password) {
-  assertTrue(on(" ").join(usedPassword), this.usedPassword.contains(password));
+  for (Optional<String> opt : usedPassword) {
+   if (password.equals(opt.orNull())) {
+    return this;
+   }
+  }
+  fail("Found: " + on(" ").join(usedPassword));
   return this;
  }
 
@@ -206,7 +213,12 @@ public PrnfsTestBuilder invokedUrl(String url) {
  }
 
  public PrnfsTestBuilder invokedUser(String user) {
-  assertTrue(on(" ").join(usedUser), this.usedUser.contains(user));
+  for (Optional<String> opt : usedUser) {
+   if (user.equals(opt.orNull())) {
+    return this;
+   }
+  }
+  fail("Found: " + on(" ").join(usedPassword));
   return this;
  }
 
@@ -238,8 +250,8 @@ public PrnfsTestBuilder trigger(PullRequestEvent event) {
    @Override
    public void ivoke(String url, Optional<String> userParam, Optional<String> passwordParam) {
     invokedUrl.add(url);
-    usedUser.add(userParam.or(""));
-    usedPassword.add(passwordParam.or(""));
+    usedUser.add(userParam);
+    usedPassword.add(passwordParam);
    }
   });
   listener.handleEvent(event);