Added HTTP authentication to HTTPServer

[dhoard]

Added HTTP authentication to HTTPServer. Once merged, this will allow adding HTTP authentication in jmx_exporter.

[dhoard]

Closing. Creating a new PR to target next-release

[fstab]

Thanks a lot for the PR. I re-opened this one, because the one against the next-release branch had some unrelated commits. I will review it and get back to you.

[fstab]

Note: I am currently working on a PR for adding a metric filter, so that users can define which metrics should be collected. The current work in progress adds an optional metricNameFilter parameter to the HTTPServer constructors https://github.com/prometheus/client_java/blob/be68c458e7a3567af6645f3922098ecbb518f50b/simpleclient_httpserver/src/main/java/io/prometheus/client/exporter/HTTPServer.java. This already results in a lot of different parameter combinations. If we now add an optional Authenticator parameter the number of constructors will explode.

There are different ways to avoid adding more and more parameters to the HTTPServer constructor. For example, we could create a configuration object that can be passed to the HTTPServer constructor. Or we could implement a Builder. I'll finish my work in progress on #680 first and then we'll see what's a good way for adding new parameters to the HTTPServer constructor.

[dhoard]

I agree/my preference would be to use a Builder pattern. I tried to implement the change in a similar way to the existing code without the refactor to prevent any possible dependent projects (Prometheus and others) from having to change.

Also, a Builder pattern would allow adding an SSLContext to be able to add SSL. Authentication and SSL are requirements (adoption blockers) for the jmx_exporter for most Enterprise customers.

[fstab]

I merged the PR with the HTTPServer.Builder yesterday. So you can now rebase this one to the current master and use the Builder for configuring HTTP authentication. Thanks a lot!

[dhoard]

@fstab code merged, but looks like test failures in unchanged code. I suspect it's around the performance of the build machine since I don't see the issues on my local development machine.

[fstab]

It's looking good, thanks a lot, I'm happy to merge it. There's only one code review remark: TestHTTPServerBasicAuthentication has a lot of copy-and-paste from TestHTTPServer. It would be good to delete the class TestHTTPServerBasicAuthentication and add tests for authentication to TestHTTPServer instead. This is easier with the current version of TestHTTPServer because I removed the global HTTPServer instance. Something like this would do (plus your helper methods from TestHTTPServerBasicAuthentication):

  Authenticator makeAuthenticator(String realm, final String validUsername, final String validPassword) {
    return new BasicAuthenticator(realm) {
      @Override
      public boolean checkCredentials(String username, String password) {
        return validUsername.equals(username) && validPassword.equals(password);
      }
    };
  }

  @Test
  public void testBasicAuthSuccess() throws IOException {
    HTTPServer s = new HTTPServer.Builder()
            .withRegistry(registry)
            .withAuthenticator(makeAuthenticator("/", "user", "secret"))
            .build();
    try {
      String response = requestWithCredentials(s, "?name[]=a&name[]=b", "/metrics", "user", "secret");
      assertThat(response).contains("a 0.0");
    } finally {
      s.close();
    }
  }

  @Test
  public void testBasicAuthCredentialsMissing() throws IOException {
    HTTPServer s = new HTTPServer.Builder()
            .withRegistry(registry)
            .withAuthenticator(makeAuthenticator("/", "user", "secret"))
            .build();
    try {
      request(s, "?name[]=a&name[]=b", "/metrics");
      Assert.fail("expected IOException with HTTP 401");
    } catch (IOException e) {
      Assert.assertTrue(e.getMessage().contains("401"));
    } finally {
      s.close();
    }
  }

  @Test
  public void testBasicAuthWrongCredentials() throws IOException {
    HTTPServer s = new HTTPServer.Builder()
            .withRegistry(registry)
            .withAuthenticator(makeAuthenticator("/", "user", "wrong"))
            .build();
    try {
      request(s, "?name[]=a&name[]=b", "/metrics");
      Assert.fail("expected IOException with HTTP 401");
    } catch (IOException e) {
      Assert.assertTrue(e.getMessage().contains("401"));
    } finally {
      s.close();
    }
  }

One more tiny thing: Please rebase the PR to the current master, because if there are no merge commits in a PR I find it easier to see what changed.

[dhoard]

@fstab Apologies for the mess... PR should be cleaned up.

[fstab]

Thanks a lot!