Auto-update secure.config OAuth token when needed.
GitHub OAuth can decide to expire an existing token
and generate a new one, based on time and requested
scope change: this may impact existing credentials
stored in secure.config and then block replication
to GitHub repos.
With this enhancement the secure.config is checked
at login and updated if needed.
NOTE: we could potentially have concurrency issues
(even if the update is a synchronised method) because
of replication auto-config with concurrent OAuth token
updates. FileBasedConfig already takes care of File-level
locking on save.
Change-Id: I41fe6eb8020c2fdecad90232ddd27af5f8a995d7
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/ConcurrentFileBasedConfigWriteException.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/ConcurrentFileBasedConfigWriteException.java
new file mode 100644
index 0000000..c620429
--- /dev/null
+++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/ConcurrentFileBasedConfigWriteException.java
@@ -0,0 +1,24 @@
+// Copyright (C) 2013 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.googlesource.gerrit.plugins.github.oauth;
+
+import java.io.IOException;
+
+public class ConcurrentFileBasedConfigWriteException extends IOException {
+ private static final long serialVersionUID = -4019991986934953417L;
+
+ public ConcurrentFileBasedConfigWriteException(String message) {
+ super(message);
+ }
+}
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfig.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfig.java
index 6645b69..a5a12ef 100644
--- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfig.java
+++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/GitHubOAuthConfig.java
@@ -51,6 +51,8 @@
public final String gitHubOAuthAccessTokenUrl;
public final boolean enabled;
public final List<OAuthProtocol.Scope> scopes;
+ public final int fileUpdateMaxRetryCount;
+ public final int fileUpdateMaxRetryIntervalMsec;
@Inject
public GitHubOAuthConfig(@GerritServerConfig Config config)
@@ -73,6 +75,9 @@
config.getString("auth", null, "type").equalsIgnoreCase(
AuthType.HTTP.toString());
scopes = parseScopes(config.getString(CONF_SECTION, null, "scopes"));
+
+ fileUpdateMaxRetryCount = config.getInt(CONF_SECTION, "fileUpdateMaxRetryCount", 3);
+ fileUpdateMaxRetryIntervalMsec = config.getInt(CONF_SECTION, "fileUpdateMaxRetryIntervalMsec", 3000);
}
private String dropTrailingSlash(String url) {
@@ -87,7 +92,7 @@
scopes.add(Enum.valueOf(Scope.class, scope));
}
}
-
+
return scopes;
}
diff --git a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthFilter.java b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthFilter.java
index ac4340b..548847b 100644
--- a/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthFilter.java
+++ b/github-oauth/src/main/java/com/googlesource/gerrit/plugins/github/oauth/OAuthFilter.java
@@ -15,6 +15,10 @@
import java.io.IOException;
import java.net.HttpURLConnection;
+import java.text.SimpleDateFormat;
+import java.util.Date;
+import java.util.Random;
+import java.util.Set;
import javax.servlet.Filter;
import javax.servlet.FilterChain;
@@ -28,10 +32,15 @@
import javax.servlet.http.HttpSession;
import org.apache.http.client.HttpClient;
+import org.eclipse.jgit.errors.ConfigInvalidException;
+import org.eclipse.jgit.storage.file.FileBasedConfig;
+import org.eclipse.jgit.util.FS;
import org.kohsuke.github.GHMyself;
+import org.kohsuke.github.GitHub;
import org.slf4j.LoggerFactory;
import com.google.common.base.Strings;
+import com.google.gerrit.server.config.SitePaths;
import com.google.gson.Gson;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -45,14 +54,17 @@
private final GitHubOAuthConfig config;
private final OAuthCookieProvider cookieProvider;
private final OAuthProtocol oauth;
+ private final Random retryRandom = new Random(System.currentTimeMillis());
+ private SitePaths sites;
@Inject
- public OAuthFilter(GitHubOAuthConfig config) {
+ public OAuthFilter(GitHubOAuthConfig config, SitePaths sites) {
this.config = config;
this.cookieProvider = new OAuthCookieProvider(new TokenCipher());
HttpClient httpClient;
httpClient = new GitHubHttpProvider().get();
this.oauth = new OAuthProtocol(config, httpClient, new Gson());
+ this.sites = sites;
}
@Override
@@ -85,14 +97,19 @@
|| (authCookie == null && gerritCookie == null)) {
if (oauth.isOAuthFinal(httpRequest)) {
+ GitHubLogin hubLogin = oauth.loginPhase2(httpRequest, httpResponse);
+ GitHub hub = hubLogin.hub;
GHMyself myself =
- oauth.loginPhase2(httpRequest, httpResponse).hub.getMyself();
+ hub.getMyself();
String user = myself.getLogin();
String email = myself.getEmail();
String fullName =
Strings.emptyToNull(myself.getName()) == null ? user : myself
.getName();
+ updateSecureConfigWithRetry(hub.getMyOrganizations().keySet(), user,
+ hubLogin.token.access_token);
+
if (user != null) {
OAuthCookie userCookie =
cookieProvider.getFromUser(user, email, fullName);
@@ -148,6 +165,80 @@
}
}
+ private void updateSecureConfigWithRetry(Set<String> organisations,
+ String user, String access_token) {
+ int retryCount = 0;
+
+ while (retryCount < config.fileUpdateMaxRetryCount) {
+ try {
+ updateSecureConfig(organisations, user, access_token);
+ return;
+ } catch (IOException e) {
+ retryCount++;
+ int retryInterval =
+ retryRandom.nextInt(config.fileUpdateMaxRetryIntervalMsec);
+ log.warn("Error whilst trying to update " + sites.secure_config
+ + (retryCount < config.fileUpdateMaxRetryCount ? ": attempt #" + retryCount + " will be retried after " + retryInterval + " msecs":""), e);
+ try {
+ Thread.sleep(retryInterval);
+ } catch (InterruptedException e1) {
+ log.error("Thread has been cancelled before retrying to save "
+ + sites.secure_config);
+ return;
+ }
+ } catch (ConfigInvalidException e) {
+ log.error("Cannot update " + sites.secure_config
+ + " as the file is corrupted", e);
+ return;
+ }
+ }
+ }
+
+ private synchronized void updateSecureConfig(Set<String> organisations,
+ String user, String access_token) throws IOException,
+ ConfigInvalidException {
+ FileBasedConfig currentSecureConfig =
+ new FileBasedConfig(sites.secure_config, FS.DETECTED);
+ long currentSecureConfigUpdateTs = sites.secure_config.lastModified();
+ currentSecureConfig.load();
+
+ boolean configUpdate = updateConfigSection(currentSecureConfig, user, user, access_token);
+ for (String organisation : organisations) {
+ configUpdate |= updateConfigSection(currentSecureConfig, organisation, user, access_token);
+ }
+
+ if(!configUpdate) {
+ return;
+ }
+
+ log.info("Updating " + sites.secure_config + " credentials for user " + user);
+
+ if (sites.secure_config.lastModified() != currentSecureConfigUpdateTs) {
+ throw new ConcurrentFileBasedConfigWriteException("File "
+ + sites.secure_config + " was written at "
+ + formatTS(sites.secure_config.lastModified())
+ + " while was trying to update security for user " + user);
+ }
+ currentSecureConfig.save();
+ }
+
+ private String formatTS(long ts) {
+ return new SimpleDateFormat("yyyy-MM-dd HH:mm:ss").format(new Date(ts));
+ }
+
+ private boolean updateConfigSection(FileBasedConfig config,
+ String section, String user, String password) {
+ String configUser = config.getString("remote", section, "username");
+ String configPassword = config.getString("remote", section, "password");
+ if(configUser == null || !configUser.equals(user) || configPassword.equals(password)) {
+ return false;
+ }
+
+ config.setString("remote", section, "username", user);
+ config.setString("remote", section, "password", password);
+ return true;
+ }
+
private Cookie getGerritCookie(HttpServletRequest httpRequest) {
for (Cookie cookie : httpRequest.getCookies()) {
if (cookie.getName().equalsIgnoreCase(GERRIT_COOKIE_NAME)) {
