From 9a39e250fdab9ba2f3b4b4b3b3f8466eca9692eb Mon Sep 17 00:00:00 2001 From: Thomas Dy Date: Wed, 25 Jun 2025 17:05:32 +0900 Subject: [PATCH 1/4] Fix noop test in GithubAuthenticationTokenTest --- .../org/jenkinsci/plugins/GithubAuthenticationTokenTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/jenkinsci/plugins/GithubAuthenticationTokenTest.java b/src/test/java/org/jenkinsci/plugins/GithubAuthenticationTokenTest.java index 1051e7b1..ec705539 100644 --- a/src/test/java/org/jenkinsci/plugins/GithubAuthenticationTokenTest.java +++ b/src/test/java/org/jenkinsci/plugins/GithubAuthenticationTokenTest.java @@ -49,7 +49,7 @@ void testTokenSerialization() throws IOException { assertEquals(deserializedToken.getAccessToken(), authenticationToken.getAccessToken()); assertEquals(deserializedToken.getPrincipal(), authenticationToken.getPrincipal()); assertEquals(deserializedToken.getGithubServer(), authenticationToken.getGithubServer()); - assertEquals(deserializedToken.getMyself().getLogin(), deserializedToken.getMyself().getLogin()); + assertEquals(deserializedToken.getMyself().getLogin(), authenticationToken.getMyself().getLogin()); } } From 4e268071c606f9334661e933a93bca2bd7a43c95 Mon Sep 17 00:00:00 2001 From: Thomas Dy Date: Wed, 25 Jun 2025 17:43:49 +0900 Subject: [PATCH 2/4] Fix unused mock in GithubAuthenticationTokenTest --- .../org/jenkinsci/plugins/GithubAuthenticationTokenTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/jenkinsci/plugins/GithubAuthenticationTokenTest.java b/src/test/java/org/jenkinsci/plugins/GithubAuthenticationTokenTest.java index ec705539..74975d6c 100644 --- a/src/test/java/org/jenkinsci/plugins/GithubAuthenticationTokenTest.java +++ b/src/test/java/org/jenkinsci/plugins/GithubAuthenticationTokenTest.java @@ -22,7 +22,7 @@ @ExtendWith(MockitoExtension.class) class GithubAuthenticationTokenTest { - @Mock(strictness = Mock.Strictness.LENIENT) + @Mock private GithubSecurityRealm securityRealm; @AfterEach @@ -34,7 +34,7 @@ private void mockJenkins(MockedStatic mockedJenkins) { Jenkins jenkins = Mockito.mock(Jenkins.class); mockedJenkins.when(Jenkins::get).thenReturn(jenkins); Mockito.when(jenkins.getSecurityRealm()).thenReturn(securityRealm); - Mockito.when(securityRealm.getOauthScopes()).thenReturn("read:org"); + Mockito.when(securityRealm.hasScope("read:org")).thenReturn(true); } @Test From 0a359ca16fa813b1155d11e6d58077184b952b6f Mon Sep 17 00:00:00 2001 From: Thomas Dy Date: Wed, 25 Jun 2025 18:13:23 +0900 Subject: [PATCH 3/4] Test that GithubAuthenticationToken.loadUser works after deserialization --- .../GithubAuthenticationTokenTest.java | 31 +++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/jenkinsci/plugins/GithubAuthenticationTokenTest.java b/src/test/java/org/jenkinsci/plugins/GithubAuthenticationTokenTest.java index 74975d6c..66a4ccf2 100644 --- a/src/test/java/org/jenkinsci/plugins/GithubAuthenticationTokenTest.java +++ b/src/test/java/org/jenkinsci/plugins/GithubAuthenticationTokenTest.java @@ -6,6 +6,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.kohsuke.github.GHMyself; +import org.kohsuke.github.GHUser; import org.kohsuke.github.GitHub; import org.kohsuke.github.GitHubBuilder; import org.kohsuke.github.RateLimitHandler; @@ -42,7 +43,8 @@ void testTokenSerialization() throws IOException { try (MockedStatic mockedJenkins = Mockito.mockStatic(Jenkins.class); MockedStatic mockedGitHubBuilder = Mockito.mockStatic(GitHubBuilder.class)) { mockJenkins(mockedJenkins); - mockGHMyselfAs(mockedGitHubBuilder, "bob"); + GitHub gh = mockGH(mockedGitHubBuilder); + mockGHMyselfAs(gh, "bob"); GithubAuthenticationToken authenticationToken = new GithubAuthenticationToken("accessToken", "https://api.github.com"); byte[] serializedToken = SerializationUtils.serialize(authenticationToken); GithubAuthenticationToken deserializedToken = (GithubAuthenticationToken) SerializationUtils.deserialize(serializedToken); @@ -53,7 +55,22 @@ void testTokenSerialization() throws IOException { } } - private static GHMyself mockGHMyselfAs(MockedStatic mockedGitHubBuilder, String username) throws IOException { + @Test + void testLoadUser() throws IOException { + try (MockedStatic mockedJenkins = Mockito.mockStatic(Jenkins.class); + MockedStatic mockedGitHubBuilder = Mockito.mockStatic(GitHubBuilder.class)) { + mockJenkins(mockedJenkins); + GitHub gh = mockGH(mockedGitHubBuilder); + mockGHMyselfAs(gh, "bob"); + mockGHUser(gh, "charlie"); + GithubAuthenticationToken authenticationToken = new GithubAuthenticationToken("accessToken", "https://api.github.com"); + byte[] serializedToken = SerializationUtils.serialize(authenticationToken); + GithubAuthenticationToken deserializedToken = (GithubAuthenticationToken) SerializationUtils.deserialize(serializedToken); + assertEquals(deserializedToken.loadUser("charlie"), authenticationToken.loadUser("charlie")); + } + } + + private static GitHub mockGH(MockedStatic mockedGitHubBuilder) throws IOException { GitHub gh = Mockito.mock(GitHub.class); GitHubBuilder builder = Mockito.mock(GitHubBuilder.class); mockedGitHubBuilder.when(GitHubBuilder::fromEnvironment).thenReturn(builder); @@ -62,9 +79,19 @@ private static GHMyself mockGHMyselfAs(MockedStatic mockedGitHubB Mockito.when(builder.withRateLimitHandler(RateLimitHandler.FAIL)).thenReturn(builder); Mockito.when(builder.withConnector(Mockito.any(OkHttpGitHubConnector.class))).thenReturn(builder); Mockito.when(builder.build()).thenReturn(gh); + return gh; + } + + private static GHMyself mockGHMyselfAs(GitHub gh, String username) throws IOException { GHMyself me = Mockito.mock(GHMyself.class); Mockito.when(gh.getMyself()).thenReturn(me); Mockito.when(me.getLogin()).thenReturn(username); return me; } + + private static GHUser mockGHUser(GitHub gh, String username) throws IOException { + GHUser user = Mockito.mock(GHUser.class); + Mockito.when(gh.getUser(username)).thenReturn(user); + return user; + } } From 53de44f0d0fee07ed0261bd69c675e0272997e18 Mon Sep 17 00:00:00 2001 From: Thomas Dy Date: Wed, 25 Jun 2025 18:14:01 +0900 Subject: [PATCH 4/4] Do not check gh in GithubAuthenticationToken methods In https://github.com/jenkinsci/github-oauth-plugin/pull/61, use of `gh` was replaced by `getGitHub()` but the check for `gh != null` was not removed. `getGitHub()` already ensures that it's present so there's no need to do the check. The check was causing calls to `loadUser`, etc to return `null` if a token was restored and nothing that transitively calls `getGitHub()` has been called yet. In particular, when a user logs in, their authorities will be empty because GithubSecurityRealm tries to `loadUser` in `loadUserByUsername2` and sets authorities to empty if the user is `null`. Only later, when something transitively calls `getGitHub()`, will the user have the authorities set. --- .../org/jenkinsci/plugins/GithubAuthenticationToken.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/jenkinsci/plugins/GithubAuthenticationToken.java b/src/main/java/org/jenkinsci/plugins/GithubAuthenticationToken.java index e1e4562a..b8b3dcb4 100644 --- a/src/main/java/org/jenkinsci/plugins/GithubAuthenticationToken.java +++ b/src/main/java/org/jenkinsci/plugins/GithubAuthenticationToken.java @@ -469,7 +469,7 @@ GHUser loadUser(@NonNull String username) throws IOException { GithubUser user; try { user = usersByIdCache.getIfPresent(username); - if (gh != null && user == null && isAuthenticated()) { + if (user == null && isAuthenticated()) { GHUser ghUser = getGitHub().getUser(username); user = new GithubUser(ghUser); usersByIdCache.put(username, user); @@ -505,7 +505,7 @@ private GHMyself loadMyself(@NonNull String token) throws IOException { @Nullable GHOrganization loadOrganization(@NonNull String organization) { try { - if (gh != null && isAuthenticated()) + if (isAuthenticated()) return getGitHub().getOrganization(organization); } catch (IOException | RuntimeException e) { LOGGER.log(Level.FINEST, e.getMessage(), e); @@ -516,7 +516,7 @@ GHOrganization loadOrganization(@NonNull String organization) { @NonNull private RepoRights loadRepository(@NonNull final String repositoryName) { try { - if (gh != null && isAuthenticated() && (myRealm.hasScope("repo") || myRealm.hasScope("public_repo"))) { + if (isAuthenticated() && (myRealm.hasScope("repo") || myRealm.hasScope("public_repo"))) { Cache repoNameToRightsCache = myRepositories(); return repoNameToRightsCache.get(repositoryName, unused -> { GHRepository repo;