diff --git a/CMakeLists.txt b/CMakeLists.txt index 014be80..dfe2d0c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -44,3 +44,12 @@ install(FILES "${CMAKE_CURRENT_BINARY_DIR}/oatpp-authkit-config.cmake" "${CMAKE_CURRENT_BINARY_DIR}/oatpp-authkit-config-version.cmake" DESTINATION ${CMAKE_INSTALL_LIBDIR}/cmake/oatpp-authkit) + +# ─── Tests ─────────────────────────────────────────────────────────────────── +# Off by default so consumers pulling us in via FetchContent don't pay the +# cost. Enable with -DOATPP_AUTHKIT_BUILD_TESTS=ON. +option(OATPP_AUTHKIT_BUILD_TESTS "Build oatpp-authkit unit tests" OFF) +if(OATPP_AUTHKIT_BUILD_TESTS) + enable_testing() + add_subdirectory(test) +endif() diff --git a/README.md b/README.md index dc385cb..1af340b 100644 --- a/README.md +++ b/README.md @@ -34,6 +34,43 @@ find_package(oatpp-authkit 0.1 REQUIRED) target_link_libraries(app PRIVATE oatpp::authkit) ``` +## Browser-friendly 401/403 + +By default `AuthInterceptor` returns `application/json` for every rejection, +which is correct for `/api/*` callers but breaks browser navigation: a user +following a stale link or an expired password-reset URL sees a raw +`{"status":"Unauthorized"}` instead of a real page. + +Override `IAuthPolicy::unauthenticatedRedirect(path)` to redirect browser +navigations to a login or landing page while keeping JSON responses for +`fetch`/`axios` callers (detected via path prefix `/api/`, +`X-Requested-With: XMLHttpRequest`, or an `Accept` header that prefers +`application/json`): + +```cpp +class AppAuthPolicy : public oatpp_authkit::IAuthPolicy { +public: + std::optional + unauthenticatedRedirect(const std::string& path) override { + return "/?next=" + oatpp_authkit::AuthInterceptor::urlEncode(path); + } +}; +``` + +Returning `std::nullopt` (the default) preserves the legacy JSON behaviour +for all responses. + +## Tests + +```bash +cmake -B build -DOATPP_AUTHKIT_BUILD_TESTS=ON +cmake --build build +ctest --test-dir build --output-on-failure +``` + +Tests are off by default so consumers pulling the library in via +`FetchContent` don't pay the cost. + ## Roadmap - **v0.2** — `AuthInterceptor` + `requireAdmin` ported onto three seams diff --git a/include/oatpp-authkit/auth/AuthInterceptor.hpp b/include/oatpp-authkit/auth/AuthInterceptor.hpp index cc2a4e5..f40fb0b 100644 --- a/include/oatpp-authkit/auth/AuthInterceptor.hpp +++ b/include/oatpp-authkit/auth/AuthInterceptor.hpp @@ -52,13 +52,101 @@ private: r->putHeader("Content-Type", "application/json"); return r; } - std::shared_ptr makeUnauthorized() { - return makeJsonError(Status::CODE_401, "{\"status\":\"Unauthorized\"}"); + + std::shared_ptr makeHtmlError(Status status, const std::string& title) { + std::string body = "" + + title + "

" + title + "

"; + auto r = ResponseFactory::createResponse(status, body.c_str()); + r->putHeader("Content-Type", "text/html; charset=utf-8"); + return r; } - std::shared_ptr makeForbidden(const std::string& msg = "") { - if (msg.empty()) return makeJsonError(Status::CODE_403, "{\"status\":\"Forbidden\"}"); - return makeJsonError(Status::CODE_403, - "{\"status\":\"Forbidden\",\"message\":\"" + msg + "\"}"); + + std::shared_ptr makeRedirect(const std::string& location) { + auto r = ResponseFactory::createResponse(Status::CODE_302, ""); + r->putHeader("Location", location.c_str()); + r->putHeader("Cache-Control", "no-store"); + return r; + } + +public: + /** + * @brief Heuristic: does this caller expect a JSON error body? + * + * True when any of: + * - path begins with `/api/` (API surface — always JSON) + * - `X-Requested-With: XMLHttpRequest` (jQuery/axios/explicit AJAX) + * - `Accept` mentions `application/json` and does NOT prefer `text/html` + * + * Otherwise treated as a browser navigation that should get HTML or a + * redirect. Exposed as a static so the negotiation rule is unit-testable + * without spinning up a request. + */ + static bool wantsJson(const std::string& path, + const std::string& xRequestedWith, + const std::string& accept) + { + if (path.size() >= 5 && path.compare(0, 5, "/api/") == 0) return true; + if (!xRequestedWith.empty()) return true; + bool hasJson = accept.find("application/json") != std::string::npos; + bool hasHtml = accept.find("text/html") != std::string::npos; + if (hasJson && !hasHtml) return true; + return false; + } + + /** + * @brief Percent-encode the unreserved subset for use in a `next=` param. + * Static + side-effect-free so consumers and tests can reuse it. + */ + static std::string urlEncode(const std::string& s) { + static const char* hex = "0123456789ABCDEF"; + std::string out; + out.reserve(s.size()); + for (unsigned char c : s) { + if ((c >= 'A' && c <= 'Z') || (c >= 'a' && c <= 'z') || + (c >= '0' && c <= '9') || c == '-' || c == '_' || c == '.' || c == '~') { + out.push_back(static_cast(c)); + } else { + out.push_back('%'); + out.push_back(hex[c >> 4]); + out.push_back(hex[c & 0xF]); + } + } + return out; + } + +private: + bool requestWantsJson(const std::shared_ptr& req, + const std::string& path) + { + auto xrw = req->getHeader("X-Requested-With"); + auto accept = req->getHeader("Accept"); + return wantsJson(path, + xrw ? *xrw : std::string{}, + accept ? *accept : std::string{}); + } + + std::shared_ptr makeUnauthorized( + const std::shared_ptr& req, const std::string& path) + { + if (requestWantsJson(req, path)) + return makeJsonError(Status::CODE_401, "{\"status\":\"Unauthorized\"}"); + if (auto loc = m_policy->unauthenticatedRedirect(path)) + return makeRedirect(*loc); + return makeHtmlError(Status::CODE_401, "Unauthorized"); + } + + std::shared_ptr makeForbidden( + const std::shared_ptr& req, const std::string& path, + const std::string& msg = "") + { + if (requestWantsJson(req, path)) { + if (msg.empty()) return makeJsonError(Status::CODE_403, "{\"status\":\"Forbidden\"}"); + return makeJsonError(Status::CODE_403, + "{\"status\":\"Forbidden\",\"message\":\"" + msg + "\"}"); + } + if (auto loc = m_policy->unauthenticatedRedirect(path)) + return makeRedirect(*loc); + return makeHtmlError(Status::CODE_403, "Forbidden"); } void writeBundle(const std::shared_ptr& req, const AuthPrincipal& p) { @@ -127,7 +215,7 @@ public: writeBundle(request, *p); if (isReadonly(p->role) && isMutation(method)) { logEvent(403, method, path, "readonly cert user mutation"); - return makeForbidden(); + return makeForbidden(request, path); } return nullptr; } @@ -137,7 +225,7 @@ public: std::string token = extractToken(request); if (token.empty()) { logEvent(401, method, path, "no token"); - return makeUnauthorized(); + return makeUnauthorized(request, path); } std::string hash = m_hashToken(token); @@ -149,7 +237,7 @@ public: viaSession = false; } else { logEvent(401, method, path, "invalid token"); - return makeUnauthorized(); + return makeUnauthorized(request, path); } // CSRF defence-in-depth: session cookie + mutation requires X-Requested-With. @@ -157,7 +245,7 @@ public: auto xrw = request->getHeader("X-Requested-With"); if (!xrw || xrw->empty()) { logEvent(403, method, path, "missing X-Requested-With"); - return makeForbidden("Missing X-Requested-With header"); + return makeForbidden(request, path, "Missing X-Requested-With header"); } } @@ -165,7 +253,7 @@ public: if (isReadonly(p->role) && isMutation(method)) { logEvent(403, method, path, "readonly user mutation"); - return makeForbidden(); + return makeForbidden(request, path); } return nullptr; } diff --git a/include/oatpp-authkit/auth/IAuthPolicy.hpp b/include/oatpp-authkit/auth/IAuthPolicy.hpp index 7718e27..458981f 100644 --- a/include/oatpp-authkit/auth/IAuthPolicy.hpp +++ b/include/oatpp-authkit/auth/IAuthPolicy.hpp @@ -1,6 +1,7 @@ #ifndef OATPP_AUTHKIT_AUTH_IAUTH_POLICY_HPP #define OATPP_AUTHKIT_AUTH_IAUTH_POLICY_HPP +#include #include #include @@ -40,6 +41,27 @@ public: * the presence of a `SETUP_MODE` sentinel file. */ virtual bool setupModeActive() { return false; } + + /** + * @brief Where to send a browser navigation that hits a 401/403, instead + * of the default JSON error body. + * + * Returning `std::nullopt` (the default) keeps the legacy behaviour: + * `AuthInterceptor` always responds with `application/json`. Returning a + * URL makes the interceptor emit a `302` with `Location:` set whenever + * the request looks like a browser navigation (HTML `Accept`, no + * `X-Requested-With`, path outside `/api/`). API callers (`fetch`, + * `axios`, anything sending `X-Requested-With: XMLHttpRequest` or + * targeting `/api/*`) still receive the JSON 401/403 so client-side + * error handling keeps working. + * + * The same hook covers both 401 (no/invalid auth) and 403 (logged in + * but not allowed) — typical wiring is to bounce both to `/` or + * `/login?next=...`. Consumers that need to differentiate can branch + * inside the override. + */ + virtual std::optional + unauthenticatedRedirect(const std::string& /*path*/) { return std::nullopt; } }; } // namespace oatpp_authkit diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt new file mode 100644 index 0000000..ad728cc --- /dev/null +++ b/test/CMakeLists.txt @@ -0,0 +1,11 @@ +# Minimal test harness for oatpp-authkit. +# +# Adds plain executable tests linked against the INTERFACE library and oatpp. +# No third-party test framework — assertions use and a tiny REQUIRE +# macro so the suite stays portable and dependency-free. + +find_package(oatpp REQUIRED) + +add_executable(test_negotiation test_negotiation.cpp) +target_link_libraries(test_negotiation PRIVATE oatpp::authkit oatpp::oatpp) +add_test(NAME negotiation COMMAND test_negotiation) diff --git a/test/test_negotiation.cpp b/test/test_negotiation.cpp new file mode 100644 index 0000000..1b6d57c --- /dev/null +++ b/test/test_negotiation.cpp @@ -0,0 +1,77 @@ +// Tests for AuthInterceptor::wantsJson + urlEncode (the negotiation primitives +// that decide whether a 401/403 returns JSON vs HTML/redirect). +// +// Kept dependency-free on purpose — the harness exists so future tests have +// somewhere to land, not to pull in doctest/Catch2. + +#include "oatpp-authkit/auth/AuthInterceptor.hpp" + +#include +#include +#include + +namespace { + +int g_failures = 0; + +#define REQUIRE(expr) do { \ + if (!(expr)) { \ + std::fprintf(stderr, "FAIL %s:%d %s\n", __FILE__, __LINE__, #expr); \ + ++g_failures; \ + } \ +} while (0) + +using oatpp_authkit::AuthInterceptor; + +void test_wantsJson_api_path() { + // /api/* always wants JSON, no matter what Accept says. + REQUIRE( AuthInterceptor::wantsJson("/api/users", "", "text/html")); + REQUIRE( AuthInterceptor::wantsJson("/api/", "", "")); +} + +void test_wantsJson_xrequested_with() { + // Explicit AJAX wins regardless of path/Accept. + REQUIRE( AuthInterceptor::wantsJson("/admin", "XMLHttpRequest", "text/html")); +} + +void test_wantsJson_accept_header() { + // application/json without text/html → JSON. + REQUIRE( AuthInterceptor::wantsJson("/admin", "", "application/json")); + // text/html present → browser navigation. + REQUIRE(!AuthInterceptor::wantsJson("/admin", "", "text/html,application/xhtml+xml")); + REQUIRE(!AuthInterceptor::wantsJson("/admin", "", "text/html,application/json")); + // No Accept → assume browser (HTML/redirect). + REQUIRE(!AuthInterceptor::wantsJson("/set-password", "", "")); +} + +void test_wantsJson_set_password_browser() { + // The motivating regression: a browser following the password-reset link + // must NOT be served JSON. (Path is public so it shouldn't reach this in + // normal flow, but if auth ever rejects it the user sees HTML/redirect.) + REQUIRE(!AuthInterceptor::wantsJson("/set-password", + "", + "text/html,application/xhtml+xml,application/xml;q=0.9")); +} + +void test_urlEncode() { + REQUIRE(AuthInterceptor::urlEncode("/admin") == "%2Fadmin"); + REQUIRE(AuthInterceptor::urlEncode("/set-password?t=1")== "%2Fset-password%3Ft%3D1"); + REQUIRE(AuthInterceptor::urlEncode("abc-_.~123") == "abc-_.~123"); + REQUIRE(AuthInterceptor::urlEncode(" ") == "%20"); +} + +} // namespace + +int main() { + test_wantsJson_api_path(); + test_wantsJson_xrequested_with(); + test_wantsJson_accept_header(); + test_wantsJson_set_password_browser(); + test_urlEncode(); + if (g_failures) { + std::fprintf(stderr, "%d test(s) failed\n", g_failures); + return 1; + } + std::puts("ok"); + return 0; +}