Fix mock server tests

.github/workflows/ci.yml

@@ -4,6 +4,8 @@ name: Continuous Integration
 on:
   workflow_dispatch:
   push:
+    branches:
+      - 'main'
   pull_request:
 
 jobs:

apache/client/include/lauth/http_client.hpp

@@ -1,6 +1,7 @@
 #ifndef __LAUTH_HTTP_CLIENT_HPP__
 #define __LAUTH_HTTP_CLIENT_HPP__
 
+#include <optional>
 #include <string>
 
 namespace mlibrary::lauth {
@@ -9,7 +10,7 @@ namespace mlibrary::lauth {
       HttpClient(const std::string& baseUrl) : baseUrl(baseUrl) {};
       virtual ~HttpClient() = default;
 
-      virtual std::string get(const std::string& path); 
+      virtual std::optional<std::string> get(const std::string &path);
 
     protected:
       const std::string baseUrl;

apache/client/src/lauth/api_client.cpp

@@ -7,7 +7,7 @@ namespace mlibrary::lauth {
   bool ApiClient::isAllowed(Request req) {
     std::stringstream url;
     url << "/users/" << req.user << "/is_allowed";
-    std::string result = client->get(url.str());
+    auto result = client->get(url.str());
     return result == "yes";
   }
 }

apache/client/src/lauth/http_client.cpp

@@ -2,13 +2,17 @@
 
 #include <httplib.h>
 
+#include <optional>
+
 namespace mlibrary::lauth {
-  std::string HttpClient::get(const std::string& path) {
+  std::optional<std::string> HttpClient::get(const std::string& path) {
     httplib::Client client(baseUrl);
 
     auto res = client.Get(path);
-
-    return res ? res->body : "";
+    if (res)
+      return res->body;
+    else
+      return std::nullopt;
   }
 }
 

apache/client/test/lauth/api_client_test.cpp

@@ -1,16 +1,18 @@
 #include <gtest/gtest.h>
 #include <gmock/gmock.h>
+
 #include "mocks.hpp"
 
 using ::testing::_;
+using ::testing::AnyNumber;
 using ::testing::Return;
 
 #include "lauth/api_client.hpp"
 #include "lauth/request.hpp"
 
 using namespace mlibrary::lauth;
 
-TEST(ApiClient, allowed_by_mock_http_client) {
+TEST(ApiClient, RequestByAuthorizedUserIsAllowed) {
   auto client = std::make_unique<MockHttpClient>();
   EXPECT_CALL(*client, get("/users/authorized/is_allowed")).WillOnce(Return("yes"));
   ApiClient api_client(std::move(client));
@@ -26,7 +28,7 @@ TEST(ApiClient, allowed_by_mock_http_client) {
   EXPECT_THAT(allowed, true);
 }
 
-TEST(ApiClient, denied_by_mock_http_client) {
+TEST(ApiClient, RequestByUnauthorizedUserIsDenied) {
   auto client = std::make_unique<MockHttpClient>();
   EXPECT_CALL(*client, get("/users/unauthorized/is_allowed")).WillOnce(Return("no"));
   ApiClient api_client(std::move(client));
@@ -42,29 +44,24 @@ TEST(ApiClient, denied_by_mock_http_client) {
   EXPECT_THAT(allowed, false);
 }
 
-TEST(ApiClient, a_request_with_no_user_is_denied) {
-  ApiClient client("http://localhost:9000");
+TEST(ApiClient, RequestByUnknownUserIsDenied) {
+  GTEST_SKIP() << "This is passing for the wrong reason and GMock is giving "
+               << "a warning because we have not set any expectations. "
+               << "There is an uninteresting/unexpected call to the HttpClient "
+               << "to get '/users//is_allowed', which should be rejected "
+               << "before the HTTP request or expressed differently (possibly "
+               << "with query params).";
+  auto http_client = std::make_unique<MockHttpClient>();
+  ApiClient client(std::move(http_client));
   Request request;
 
   bool result = client.isAllowed(request);
   EXPECT_THAT(result, false);
 }
 
-
-TEST(ApiClient, a_request_with_authorized_user_is_allowed) {
-  ApiClient client("http://localhost:9000");
-  Request request;
-
-  request.user = "authorized";
-  bool result = client.isAllowed(request);
-  EXPECT_THAT(result, true);
-}
-
-TEST(ApiClient, a_request_with_unauthorized_user_is_denied) {
-  ApiClient client("http://localhost:9000");
-  Request request;
-
-  request.user = "unauthorized";
-  bool result = client.isAllowed(request);
-  EXPECT_THAT(result, false);
-}
+TEST(ApiClient, UsesTheSuppliedApiUrl) {
+  GTEST_SKIP() << "Skipping test that ApiClient makes an HttpClient for the "
+               << "correct URL... pushing everything back to config and likely "
+               << "a factory/builder rather than concrete class dependency.";
+  ApiClient client("http://api.invalid");
+}

apache/client/test/lauth/http_client_test.cpp

@@ -1,27 +1,53 @@
 #include <gtest/gtest.h>
 #include <gmock/gmock.h>
 
-using testing::_;
-
 #include <httplib.h>
 
+#include "mocks.hpp"
+
 #include "lauth/http_client.hpp"
 #include "lauth/request.hpp"
 
+#include <string>
+#include <cstdlib>
+
+using testing::_;
+using testing::Eq;
+using testing::IsTrue;
+
 using namespace mlibrary::lauth;
 
-const std::string api_url = "http://localhost:9000";
+const std::string LOCAL_MOCK_API_URL = "http://localhost:9000";
+
+const static std::string& MOCK_API_URL() {
+    static std::string url;
+    if (url.empty()) {
+        if (const char *env_url = std::getenv("LAUTH_TEST_API_URL"))
+            url = std::string(env_url);
+        else
+            url = LOCAL_MOCK_API_URL;
+    }
+    return url;
+}
+
+TEST(HttpClient, uses_mock_server) {
+  HttpClient client(MOCK_API_URL());
+
+  auto response = client.get("/");
+  ASSERT_THAT(response.has_value(), IsTrue()) << "Mock server does not appear responsive at "
+    << MOCK_API_URL() << ". Is it running? Have you set LAUTH_TEST_API_URL correctly?";
+}
 
 TEST(HttpClient, get_request_returns_body) {
-  HttpClient client(api_url);
+  HttpClient client(MOCK_API_URL());
 
-  std::string response = client.get("/");
-  EXPECT_THAT(response, "Root");
+  auto response = client.get("/");
+  EXPECT_THAT(response, Eq("Root"));
 }
 
 TEST(HttpClient, get_request_with_path_returns_body) {
-  HttpClient client(api_url);
+  HttpClient client(MOCK_API_URL());
 
-  std::string response = client.get("/ping");
+  auto response = client.get("/ping");
   EXPECT_THAT(response, "pong");
 }

apache/client/test/lauth/mocks.hpp

@@ -4,18 +4,20 @@
 #include "lauth/api_client.hpp"
 #include "lauth/http_client.hpp"
 
+#include <optional>
+
 using namespace mlibrary::lauth;
 
-class MockApiClient : public ApiClient {
+class MockHttpClient : public HttpClient {
     public:
-    MockApiClient() : ApiClient("http://localhost:9000") {};
-    MOCK_METHOD(bool, isAllowed, (Request), (override));
+    MockHttpClient() : HttpClient("http://api.invalid") {};
+    MOCK_METHOD(std::optional<std::string>, get, (const std::string&), (override));
 };
 
-class MockHttpClient : public HttpClient {
+class MockApiClient : public ApiClient {
     public:
-    MockHttpClient() : HttpClient("http://localhost:9000") {};
-    MOCK_METHOD(std::string, get, (const std::string&), (override));
+    MockApiClient() : ApiClient(std::make_unique<MockHttpClient>()) {};
+    MOCK_METHOD(bool, isAllowed, (Request), (override));
 };
 
 #endif

apache/client/test/mock_service.cpp

@@ -15,18 +15,22 @@ int main(int argc, char **argv) {
   }
 
   server.Get("/", [](const Request &, Response &res) {
+    std::cout << "GET /" << std::endl;
     res.set_content("Root", "text/plain");
   });
 
   server.Get("/ping", [](const Request &, Response &res) {
+    std::cout << "GET /ping" << std::endl;
     res.set_content("pong", "text/plain");
   });
 
   server.Get("/users/authorized/is_allowed", [](const Request &, Response &res) {
+    std::cout << "GET /users/authorized/is_allowed" << std::endl;
     res.set_content("yes", "text/plain");
   });
 
   server.Get("/users/unauthorized/is_allowed", [](const Request &, Response &res) {
+    std::cout << "GET /users/unauthorized/is_allowed" << std::endl;
     res.set_content("no", "text/plain");
   });
 
@@ -37,7 +41,8 @@ int main(int argc, char **argv) {
 
   server.Get("/users/:id/is_allowed", [](const Request &req, Response &res) {
     auto user_id = req.path_params.at("id");
-    std::cout << "Got request for /users/:id/is_allowed" << std::endl;
+    std::cout << "GET /users/:id/is_allowed -- "
+              << "binding :id as '" << user_id << "'" << std::endl;
     res.set_content("no", "text/plain");
   });
 

apache/module/mod_lauth.cpp

@@ -44,7 +44,7 @@ static authz_status lauth_check_authorization(request_rec *r,
       .user = r->user ? std::string(r->user) : ""
     };
 
-    return Authorizer("http://api-mock:9000").isAllowed(req) ? AUTHZ_GRANTED : AUTHZ_DENIED;
+    return Authorizer("http://mock-api:9000").isAllowed(req) ? AUTHZ_GRANTED : AUTHZ_DENIED;
 }
 
 static const authz_provider authz_lauth_provider =

docker-compose.yml

@@ -82,6 +82,8 @@ services:
       context: ./
       dockerfile: ./apache/Dockerfile
       target: tests
+    environment:
+      - LAUTH_TEST_API_URL=http://mock-api:9000
     volumes:
       - ./apache/client:/lauth/apache/client
       - ./apache/module:/lauth/apache/module
@@ -93,6 +95,10 @@ services:
       context: ./
       dockerfile: ./apache/Dockerfile
       target: tests
+    hostname: mock-api.lauth.local
+    networks:
+      default:
+        aliases: ["mock-api"]
     ports:
       - "127.0.0.1:9000:9000"
     healthcheck:
@@ -105,15 +111,17 @@ services:
     command: ["./http-service", "9000"]
 
   client-tests:
-    profiles: [ "test" ]
-    depends_on:
-      mock-api:
-        condition:
-          "service_healthy"
+    profiles: [ "test", "integration" ]
     build:
       context: ./
       dockerfile: ./apache/Dockerfile
       target: client-tests
+    depends_on:
+      mock-api:
+        condition:
+          "service_healthy"
+    environment:
+      - LAUTH_TEST_API_URL=http://mock-api.lauth.local:9000
 
   test:
     profiles: ["test"]