Hi, please review the attached patches. -Salvatore.
commit cdeddefc56797e46ec087a980a72f5670e6df882 Author: Salvatore Iovene <[email protected]> WebDavSource.cpp: hijack error 404 to 401 when appropriate. If we get a 404 error while contacting the server, it might mean that the username was wrong, so the server gave us a not found error. It's better to let the user know that, because we don't have a clear heuristic to determin whether this might have been a true 404 error. The convertion of 404 errors to 401 should happen only if the URL we're trying to open is one in which it was us who injected the username into the URL. This was achieved by removing the username injection from the context creation code, and moving it into the loop that does the autodiscovery, adding it path by path as it was necessary. Notice: this required NeonCXX to be aware of the "%u" semantic, something I'm not completely comfortable with. See also: https://bugs.meego.com/show_bug.cgi?id=17862 commit 8c55193d34400a2e94089d9fa2e750866c491515 Author: Salvatore Iovene <[email protected]> NeonCXX: don't trust libneon's escape and unescape functions. commit b700c67b0c8500d673a2e3fa08445a4bc60f7996 Author: Salvatore Iovene <[email protected]> NeonCXX: rename check to checkError. Using a name that gives better context. -- Salvatore Iovene <[email protected]> Linux Software Engineer Intel Open Source Technology Center, Finland Tel.: +358504804026
>From b700c67b0c8500d673a2e3fa08445a4bc60f7996 Mon Sep 17 00:00:00 2001 From: Salvatore Iovene <[email protected]> Date: Mon, 23 May 2011 12:04:06 +0300 Subject: [PATCH 1/3] NeonCXX: rename check to checkError. Using a name that gives better context. --- src/backends/webdav/NeonCXX.cpp | 12 ++++++------ src/backends/webdav/NeonCXX.h | 16 ++++++++-------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/backends/webdav/NeonCXX.cpp b/src/backends/webdav/NeonCXX.cpp index dcabf00..7097011 100644 --- a/src/backends/webdav/NeonCXX.cpp +++ b/src/backends/webdav/NeonCXX.cpp @@ -367,7 +367,7 @@ int Session::sslVerify(void *userdata, int failures, const ne_ssl_certificate *c unsigned int Session::options(const std::string &path) { unsigned int caps; - check(ne_options2(m_session, path.c_str(), &caps)); + checkError(ne_options2(m_session, path.c_str(), &caps)); return caps; } #endif // HAVE_LIBNEON_OPTIONS @@ -405,7 +405,7 @@ void Session::propfindURI(const std::string &path, int depth, const char *tmp = ne_get_response_header(req, "Location"); std::string location(tmp ? tmp : ""); - if (!check(error, status->code, status, location)) { + if (!checkError(error, status->code, status, location)) { goto retry; } } @@ -480,7 +480,7 @@ void Session::flush() } } -bool Session::check(int error, int code, const ne_status *status, const string &location) +bool Session::checkError(int error, int code, const ne_status *status, const string &location) { flush(); @@ -822,7 +822,7 @@ bool Request::run() error = ne_xml_dispatch_request(m_req, m_parser->get()); } - return check(error); + return checkError(error); } int Request::addResultData(void *userdata, const char *buf, size_t len) @@ -832,9 +832,9 @@ int Request::addResultData(void *userdata, const char *buf, size_t len) return 0; } -bool Request::check(int error) +bool Request::checkError(int error) { - return m_session.check(error, getStatus()->code, getStatus(), getResponseHeader("Location")); + return m_session.checkError(error, getStatus()->code, getStatus(), getResponseHeader("Location")); } } diff --git a/src/backends/webdav/NeonCXX.h b/src/backends/webdav/NeonCXX.h index f4b5407..67210ef 100644 --- a/src/backends/webdav/NeonCXX.h +++ b/src/backends/webdav/NeonCXX.h @@ -277,7 +277,7 @@ class Session { * * call sequence is this: * - startOperation() - * - repeat until success or final failure: create request, run(), check() + * - repeat until success or final failure: create request, run(), checkError() * * @param operation internal descriptor for debugging (for example, PROPFIND) * @param deadline time at which the operation must be completed, otherwise it'll be considered failed; @@ -287,7 +287,7 @@ class Session { /** * to be called after each operation which might have produced debugging output by neon; - * automatically called by check() + * automatically called by checkError() */ void flush(); @@ -303,7 +303,7 @@ class Session { * @return true for success, false if retry needed (only if deadline not empty); * errors reported via exceptions */ - bool check(int error, int code = 0, const ne_status *status = NULL, + bool checkError(int error, int code = 0, const ne_status *status = NULL, const string &location = ""); ne_session *getSession() const { return m_session; } @@ -321,9 +321,9 @@ class Session { ne_session *m_session; URI m_uri; std::string m_proxyURL; - /** time when last successul request completed, maintained by check() */ + /** time when last successul request completed, maintained by checkError() */ Timespec m_lastRequestEnd; - /** number of times a request was sent, maintained by startOperation(), the credentials callback, and check() */ + /** number of times a request was sent, maintained by startOperation(), the credentials callback, and checkError() */ int m_attempt; /** ne_set_server_auth() callback */ @@ -485,10 +485,10 @@ class Request /** * Execute the request. May only be called once per request. Uses - * Session::check() underneath to detect fatal errors and throw + * Session::checkError() underneath to detect fatal errors and throw * exceptions. * - * @return result of Session::check() + * @return result of Session::checkError() */ bool run(); @@ -514,7 +514,7 @@ class Request static int addResultData(void *userdata, const char *buf, size_t len); /** throw error if error code *or* current status indicates failure */ - bool check(int error); + bool checkError(int error); }; /** thrown for 301 HTTP status */ -- 1.7.2.2
>From 8c55193d34400a2e94089d9fa2e750866c491515 Mon Sep 17 00:00:00 2001 From: Salvatore Iovene <[email protected]> Date: Tue, 21 Jun 2011 04:11:25 -0700 Subject: [PATCH 2/3] NeonCXX: don't trust libneon's escape and unescape functions. --- src/backends/webdav/NeonCXX.cpp | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/backends/webdav/NeonCXX.cpp b/src/backends/webdav/NeonCXX.cpp index 7097011..5752a8c 100644 --- a/src/backends/webdav/NeonCXX.cpp +++ b/src/backends/webdav/NeonCXX.cpp @@ -120,13 +120,15 @@ std::string URI::toURL() const std::string URI::escape(const std::string &text) { SmartPtr<char *> tmp(ne_path_escape(text.c_str())); - return tmp.get(); + // Fail gracefully. + return tmp ? tmp.get() : text; } std::string URI::unescape(const std::string &text) { SmartPtr<char *> tmp(ne_path_unescape(text.c_str())); - return tmp.get(); + // Fail gracefully. + return tmp ? tmp.get() : text; } std::string URI::normalizePath(const std::string &path, bool collection) -- 1.7.2.2
>From cdeddefc56797e46ec087a980a72f5670e6df882 Mon Sep 17 00:00:00 2001 From: Salvatore Iovene <[email protected]> Date: Mon, 23 May 2011 12:04:51 +0300 Subject: [PATCH 3/3] WebDavSource.cpp: hijack error 404 to 401 when appropriate. If we get a 404 error while contacting the server, it might mean that the username was wrong, so the server gave us a not found error. It's better to let the user know that, because we don't have a clear heuristic to determin whether this might have been a true 404 error. The convertion of 404 errors to 401 should happen only if the URL we're trying to open is one in which it was us who injected the username into the URL. This was achieved by removing the username injection from the context creation code, and moving it into the loop that does the autodiscovery, adding it path by path as it was necessary. Notice: this required NeonCXX to be aware of the "%u" semantic, something I'm not completely comfortable with. See also: https://bugs.meego.com/show_bug.cgi?id=17862 --- src/backends/webdav/NeonCXX.cpp | 12 ++++++++++- src/backends/webdav/WebDAVSource.cpp | 36 ++++++++++++++++++++++++++++----- 2 files changed, 41 insertions(+), 7 deletions(-) diff --git a/src/backends/webdav/NeonCXX.cpp b/src/backends/webdav/NeonCXX.cpp index 5752a8c..0f4cf06 100644 --- a/src/backends/webdav/NeonCXX.cpp +++ b/src/backends/webdav/NeonCXX.cpp @@ -140,7 +140,17 @@ std::string URI::normalizePath(const std::string &path, bool collection) string_split_iterator it = boost::make_split_iterator(path, boost::first_finder("/", boost::is_iequal())); while (!it.eof()) { - res += escape(unescape(std::string(it->begin(), it->end()))); + std::string split(it->begin(), it->end()); + // Let's have an exception here for "%u", since we use that to replace the + // actual username into the path. It's safe to ignore "%u" because it + // couldn't be in a valid URI anyway. + // TODO: we should find a neat way to remove the awareness of "%u" from + // NeonCXX. + std::string normalizedSplit = split; + if (split != "%u") { + normalizedSplit = escape(unescape(split)); + } + res += normalizedSplit; ++it; if (!it.eof()) { res += '/'; diff --git a/src/backends/webdav/WebDAVSource.cpp b/src/backends/webdav/WebDAVSource.cpp index fe2b40f..cb26fee 100644 --- a/src/backends/webdav/WebDAVSource.cpp +++ b/src/backends/webdav/WebDAVSource.cpp @@ -8,6 +8,7 @@ #include <boost/algorithm/string/replace.hpp> #include <boost/algorithm/string/predicate.hpp> #include <boost/algorithm/string/classification.hpp> +#include <boost/algorithm/string/find.hpp> #include <boost/scoped_ptr.hpp> #include <syncevo/LogRedirect.h> @@ -45,12 +46,14 @@ public: { if (m_context) { vector<string> urls = m_context->getSyncURL(); + string urlWithUsername; + if (!urls.empty()) { - m_url = urls.front(); + m_url = urlWithUsername = urls.front(); std::string username = m_context->getSyncUsername(); - boost::replace_all(m_url, "%u", Neon::URI::escape(username)); + boost::replace_all(urlWithUsername, "%u", Neon::URI::escape(username)); } - Neon::URI uri = Neon::URI::parse(m_url); + Neon::URI uri = Neon::URI::parse(urlWithUsername); typedef boost::split_iterator<string::iterator> string_split_iterator; for (string_split_iterator arg = boost::make_split_iterator(uri.m_query, boost::first_finder("&", boost::is_iequal())); @@ -77,13 +80,13 @@ public: } else { SE_THROW(StringPrintf("unknown SyncEvolution flag %s in URL %s", std::string(flag->begin(), flag->end()).c_str(), - m_url.c_str())); + urlWithUsername.c_str())); } } } else if (arg->end() != arg->begin()) { SE_THROW(StringPrintf("unknown parameter %s in URL %s", std::string(arg->begin(), arg->end()).c_str(), - m_url.c_str())); + urlWithUsername.c_str())); } } boost::shared_ptr<FilterConfigNode> node = m_context->getNode(WebDAVCredentialsOkay); @@ -449,8 +452,18 @@ void WebDAVSource::contactServer() Timespec finalDeadline = createDeadline(); // no resending if left empty while (true) { + bool usernameInserted = false; std::string next; + // Replace %u with the username, if the %u is found. Also, keep track + // of this event happening, because if we later on get a 404 error, + // we will convert it to 401 only if the path contains the username + // and it was indeed us who put the username there (not the server). + if (boost::find_first(path, "%u")) { + boost::replace_all(path, "%u", Neon::URI::escape(username)); + usernameInserted = true; + } + // must normalize so that we can compare against results from server path = Neon::URI::normalizePath(path, true); SE_LOG_DEBUG(NULL, NULL, "testing %s", path.c_str()); @@ -573,6 +586,17 @@ void WebDAVSource::contactServer() } else { candidates.push_front(next.m_path); } + } catch (const TransportStatusException &ex) { + SE_LOG_DEBUG(NULL, NULL, "TransportStatusException: %s", ex.what()); + if (ex.syncMLStatus() == 404 && boost::find_first(path, username) && usernameInserted) { + // We're actually looking at an authentication error: the path to the calendar has + // not been found, so the username was wrong. Let's hijack the error message and + // code of the exception by throwing a new one. + string descr = StringPrintf("Path not found: %s. Is the username '%s' correct?", + path.c_str(), username.c_str()); + int code = 401; + SE_THROW_EXCEPTION_STATUS(TransportStatusException, descr, SyncMLStatus(code)); + } } catch (const Exception &ex) { if (candidates.empty()) { // nothing left to try, bail out with this error @@ -678,7 +702,7 @@ void WebDAVSource::contactServer() if (next.empty()) { // use next candidate if (candidates.empty()) { - throwError(StringPrintf("no collection found in %s", m_settings->getURL().c_str())); + throwError(StringPrintf("no collection found in %s", path.c_str())); } next = candidates.front(); candidates.pop_front(); -- 1.7.2.2
_______________________________________________ SyncEvolution mailing list [email protected] http://lists.syncevolution.org/listinfo/syncevolution
