From 6da4278af6649aeda48d4bcf1ad17dbbc1d92ab4 Mon Sep 17 00:00:00 2001 From: Alberto Mardegan Date: Fri, 28 Nov 2014 17:46:40 +0200 Subject: [PATCH 2/5] AuthProvider: removed failure count Remove the failure count from the getOAuth2Bearer() method, and add an invalidateCachedSecrets() method instead. This moves the logic of how to deal with failures back to the AuthProvider backend and simplifies the session code, which only needs to call invalidateCachedSecrets() when the token is wrong. This will help implementing a similar logic for the getCredentials() method, where authentication errors could lead to requesting a new password. --- src/backends/goa/goa.cpp | 5 ++--- src/backends/oauth2/oauth2.cpp | 16 +++++++-------- src/backends/signon/signon-accounts.cpp | 36 +++++++++++++++++++++++---------- src/backends/signon/signon.cpp | 32 ++++++++++++++++++++--------- src/backends/webdav/NeonCXX.cpp | 36 ++++++++++++++++----------------- src/backends/webdav/NeonCXX.h | 8 -------- src/syncevo/IdentityProvider.cpp | 4 ++-- src/syncevo/IdentityProvider.h | 25 ++++++++++++----------- 8 files changed, 88 insertions(+), 74 deletions(-) diff --git a/src/backends/goa/goa.cpp b/src/backends/goa/goa.cpp index 45b7604..8781773 100644 --- a/src/backends/goa/goa.cpp +++ b/src/backends/goa/goa.cpp @@ -197,10 +197,9 @@ public: virtual bool methodIsSupported(AuthMethod method) const { return method == AUTH_METHOD_OAUTH2; } - virtual Credentials getCredentials() const { SE_THROW("only OAuth2 is supported"); } + virtual Credentials getCredentials() { SE_THROW("only OAuth2 is supported"); } - virtual std::string getOAuth2Bearer(int failedTokens, - const PasswordUpdateCallback &passwordUpdateCallback) const + virtual std::string getOAuth2Bearer(const PasswordUpdateCallback &passwordUpdateCallback) { m_account->m_ensureCredentials(); std::string token = m_account->m_getAccessToken(); diff --git a/src/backends/oauth2/oauth2.cpp b/src/backends/oauth2/oauth2.cpp index 45eb717..087fe52 100644 --- a/src/backends/oauth2/oauth2.cpp +++ b/src/backends/oauth2/oauth2.cpp @@ -37,7 +37,7 @@ class RefreshTokenAuthProvider : public AuthProvider std::string m_clientID; std::string m_clientSecret; std::string m_refreshToken; - mutable std::string m_accessToken; + std::string m_accessToken; public: RefreshTokenAuthProvider(const char* tokenHost, @@ -63,16 +63,11 @@ public: virtual bool methodIsSupported(AuthMethod method) const { return method == AUTH_METHOD_OAUTH2; } - virtual Credentials getCredentials() const { SE_THROW("only OAuth2 is supported"); } + virtual Credentials getCredentials() { SE_THROW("only OAuth2 is supported"); } - virtual std::string getOAuth2Bearer(int failedTokens, - const PasswordUpdateCallback &passwordUpdateCallback) const + virtual std::string getOAuth2Bearer(const PasswordUpdateCallback &passwordUpdateCallback) { - SE_LOG_DEBUG(NULL, "retrieving OAuth2 token, attempt %d", failedTokens); - //in case of retry do not use cached access token, request it again - if (1 >= failedTokens) { - m_accessToken.clear(); - } + SE_LOG_DEBUG(NULL, "retrieving OAuth2 token"); if (m_accessToken.empty()) { const char *reply; @@ -156,6 +151,9 @@ public: } return m_accessToken; } + + virtual void invalidateCachedSecrets() { m_accessToken.clear(); } + virtual std::string getUsername() const { return ""; } }; diff --git a/src/backends/signon/signon-accounts.cpp b/src/backends/signon/signon-accounts.cpp index 68c83b8..33116f2 100644 --- a/src/backends/signon/signon-accounts.cpp +++ b/src/backends/signon/signon-accounts.cpp @@ -65,6 +65,8 @@ class SignonAuthProvider : public AuthProvider SignonAuthSessionCXX m_authSession; GHashTableCXX m_sessionData; std::string m_mechanism; + std::string m_accessToken; + bool m_invalidateCache; public: SignonAuthProvider(const SignonAuthSessionCXX &authSession, @@ -72,21 +74,28 @@ public: const std::string &mechanism) : m_authSession(authSession), m_sessionData(sessionData), - m_mechanism(mechanism) + m_mechanism(mechanism), + m_invalidateCache(false) {} virtual bool methodIsSupported(AuthMethod method) const { return method == AUTH_METHOD_OAUTH2; } - virtual Credentials getCredentials() const { SE_THROW("only OAuth2 is supported"); } + virtual Credentials getCredentials() { SE_THROW("only OAuth2 is supported"); } - virtual std::string getOAuth2Bearer(int failedTokens, - const PasswordUpdateCallback &passwordUpdateCallback) const + virtual std::string getOAuth2Bearer(const PasswordUpdateCallback &passwordUpdateCallback) { - SE_LOG_DEBUG(NULL, "retrieving OAuth2 token, attempt %d", failedTokens); + SE_LOG_DEBUG(NULL, "retrieving OAuth2 token"); + + if (!m_accessToken.empty() && !m_invalidateCache) { + return m_accessToken.c_str(); + } + + if (m_invalidateCache) { + // Retry login if even the refreshed token failed. + g_hash_table_insert(m_sessionData, g_strdup("ForceTokenRefresh"), + g_variant_ref_sink(g_variant_new_boolean(true))); + } - // Retry login if even the refreshed token failed. - g_hash_table_insert(m_sessionData, g_strdup("UiPolicy"), - g_variant_ref_sink(g_variant_new_uint32(failedTokens >= 2 ? SIGNON_POLICY_REQUEST_PASSWORD : 0))); // We get assigned a plain pointer to an instance that we'll own, // so we have to use the "steal" variant to enable that assignment. GVariantStealCXX resultDataVar; @@ -115,13 +124,18 @@ public: if (!tokenVar) { SE_THROW("no AccessToken in OAuth2 response"); } - const char *token = g_variant_get_string(tokenVar, NULL); - if (!token) { + std::string newToken = g_variant_get_string(tokenVar, NULL); + if (newToken.empty()) { SE_THROW("AccessToken did not contain a string value"); + } else if (m_invalidateCache && newToken == m_accessToken) { + SE_THROW("Got the same invalid AccessToken"); } - return token; + m_accessToken = newToken; + return m_accessToken.c_str(); } + virtual void invalidateCachedSecrets() { m_invalidateCache = true; } + virtual std::string getUsername() const { return ""; } }; diff --git a/src/backends/signon/signon.cpp b/src/backends/signon/signon.cpp index 18761db..129a307 100644 --- a/src/backends/signon/signon.cpp +++ b/src/backends/signon/signon.cpp @@ -52,6 +52,8 @@ class SignonAuthProvider : public AuthProvider SignonAuthSessionCXX m_authSession; GHashTableCXX m_sessionData; std::string m_mechanism; + std::string m_accessToken; + bool m_invalidateCache; public: SignonAuthProvider(const SignonAuthSessionCXX &authSession, @@ -59,21 +61,26 @@ public: const std::string &mechanism) : m_authSession(authSession), m_sessionData(sessionData), - m_mechanism(mechanism) + m_mechanism(mechanism), + m_invalidateCache(false) {} virtual bool methodIsSupported(AuthMethod method) const { return method == AUTH_METHOD_OAUTH2; } - virtual Credentials getCredentials() const { SE_THROW("only OAuth2 is supported"); } + virtual Credentials getCredentials() { SE_THROW("only OAuth2 is supported"); } - virtual std::string getOAuth2Bearer(int failedTokens, - const PasswordUpdateCallback &passwordUpdateCallback) const + virtual std::string getOAuth2Bearer(const PasswordUpdateCallback &passwordUpdateCallback) { - SE_LOG_DEBUG(NULL, "retrieving OAuth2 token, attempt %d", failedTokens); + SE_LOG_DEBUG(NULL, "retrieving OAuth2 token"); + + if (!m_accessToken.empty() && !m_invalidateCache) { + return m_accessToken.c_str(); + } // Retry login if even the refreshed token failed. - g_hash_table_insert(m_sessionData, g_strdup("UiPolicy"), - g_variant_ref_sink(g_variant_new_uint32(failedTokens >= 2 ? SIGNON_POLICY_REQUEST_PASSWORD : 0))); + g_hash_table_insert(m_sessionData, g_strdup("ForceTokenRefresh"), + g_variant_ref_sink(g_variant_new_boolean(m_invalidateCache))); + // We get assigned a plain pointer to an instance that we'll own, // so we have to use the "steal" variant to enable that assignment. GVariantStealCXX resultDataVar; @@ -102,13 +109,18 @@ public: if (!tokenVar) { SE_THROW("no AccessToken in OAuth2 response"); } - const char *token = g_variant_get_string(tokenVar, NULL); - if (!token) { + std::string newToken = g_variant_get_string(tokenVar, NULL); + if (newToken.empty()) { SE_THROW("AccessToken did not contain a string value"); + } else if (m_invalidateCache && newToken == m_accessToken) { + SE_THROW("Got the same invalid AccessToken"); } - return token; + m_accessToken = newToken; + return m_accessToken.c_str(); } + virtual void invalidateCachedSecrets() { m_invalidateCache = true; } + virtual std::string getUsername() const { return ""; } }; diff --git a/src/backends/webdav/NeonCXX.cpp b/src/backends/webdav/NeonCXX.cpp index 676679a..60fa572 100644 --- a/src/backends/webdav/NeonCXX.cpp +++ b/src/backends/webdav/NeonCXX.cpp @@ -191,7 +191,6 @@ std::string Status2String(const ne_status *status) Session::Session(const boost::shared_ptr &settings) : m_forceAuthorizationOnce(false), m_credentialsSent(false), - m_oauthTokenRejections(0), m_settings(settings), m_debugging(false), m_session(NULL), @@ -640,29 +639,29 @@ bool Session::checkError(int error, int code, const ne_status *status, SE_LOG_DEBUG(NULL, "credentials accepted"); m_settings->setCredentialsOkay(true); } - m_oauthTokenRejections = 0; return true; } break; case NE_AUTH: { - // Retry OAuth2-based request if we still have a valid token. - bool useOAuth2 = m_authProvider && m_authProvider->methodIsSupported(AuthProvider::AUTH_METHOD_OAUTH2); - if (useOAuth2) { - // Try again with new token? Need to restore the counter, - // because it is relevant for getOAuth2Bearer() in preSend(). - if (m_oauthTokenRejections < 2) { - if (!m_oauth2Bearer.empty() && m_credentialsSent) { - SE_LOG_DEBUG(NULL, "discarding used and rejected OAuth2 token '%s'", m_oauth2Bearer.c_str()); - m_oauthTokenRejections++; - m_oauth2Bearer.clear(); - } else { - SE_LOG_DEBUG(NULL, "OAuth2 token '%s' not used?!", m_oauth2Bearer.c_str()); - } + if (m_authProvider) { + // The m_oauth2Bearer is empty if the getOAuth2Bearer() method + // raised an exception, and in that case we should not retry + // invoking that method again. + if (!m_oauth2Bearer.empty()) { retry = true; - SE_LOG_DEBUG(NULL, "OAuth2 retry after %d failed tokens", m_oauthTokenRejections); + } + + // If we have been using this OAuth token and we got NE_AUTH, it + // means that the token is invalid (probably it's expired); we must + // tell the AuthProvider to invalidate its cache so that next time + // we'll hopefully get a new working token. + if (m_credentialsSent) { + SE_LOG_DEBUG(NULL, "discarding used and rejected OAuth2 token '%s'", m_oauth2Bearer.c_str()); + m_authProvider->invalidateCachedSecrets(); + m_oauth2Bearer.clear(); } else { - SE_LOG_DEBUG(NULL, "too many failed OAuth2 tokens, giving up"); + SE_LOG_DEBUG(NULL, "OAuth2 token '%s' not used?!", m_oauth2Bearer.c_str()); } } @@ -957,8 +956,7 @@ void Session::checkAuthorization() // Count the number of times we asked for new tokens. This helps // the provider determine whether the token that it returns are valid. try { - m_oauth2Bearer = m_authProvider->getOAuth2Bearer(m_oauthTokenRejections, - boost::bind(&Settings::updatePassword, m_settings, _1)); + m_oauth2Bearer = m_authProvider->getOAuth2Bearer(boost::bind(&Settings::updatePassword, m_settings, _1)); SE_LOG_DEBUG(NULL, "got new OAuth2 token '%s' for next request", m_oauth2Bearer.c_str()); } catch (...) { std::string explanation; diff --git a/src/backends/webdav/NeonCXX.h b/src/backends/webdav/NeonCXX.h index 7bfb7cc..c82e983 100644 --- a/src/backends/webdav/NeonCXX.h +++ b/src/backends/webdav/NeonCXX.h @@ -270,14 +270,6 @@ class Session { bool m_credentialsSent; /** - * Count the number of consecutive times that an OAuth2 token - * failed to get accepted. This can happen when the current one - * expired and needs to be refreshed or we need re-authorization - * by the user. - */ - int m_oauthTokenRejections; - - /** * Cached token for OAuth2. Obtained before starting the request in run(), * used in preSend(), invalidated when it caused an authentication error * in checkError(). diff --git a/src/syncevo/IdentityProvider.cpp b/src/syncevo/IdentityProvider.cpp index d36b6ff..62e8b75 100644 --- a/src/syncevo/IdentityProvider.cpp +++ b/src/syncevo/IdentityProvider.cpp @@ -60,8 +60,8 @@ public: virtual bool wasConfigured() const { return !m_creds.m_username.empty() || !m_creds.m_password.empty(); } virtual bool methodIsSupported(AuthMethod method) const { return method == AUTH_METHOD_CREDENTIALS; } - virtual Credentials getCredentials() const { return m_creds; } - virtual std::string getOAuth2Bearer(int failedTokens, const PasswordUpdateCallback &passwordUpdateCallback) const { SE_THROW("OAuth2 not supported"); return ""; } + virtual Credentials getCredentials() { return m_creds; } + virtual std::string getOAuth2Bearer(const PasswordUpdateCallback &passwordUpdateCallback) { SE_THROW("OAuth2 not supported"); return ""; } virtual std::string getUsername() const { return m_creds.m_username; } }; diff --git a/src/syncevo/IdentityProvider.h b/src/syncevo/IdentityProvider.h index 4ca7beb..392e104 100644 --- a/src/syncevo/IdentityProvider.h +++ b/src/syncevo/IdentityProvider.h @@ -83,7 +83,7 @@ class AuthProvider /** * Returns username/password credentials. Throws an error if not supported. */ - virtual Credentials getCredentials() const = 0; + virtual Credentials getCredentials() = 0; /** * Returns the 'Bearer b64token' string required for logging into @@ -95,24 +95,25 @@ class AuthProvider * * An application should: * - request a token and try to use it - * - in case of failure try to request a token again, and try to use it - * again (in case the first token has expired just before using it) - * - if the second token also fails, request a third token with full - * re-authentication as above. - * - if that fails, then give up. + * - in case the token is not working (expired), call + * invalidateCachedSecrets() and then this method again. + * - if this method raises an exception, give up. * - * To achieve that, the caller must count how often he got a token that - * did not work. - * - * @param failedTokens zero when asking for initial token, one for refresh, two for full re-authorization * @param passwordUpdateCallback callback function to be called when stored refresh token need to be updated. * Only parameter of this callback function is new value of refresh token. * * @return a base64 encoded token, ready to be used in "Authorization: Bearer %s" */ typedef boost::function PasswordUpdateCallback; - virtual std::string getOAuth2Bearer(int failedTokens, - const PasswordUpdateCallback &passwordUpdateCallback) const = 0; + virtual std::string getOAuth2Bearer(const PasswordUpdateCallback &passwordUpdateCallback) = 0; + + /** + * Informs the AuthProvider that the password or authentication token is + * wrong. If the AuthProvider keeps it in a cache, the next time that it's + * being asked for a password or token it should attempt to obtain a new + * value. + */ + virtual void invalidateCachedSecrets() {} /** * Returns username at the remote service. Works for -- 2.1.3