From e9348c823a329f47302953f231b221e5eb1f197a Mon Sep 17 00:00:00 2001 From: Alberto Mardegan Date: Fri, 28 Nov 2014 17:46:40 +0200 Subject: [PATCH] AuthProvider: removed failure count Remove the failure count from the getOAuth2Bearer() method, and add an invalidateCachedSecrets() method instead. --- src/backends/goa/goa.cpp | 5 ++--- src/backends/oauth2/oauth2.cpp | 16 +++++++--------- src/backends/signon/signon.cpp | 36 +++++++++++++++++++++++++----------- src/backends/webdav/NeonCXX.cpp | 28 +++++++++------------------- src/backends/webdav/NeonCXX.h | 8 -------- src/syncevo/IdentityProvider.cpp | 4 ++-- src/syncevo/IdentityProvider.h | 25 +++++++++++++------------ 7 files changed, 58 insertions(+), 64 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.cpp b/src/backends/signon/signon.cpp index 164e7a5..3161aa9 100644 --- a/src/backends/signon/signon.cpp +++ b/src/backends/signon/signon.cpp @@ -71,6 +71,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, @@ -78,21 +80,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; @@ -121,13 +130,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..a5f6511 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,21 @@ 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) { + if (!m_oauth2Bearer.empty()) { retry = true; - SE_LOG_DEBUG(NULL, "OAuth2 retry after %d failed tokens", m_oauthTokenRejections); + } + 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 +948,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