Merge pull request #869 from kiwix/userlang_cookie_fixes

src/server/request_context.cpp

@@ -68,12 +68,13 @@ fullURL2LocalURL(const std::string& full_url, const std::string& rootLocation)
 } // unnamed namespace
 
 RequestContext::RequestContext(struct MHD_Connection* connection,
-                               std::string rootLocation,
+                               std::string _rootLocation,
                                const std::string& _url,
                                const std::string& _method,
                                const std::string& version) :
+  rootLocation(_rootLocation),
   full_url(_url),
-  url(fullURL2LocalURL(_url, rootLocation)),
+  url(fullURL2LocalURL(_url, _rootLocation)),
   method(str2RequestMethod(_method)),
   version(version),
   requestIndex(s_requestIndex++),
@@ -193,6 +194,10 @@ std::string RequestContext::get_full_url() const {
   return full_url;
 }
 
+std::string RequestContext::get_root_path() const {
+  return rootLocation.empty() ? "/" : rootLocation;
+}
+
 bool RequestContext::is_valid_url() const {
   return !url.empty();
 }
@@ -212,26 +217,32 @@ std::string RequestContext::get_header(const std::string& name) const {
 
 std::string RequestContext::get_user_language() const
 {
-  return userlang;
+  return userlang.lang;
 }
 
-std::string RequestContext::determine_user_language() const
+bool RequestContext::user_language_comes_from_cookie() const
+{
+  return userlang.selectedBy == UserLanguage::SelectorKind::COOKIE;
+}
+
+RequestContext::UserLanguage RequestContext::determine_user_language() const
 {
   try {
-    return get_argument("userlang");
+    return {UserLanguage::SelectorKind::QUERY_PARAM, get_argument("userlang")};
   } catch(const std::out_of_range&) {}
 
   try {
-     return cookies.at("userlang");
+     return {UserLanguage::SelectorKind::COOKIE, cookies.at("userlang")};
   } catch(const std::out_of_range&) {}
 
   try {
     const std::string acceptLanguage = get_header("Accept-Language");
     const auto userLangPrefs = parseUserLanguagePreferences(acceptLanguage);
-    return selectMostSuitableLanguage(userLangPrefs);
+    const auto lang = selectMostSuitableLanguage(userLangPrefs);
+    return {UserLanguage::SelectorKind::ACCEPT_LANGUAGE_HEADER, lang};
   } catch(const std::out_of_range&) {}
 
-  return "en";
+  return {UserLanguage::SelectorKind::DEFAULT, "en"};
 }
 
 std::string RequestContext::get_requested_format() const

src/server/request_context.h

@@ -91,6 +91,7 @@ class RequestContext {
     std::string get_url() const;
     std::string get_url_part(int part) const;
     std::string get_full_url() const;
+    std::string get_root_path() const;
 
     std::string get_query() const { return queryString; }
 
@@ -118,7 +119,25 @@ class RequestContext {
     std::string get_user_language() const;
     std::string get_requested_format() const;
 
+    bool user_language_comes_from_cookie() const;
+
+  private: // types
+    struct UserLanguage
+    {
+      enum SelectorKind
+      {
+        QUERY_PARAM,
+        COOKIE,
+        ACCEPT_LANGUAGE_HEADER,
+        DEFAULT
+      };
+
+      SelectorKind selectedBy;
+      std::string  lang;
+    };
+
   private: // data
+    std::string rootLocation;
     std::string full_url;
     std::string url;
     RequestMethod method;
@@ -132,10 +151,10 @@ class RequestContext {
     std::map<std::string, std::vector<std::string>> arguments;
     std::map<std::string, std::string> cookies;
     std::string queryString;
-    std::string userlang;
+    UserLanguage userlang;
 
   private: // functions
-    std::string determine_user_language() const;
+    UserLanguage determine_user_language() const;
 
     static MHD_Result fill_header(void *, enum MHD_ValueKind, const char*, const char*);
     static MHD_Result fill_cookie(void *, enum MHD_ValueKind, const char*, const char*);

src/server/response.cpp

@@ -387,8 +387,12 @@ MHD_Result Response::send(const RequestContext& request, MHD_Connection* connect
     MHD_add_response_header(response, p.first.c_str(), p.second.c_str());
   }
 
-  const std::string cookie = "userlang=" + request.get_user_language();
+  if ( ! request.user_language_comes_from_cookie() ) {
+    const std::string cookie = "userlang=" + request.get_user_language()
+                               + ";Path=" + request.get_root_path()
+                               + ";Max-Age=31536000";
     MHD_add_response_header(response, MHD_HTTP_HEADER_SET_COOKIE, cookie.c_str());
+  }
 
   if (m_returnCode == MHD_HTTP_OK && m_byteRange.kind() == ByteRange::RESOLVED_PARTIAL_CONTENT)
     m_returnCode = MHD_HTTP_PARTIAL_CONTENT;

test/server.cpp

@@ -1007,7 +1007,7 @@ TEST_F(ServerTest, UserLanguageControl)
       /*url*/ "/ROOT/content/zimfile/invalid-article",
       /*Accept-Language:*/ "",
       /*Request Cookie:*/       NO_COOKIE,
-      /*Response Set-Cookie:*/  "userlang=en",
+      /*Response Set-Cookie:*/  "userlang=en;Path=/ROOT;Max-Age=31536000",
       /* expected <h1> */ "Not Found"
     },
     {
@@ -1015,7 +1015,7 @@ TEST_F(ServerTest, UserLanguageControl)
       /*url*/ "/ROOT/content/zimfile/invalid-article?userlang=en",
       /*Accept-Language:*/ "",
       /*Request Cookie:*/       NO_COOKIE,
-      /*Response Set-Cookie:*/  "userlang=en",
+      /*Response Set-Cookie:*/  "userlang=en;Path=/ROOT;Max-Age=31536000",
       /* expected <h1> */ "Not Found"
     },
     {
@@ -1023,7 +1023,7 @@ TEST_F(ServerTest, UserLanguageControl)
       /*url*/ "/ROOT/content/zimfile/invalid-article?userlang=test",
       /*Accept-Language:*/ "",
       /*Request Cookie:*/       NO_COOKIE,
-      /*Response Set-Cookie:*/  "userlang=test",
+      /*Response Set-Cookie:*/  "userlang=test;Path=/ROOT;Max-Age=31536000",
       /* expected <h1> */ "[I18N TESTING] Content not found, but at least the server is alive"
     },
     {
@@ -1031,7 +1031,7 @@ TEST_F(ServerTest, UserLanguageControl)
       /*url*/ "/ROOT/content/zimfile/invalid-article",
       /*Accept-Language:*/ "*",
       /*Request Cookie:*/       NO_COOKIE,
-      /*Response Set-Cookie:*/  "userlang=en",
+      /*Response Set-Cookie:*/  "userlang=en;Path=/ROOT;Max-Age=31536000",
       /* expected <h1> */ "Not Found"
     },
     {
@@ -1039,7 +1039,7 @@ TEST_F(ServerTest, UserLanguageControl)
       /*url*/ "/ROOT/content/zimfile/invalid-article",
       /*Accept-Language:*/ "test",
       /*Request Cookie:*/       NO_COOKIE,
-      /*Response Set-Cookie:*/  "userlang=test",
+      /*Response Set-Cookie:*/  "userlang=test;Path=/ROOT;Max-Age=31536000",
       /* expected <h1> */ "[I18N TESTING] Content not found, but at least the server is alive"
     },
     {
@@ -1047,7 +1047,7 @@ TEST_F(ServerTest, UserLanguageControl)
       /*url*/ "/ROOT/content/zimfile/invalid-article",
       /*Accept-Language:*/ "",
       /*Request Cookie:*/       "userlang=test",
-      /*Response Set-Cookie:*/  "userlang=test",
+      /*Response Set-Cookie:*/  NO_COOKIE,
       /* expected <h1> */ "[I18N TESTING] Content not found, but at least the server is alive"
     },
     {
@@ -1055,7 +1055,7 @@ TEST_F(ServerTest, UserLanguageControl)
       /*url*/ "/ROOT/content/zimfile/invalid-article",
       /*Accept-Language:*/ "",
       /*Request Cookie:*/       "anothercookie=123; userlang=test",
-      /*Response Set-Cookie:*/  "userlang=test",
+      /*Response Set-Cookie:*/  NO_COOKIE,
       /* expected <h1> */ "[I18N TESTING] Content not found, but at least the server is alive"
     },
     {
@@ -1063,7 +1063,7 @@ TEST_F(ServerTest, UserLanguageControl)
       /*url*/ "/ROOT/content/zimfile/invalid-article",
       /*Accept-Language:*/ "",
       /*Request Cookie:*/       "userlang=test; anothercookie=abc",
-      /*Response Set-Cookie:*/  "userlang=test",
+      /*Response Set-Cookie:*/  NO_COOKIE,
       /* expected <h1> */ "[I18N TESTING] Content not found, but at least the server is alive"
     },
     {
@@ -1071,7 +1071,7 @@ TEST_F(ServerTest, UserLanguageControl)
       /*url*/ "/ROOT/content/zimfile/invalid-article",
       /*Accept-Language:*/ "",
       /*Request Cookie:*/       "cookie1=abc; userlang=test; cookie2=xyz",
-      /*Response Set-Cookie:*/  "userlang=test",
+      /*Response Set-Cookie:*/  NO_COOKIE,
       /* expected <h1> */ "[I18N TESTING] Content not found, but at least the server is alive"
     },
     {
@@ -1079,7 +1079,7 @@ TEST_F(ServerTest, UserLanguageControl)
       /*url*/ "/ROOT/content/zimfile/invalid-article",
       /*Accept-Language:*/ "",
       /*Request Cookie:*/       "cookie1=abc; userlang=en; userlang=test; cookie2=xyz",
-      /*Response Set-Cookie:*/  "userlang=test",
+      /*Response Set-Cookie:*/  NO_COOKIE,
       /* expected <h1> */ "[I18N TESTING] Content not found, but at least the server is alive"
     },
     {
@@ -1087,7 +1087,7 @@ TEST_F(ServerTest, UserLanguageControl)
       /*url*/ "/ROOT/content/zimfile/invalid-article?userlang=en",
       /*Accept-Language:*/ "test",
       /*Request Cookie:*/       NO_COOKIE,
-      /*Response Set-Cookie:*/  "userlang=en",
+      /*Response Set-Cookie:*/  "userlang=en;Path=/ROOT;Max-Age=31536000",
       /* expected <h1> */ "Not Found"
     },
     {
@@ -1095,7 +1095,7 @@ TEST_F(ServerTest, UserLanguageControl)
       /*url*/ "/ROOT/content/zimfile/invalid-article?userlang=en",
       /*Accept-Language:*/ "",
       /*Request Cookie:*/       "userlang=test",
-      /*Response Set-Cookie:*/  "userlang=en",
+      /*Response Set-Cookie:*/  "userlang=en;Path=/ROOT;Max-Age=31536000",
       /* expected <h1> */ "Not Found"
     },
     {
@@ -1103,19 +1103,29 @@ TEST_F(ServerTest, UserLanguageControl)
       /*url*/ "/ROOT/content/zimfile/invalid-article",
       /*Accept-Language:*/ "test",
       /*Request Cookie:*/       "userlang=en",
-      /*Response Set-Cookie:*/  "userlang=en",
+      /*Response Set-Cookie:*/  NO_COOKIE,
       /* expected <h1> */ "Not Found"
     },
     {
-      "The value of the Accept-Language header is not currently parsed.",
+      "Most suitable language is selected from the Accept-Language header",
       // In case of a comma separated list of languages (optionally weighted
-      // with quality values) the default (en) language is used instead.
+      // with quality values) the most suitable language is selected.
       /*url*/ "/ROOT/content/zimfile/invalid-article",
       /*Accept-Language:*/ "test;q=0.9, en;q=0.2",
       /*Request Cookie:*/       NO_COOKIE,
-      /*Response Set-Cookie:*/  "userlang=test",
+      /*Response Set-Cookie:*/  "userlang=test;Path=/ROOT;Max-Age=31536000",
       /* expected <h1> */ "[I18N TESTING] Content not found, but at least the server is alive"
     },
+    {
+      "Most suitable language is selected from the Accept-Language header",
+      // In case of a comma separated list of languages (optionally weighted
+      // with quality values) the most suitable language is selected.
+      /*url*/ "/ROOT/content/zimfile/invalid-article",
+      /*Accept-Language:*/ "test;q=0.2, en;q=0.9",
+      /*Request Cookie:*/       NO_COOKIE,
+      /*Response Set-Cookie:*/  "userlang=en;Path=/ROOT;Max-Age=31536000",
+      /* expected <h1> */ "Not Found"
+    },
   };
 
   const std::regex h1Regex("<h1>(.+)</h1>");
@@ -1130,6 +1140,7 @@ TEST_F(ServerTest, UserLanguageControl)
     }
     const auto r = zfs1_->GET(t.url.c_str(), headers);
     if ( t.responseSetCookie ) {
+      ASSERT_TRUE(r->has_header("Set-Cookie")) << t;
       EXPECT_EQ(t.responseSetCookie, getHeaderValue(r->headers, "Set-Cookie")) << t;
     } else {
       EXPECT_FALSE(r->has_header("Set-Cookie"));