Skip to content

Commit b779d25

Browse files
bitwisemanbitwiseman
committed
Remove GitHubClient.login field
1 parent 3a76281 commit b779d25

File tree

9 files changed

+204
-55
lines changed

9 files changed

+204
-55
lines changed

‎src/main/java/org/kohsuke/github/GitHub.java‎

Lines changed: 58 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,8 @@
2828
import com.infradna.tool.bridge_method_injector.WithBridgeMethods;
2929
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
3030
import org.kohsuke.github.authorization.AuthorizationProvider;
31+
import org.kohsuke.github.authorization.ImmutableAuthorizationProvider;
32+
import org.kohsuke.github.authorization.UserAuthorizationProvider;
3133
import org.kohsuke.github.internal.Previews;
3234

3335
import java.io.*;
@@ -115,23 +117,70 @@ public class GitHub {
115117
AuthorizationProvider authorizationProvider) throws IOException {
116118
if (authorizationProvider instanceof DependentAuthorizationProvider) {
117119
((DependentAuthorizationProvider) authorizationProvider).bind(this);
120+
} else if (authorizationProvider instanceof ImmutableAuthorizationProvider
121+
&& authorizationProvider instanceof UserAuthorizationProvider) {
122+
UserAuthorizationProvider provider = (UserAuthorizationProvider) authorizationProvider;
123+
if (provider.getLogin() == null && provider.getEncodedAuthorization() != null
124+
&& provider.getEncodedAuthorization().startsWith("token")) {
125+
authorizationProvider = new LoginLoadingUserAuthorizationProvider(provider, this);
126+
}
118127
}
119128

129+
users = new ConcurrentHashMap<>();
130+
orgs = new ConcurrentHashMap<>();
131+
120132
this.client = new GitHubHttpUrlConnectionClient(apiUrl,
121133
connector,
122134
rateLimitHandler,
123135
abuseLimitHandler,
124136
rateLimitChecker,
125-
(myself) -> setMyself(myself),
126137
authorizationProvider);
127-
users = new ConcurrentHashMap<>();
128-
orgs = new ConcurrentHashMap<>();
138+
139+
// Ensure we have the login if it is available
140+
// This preserves previously existing behavior. Consider removing in future.
141+
if (authorizationProvider instanceof LoginLoadingUserAuthorizationProvider) {
142+
((LoginLoadingUserAuthorizationProvider) authorizationProvider).getLogin();
143+
}
129144
}
130145

131146
private GitHub(GitHubClient client) {
132-
this.client = client;
133147
users = new ConcurrentHashMap<>();
134148
orgs = new ConcurrentHashMap<>();
149+
this.client = client;
150+
}
151+
152+
private static class LoginLoadingUserAuthorizationProvider implements UserAuthorizationProvider {
153+
private final GitHub gitHub;
154+
private final AuthorizationProvider authorizationProvider;
155+
private boolean loginLoaded = false;
156+
private String login;
157+
158+
LoginLoadingUserAuthorizationProvider(AuthorizationProvider authorizationProvider, GitHub gitHub) {
159+
this.gitHub = gitHub;
160+
this.authorizationProvider = authorizationProvider;
161+
}
162+
163+
@Override
164+
public String getEncodedAuthorization() throws IOException {
165+
return authorizationProvider.getEncodedAuthorization();
166+
}
167+
168+
@Override
169+
public String getLogin() {
170+
synchronized (this) {
171+
if (!loginLoaded) {
172+
loginLoaded = true;
173+
try {
174+
GHMyself u = gitHub.setMyself();
175+
if (u != null) {
176+
login = u.getLogin();
177+
}
178+
} catch (IOException e) {
179+
}
180+
}
181+
return login;
182+
}
183+
}
135184
}
136185

137186
public static abstract class DependentAuthorizationProvider implements AuthorizationProvider {
@@ -506,21 +555,18 @@ public GHRateLimit rateLimit() throws IOException {
506555
@WithBridgeMethods(value = GHUser.class)
507556
public GHMyself getMyself() throws IOException {
508557
client.requireCredential();
558+
return setMyself();
559+
}
560+
561+
private GHMyself setMyself() throws IOException {
509562
synchronized (this) {
510563
if (this.myself == null) {
511-
GHMyself u = createRequest().withUrlPath("/user").fetch(GHMyself.class);
512-
setMyself(u);
564+
this.myself = createRequest().withUrlPath("/user").fetch(GHMyself.class);
513565
}
514566
return myself;
515567
}
516568
}
517569

518-
private void setMyself(GHMyself myself) {
519-
synchronized (this) {
520-
this.myself = myself;
521-
}
522-
}
523-
524570
/**
525571
* Obtains the object that represents the named user.
526572
*

‎src/main/java/org/kohsuke/github/GitHubClient.java‎

Lines changed: 14 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@
1515
import java.time.temporal.ChronoUnit;
1616
import java.util.*;
1717
import java.util.concurrent.atomic.AtomicReference;
18-
import java.util.function.Consumer;
1918
import java.util.logging.Logger;
2019

2120
import javax.annotation.CheckForNull;
@@ -41,7 +40,6 @@ abstract class GitHubClient {
4140
* If timeout issues let's retry after milliseconds.
4241
*/
4342
static final int retryTimeoutMillis = 100;
44-
/* private */ final String login;
4543

4644
// Cache of myself object.
4745
private final String apiUrl;
@@ -76,7 +74,6 @@ abstract class GitHubClient {
7674
RateLimitHandler rateLimitHandler,
7775
AbuseLimitHandler abuseLimitHandler,
7876
GitHubRateLimitChecker rateLimitChecker,
79-
Consumer<GHMyself> myselfConsumer,
8077
AuthorizationProvider authorizationProvider) throws IOException {
8178

8279
if (apiUrl.endsWith("/")) {
@@ -95,37 +92,25 @@ abstract class GitHubClient {
9592
this.rateLimitHandler = rateLimitHandler;
9693
this.abuseLimitHandler = abuseLimitHandler;
9794
this.rateLimitChecker = rateLimitChecker;
98-
99-
this.login = getCurrentUser(myselfConsumer);
10095
}
10196

102-
private String getCurrentUser(Consumer<GHMyself> myselfConsumer) throws IOException {
103-
String login = null;
104-
if (this.authorizationProvider instanceof UserAuthorizationProvider
105-
&& this.authorizationProvider.getEncodedAuthorization() != null) {
106-
107-
UserAuthorizationProvider userAuthorizationProvider = (UserAuthorizationProvider) this.authorizationProvider;
97+
String getLogin() {
98+
try {
99+
if (this.authorizationProvider instanceof UserAuthorizationProvider
100+
&& this.authorizationProvider.getEncodedAuthorization() != null) {
108101

109-
login = userAuthorizationProvider.getLogin();
102+
UserAuthorizationProvider userAuthorizationProvider = (UserAuthorizationProvider) this.authorizationProvider;
110103

111-
if (login == null) {
112-
try {
113-
GHMyself myself = fetch(GHMyself.class, "/user");
114-
if (myselfConsumer != null) {
115-
myselfConsumer.accept(myself);
116-
}
117-
login = myself.getLogin();
118-
} catch (IOException e) {
119-
return null;
120-
}
104+
return userAuthorizationProvider.getLogin();
121105
}
106+
} catch (IOException e) {
122107
}
123-
return login;
108+
return null;
124109
}
125110

126111
private <T> T fetch(Class<T> type, String urlPath) throws IOException {
127112
GitHubRequest request = GitHubRequest.newBuilder().withApiUrl(getApiUrl()).withUrlPath(urlPath).build();
128-
return this.sendRequest(request, (responseInfo) -> GitHubResponse.parseBody(responseInfo, type)).body();
113+
return sendRequest(request, (responseInfo) -> GitHubResponse.parseBody(responseInfo, type)).body();
129114
}
130115

131116
/**
@@ -141,7 +126,7 @@ public boolean isCredentialValid() {
141126
return true;
142127
} catch (IOException e) {
143128
LOGGER.log(FINE,
144-
"Exception validating credentials on " + getApiUrl() + " with login '" + login + "' " + e,
129+
"Exception validating credentials on " + getApiUrl() + " with login '" + getLogin() + "' " + e,
145130
e);
146131
return false;
147132
}
@@ -185,7 +170,7 @@ public void setConnector(HttpConnector connector) {
185170
*/
186171
public boolean isAnonymous() {
187172
try {
188-
return login == null && this.authorizationProvider.getEncodedAuthorization() == null;
173+
return getLogin() == null && this.authorizationProvider.getEncodedAuthorization() == null;
189174
} catch (IOException e) {
190175
// An exception here means that the provider failed to provide authorization parameters,
191176
// basically meaning the same as "no auth"
@@ -319,7 +304,7 @@ private GHRateLimit updateRateLimit(@Nonnull GHRateLimit observed) {
319304
*/
320305
public void checkApiUrlValidity() throws IOException {
321306
try {
322-
fetch(GHApiInfo.class, "/").check(getApiUrl());
307+
this.fetch(GHApiInfo.class, "/").check(getApiUrl());
323308
} catch (IOException e) {
324309
if (isPrivateModeEnabled()) {
325310
throw (IOException) new IOException(
@@ -419,8 +404,8 @@ public <T> GitHubResponse<T> sendRequest(GitHubRequest request, @CheckForNull Gi
419404

420405
private void logRequest(@Nonnull final GitHubRequest request) {
421406
LOGGER.log(FINE,
422-
() -> "GitHub API request [" + (login == null ? "anonymous" : login) + "]: " + request.method() + " "
423-
+ request.url().toString());
407+
() -> "GitHub API request [" + (getLogin() == null ? "anonymous" : getLogin()) + "]: "
408+
+ request.method() + " " + request.url().toString());
424409
}
425410

426411
@Nonnull

‎src/main/java/org/kohsuke/github/GitHubHttpUrlConnectionClient.java‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@
1212
import java.util.HashMap;
1313
import java.util.List;
1414
import java.util.Map;
15-
import java.util.function.Consumer;
1615
import java.util.logging.Logger;
1716
import java.util.zip.GZIPInputStream;
1817

@@ -38,15 +37,8 @@ class GitHubHttpUrlConnectionClient extends GitHubClient {
3837
RateLimitHandler rateLimitHandler,
3938
AbuseLimitHandler abuseLimitHandler,
4039
GitHubRateLimitChecker rateLimitChecker,
41-
Consumer<GHMyself> myselfConsumer,
4240
AuthorizationProvider authorizationProvider) throws IOException {
43-
super(apiUrl,
44-
connector,
45-
rateLimitHandler,
46-
abuseLimitHandler,
47-
rateLimitChecker,
48-
myselfConsumer,
49-
authorizationProvider);
41+
super(apiUrl, connector, rateLimitHandler, abuseLimitHandler, rateLimitChecker, authorizationProvider);
5042
}
5143

5244
@Nonnull

‎src/test/java/org/kohsuke/github/AppTest.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -453,6 +453,7 @@ public void testRepoPermissions() throws Exception {
453453
public void testGetMyself() throws Exception {
454454
GHMyself me = gitHub.getMyself();
455455
assertThat(me, notNullValue());
456+
assertThat(me.root(), sameInstance(gitHub));
456457
assertThat(gitHub.getUser("bitwiseman"), notNullValue());
457458
PagedIterable<GHRepository> ghRepositories = me.listRepositories();
458459
assertThat(ghRepositories, is(not(emptyIterable())));

‎src/test/java/org/kohsuke/github/GitHubConnectionTest.java‎

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import org.junit.Assume;
55
import org.junit.Test;
66
import org.kohsuke.github.authorization.AuthorizationProvider;
7+
import org.kohsuke.github.authorization.ImmutableAuthorizationProvider;
78
import org.kohsuke.github.authorization.UserAuthorizationProvider;
89

910
import java.io.File;
@@ -295,7 +296,7 @@ public void testGithubBuilderWithAppInstallationToken() throws Exception {
295296
GitHub github = builder.build();
296297
// change this to get a request
297298
assertThat(github.getClient().getEncodedAuthorization(), equalTo("token bogus app token"));
298-
assertThat(github.getClient().login, is(emptyString()));
299+
assertThat(github.getClient().getLogin(), is(emptyString()));
299300
}
300301

301302
@Test
@@ -326,6 +327,20 @@ public void testGitHubIsApiUrlValid() throws IOException {
326327
}
327328
}
328329

330+
@Test
331+
public void testGitHubOAuthUserQuery() throws IOException {
332+
snapshotNotAllowed();
333+
mockGitHub.customizeRecordSpec(recordSpecBuilder -> recordSpecBuilder.captureHeader("Authorization"));
334+
gitHub = getGitHubBuilder().withEndpoint(mockGitHub.apiServer().baseUrl())
335+
.withAuthorizationProvider(ImmutableAuthorizationProvider.fromOauthToken("super_secret_token"))
336+
.build();
337+
assertThat(mockGitHub.getRequestCount(), equalTo(1));
338+
assertThat(gitHub.getMyself(), notNullValue());
339+
assertThat(gitHub.getMyself().root(), notNullValue());
340+
assertThat(gitHub.getMyself().getLogin(), equalTo("bitwiseman"));
341+
assertThat(mockGitHub.getRequestCount(), equalTo(1));
342+
}
343+
329344
/*
330345
* Copied from StackOverflow: http://stackoverflow.com/a/7201825/2336755
331346
*

‎src/test/java/org/kohsuke/github/WireMockStatusReporterTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ public void user_whenProxying_AuthCorrectlyConfigured() throws Exception {
2525

2626
verifyAuthenticated(gitHub);
2727

28-
assertThat(gitHub.getClient().login, not(equalTo(STUBBED_USER_LOGIN)));
28+
assertThat(gitHub.getClient().getLogin(), not(equalTo(STUBBED_USER_LOGIN)));
2929

3030
// If this user query fails, either the proxying config has broken (unlikely)
3131
// or your auth settings are not being retrieved from the environment.
@@ -47,7 +47,7 @@ public void user_whenNotProxying_Stubbed() throws Exception {
4747
assumeFalse("Test only valid when not proxying", mockGitHub.isUseProxy());
4848

4949
verifyAuthenticated(gitHub);
50-
assertThat(gitHub.getClient().login, equalTo(STUBBED_USER_LOGIN));
50+
assertThat(gitHub.getClient().getLogin(), equalTo(STUBBED_USER_LOGIN));
5151

5252
GHUser user = gitHub.getMyself();
5353
// NOTE: the stubbed user does not have to match the login provided from the github object

‎src/test/java/org/kohsuke/github/junit/GitHubWireMockRule.java‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import com.github.tomakehurst.wiremock.extension.ResponseTransformer;
88
import com.github.tomakehurst.wiremock.http.*;
99
import com.github.tomakehurst.wiremock.matching.RequestPatternBuilder;
10+
import com.github.tomakehurst.wiremock.recording.RecordSpecBuilder;
1011
import com.google.gson.*;
1112
import edu.umd.cs.findbugs.annotations.NonNull;
1213

@@ -18,6 +19,7 @@
1819
import java.util.Collection;
1920
import java.util.HashMap;
2021
import java.util.Map;
22+
import java.util.function.Consumer;
2123

2224
import javax.annotation.Nonnull;
2325

@@ -41,6 +43,12 @@ public class GitHubWireMockRule extends WireMockMultiServerRule {
4143
private final static boolean useProxy = takeSnapshot
4244
|| System.getProperty("test.github.useProxy", "false") != "false";
4345

46+
public void customizeRecordSpec(Consumer<RecordSpecBuilder> customizeRecordSpec) {
47+
this.customizeRecordSpec = customizeRecordSpec;
48+
}
49+
50+
private Consumer<RecordSpecBuilder> customizeRecordSpec = null;
51+
4452
public GitHubWireMockRule() {
4553
this(WireMockConfiguration.options());
4654
}
@@ -158,7 +166,7 @@ protected void after() {
158166
private void recordSnapshot(WireMockServer server, String target, boolean isRawServer) {
159167
if (server != null) {
160168

161-
server.snapshotRecord(recordSpec().forTarget(target)
169+
final RecordSpecBuilder recordSpecBuilder = recordSpec().forTarget(target)
162170
// "If-None-Match" header used for ETag matching for caching connections
163171
.captureHeader("If-None-Match")
164172
// "If-Modified-Since" header used for ETag matching for caching connections
@@ -170,7 +178,13 @@ private void recordSnapshot(WireMockServer server, String target, boolean isRawS
170178
// For example, if you update "title" and "body", and then update just "title" to the same value
171179
// the mock framework will treat those two requests as equivalent, which we do not want.
172180
.chooseBodyMatchTypeAutomatically(true, false, false)
173-
.extractTextBodiesOver(255));
181+
.extractTextBodiesOver(255);
182+
183+
if (customizeRecordSpec != null) {
184+
customizeRecordSpec.accept(recordSpecBuilder);
185+
}
186+
187+
server.snapshotRecord(recordSpecBuilder);
174188

175189
// After taking the snapshot, format the output
176190
formatTestResources(new File(server.getOptions().filesRoot().getPath()).toPath(), isRawServer);

0 commit comments

Comments
 (0)