Repository: hub4j/github-api (java)

Issue:
Problematic regression with ~/.github file authentication
Authenticating with `~/.github` when using `GitHub.connect()` doesn't work anymore in recent versions of the GitHub API.

The client "authenticates" as anonymous instead of using the credentials.

The issue is that the `GitHubBuilder.authorizationProvider` field is never null as it's now initialized to an `ANONYMOUS` instance here: https://github.com/hub4j/github-api/blob/main/src/main/java/org/kohsuke/github/GitHubBuilder.java#L35 .

Thus when arriving here https://github.com/hub4j/github-api/blob/main/src/main/java/org/kohsuke/github/GitHubBuilder.java#L63 , the field is never null and we never get the chance to use the  `~/.github` property file to authenticate a few lines below.

I'm not exactly sure how we want to fix this. A comment in `GitHubBuilder.fromCredentials()` says `@throws IOException If there are no credentials defined in the ~/.github properties file or the process environment.` which is obviously not true anymore.

So:
- we definitely need to test that `authorizationProvider` is not `ANONYMOUS` here: https://github.com/hub4j/github-api/blob/main/src/main/java/org/kohsuke/github/GitHubBuilder.java#L35 .
- I'm not sure if we want to fix the comment and allow anonymous access (but given we explicitly called `fromCredentials()`, I would rather throw an exception) or throw an exception again if we don't have any credentials. The latter would obviously prevent anonymous access.

Note that it can be a bit hard to debug as in my case the GH API was just throwing 404 Not Found errors when trying to do some write operations instead of complaining about the missing credentials.

Also, no idea on how to test this :).

Thoughts?

Pull request description:
Fix withCredetials() to correctly detect missing creds

# Description

Fixes #1155

@gsmet @hcoles

If you want to test this fix manually against #1156 or #1157 , that would be helpful. 

# Before submitting a PR:
We love getting PRs, but we hate asking people for the same basic changes every time.

- [x] Push your changes to a branch other than `main`. Create your PR from that branch.
- [x] Add JavaDocs and other comments
- [x] Write tests that run and pass in CI. See [CONTRIBUTING.md](CONTRIBUTING.md) for details on how to capture snapshot data.
- [x] Run `mvn -D enable-ci clean install site` locally. If this command doesn't succeed, your change will not pass CI.

# When creating a PR: 

- [x] Fill in the "Description" above.
- [x] Enable "Allow edits from maintainers".


Patch:
diff --git a/src/main/java/org/kohsuke/github/GitHubBuilder.java b/src/main/java/org/kohsuke/github/GitHubBuilder.java
index 94ba46a0e..80960b979 100644
--- a/src/main/java/org/kohsuke/github/GitHubBuilder.java
+++ b/src/main/java/org/kohsuke/github/GitHubBuilder.java
@@ -24,6 +24,9 @@ import javax.annotation.Nonnull;
  */
 public class GitHubBuilder implements Cloneable {
 
+    // for testing
+    static File HOME_DIRECTORY = null;
+
     // default scoped so unit tests can read them.
     /* private */ String endpoint = GitHubClient.GITHUB_URL;
 
@@ -60,13 +63,13 @@ public class GitHubBuilder implements Cloneable {
 
         builder = fromEnvironment();
 
-        if (builder.authorizationProvider != null)
+        if (builder.authorizationProvider != AuthorizationProvider.ANONYMOUS)
             return builder;
 
         try {
             builder = fromPropertyFile();
 
-            if (builder.authorizationProvider != null)
+            if (builder.authorizationProvider != AuthorizationProvider.ANONYMOUS)
                 return builder;
         } catch (FileNotFoundException e) {
             // fall through
@@ -178,7 +181,7 @@ public class GitHubBuilder implements Cloneable {
      *             the io exception
      */
     public static GitHubBuilder fromPropertyFile() throws IOException {
-        File homeDir = new File(System.getProperty("user.home"));
+        File homeDir = HOME_DIRECTORY != null ? HOME_DIRECTORY : new File(System.getProperty("user.home"));
         File propertyFile = new File(homeDir, ".github");
         return fromPropertyFile(propertyFile.getPath());
     }


Test patch:
diff --git a/src/test/java/org/kohsuke/github/GitHubConnectionTest.java b/src/test/java/org/kohsuke/github/GitHubConnectionTest.java
index 852b64e39..5e96cc280 100644
--- a/src/test/java/org/kohsuke/github/GitHubConnectionTest.java
+++ b/src/test/java/org/kohsuke/github/GitHubConnectionTest.java
@@ -1,14 +1,19 @@
 package org.kohsuke.github;
 
+import org.apache.commons.lang3.SystemUtils;
 import org.junit.Assume;
 import org.junit.Test;
+import org.kohsuke.github.authorization.AuthorizationProvider;
 import org.kohsuke.github.authorization.UserAuthorizationProvider;
 
+import java.io.File;
+import java.io.FileOutputStream;
 import java.io.IOException;
 import java.lang.reflect.Field;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Properties;
 
 import static org.hamcrest.Matchers.*;
 
@@ -137,6 +142,147 @@ public class GitHubConnectionTest extends AbstractGitHubWireMockTest {
         assertThat(((UserAuthorizationProvider) builder.authorizationProvider).getLogin(), equalTo("bogus login"));
     }
 
+    @Test
+    public void testGitHubBuilderFromCredentialsWithEnvironment() throws IOException {
+        // we disable this test for JDK 16+ as the current hacks in setupEnvironment() don't work with JDK 16+
+        Assume.assumeThat(Double.valueOf(System.getProperty("java.specification.version")), lessThan(16.0));
+        Assume.assumeFalse(SystemUtils.IS_OS_WINDOWS);
+
+        Map<String, String> props = new HashMap<String, String>();
+
+        props.put("endpoint", "bogus endpoint url");
+        props.put("oauth", "bogus oauth token string");
+        setupEnvironment(props);
+        GitHubBuilder builder = GitHubBuilder.fromCredentials();
+
+        assertThat(builder.endpoint, equalTo("bogus endpoint url"));
+
+        assertThat(builder.authorizationProvider, instanceOf(UserAuthorizationProvider.class));
+        assertThat(builder.authorizationProvider.getEncodedAuthorization(), equalTo("token bogus oauth token string"));
+        assertThat(((UserAuthorizationProvider) builder.authorizationProvider).getLogin(), nullValue());
+
+        props.put("login", "bogus login");
+        setupEnvironment(props);
+        builder = GitHubBuilder.fromCredentials();
+
+        assertThat(builder.authorizationProvider, instanceOf(UserAuthorizationProvider.class));
+        assertThat(builder.authorizationProvider.getEncodedAuthorization(), equalTo("token bogus oauth token string"));
+        assertThat(((UserAuthorizationProvider) builder.authorizationProvider).getLogin(), equalTo("bogus login"));
+
+        props.put("jwt", "bogus jwt token string");
+        setupEnvironment(props);
+        builder = GitHubBuilder.fromCredentials();
+
+        assertThat(builder.authorizationProvider, not(instanceOf(UserAuthorizationProvider.class)));
+        assertThat(builder.authorizationProvider.getEncodedAuthorization(), equalTo("Bearer bogus jwt token string"));
+
+        props.put("password", "bogus weak password");
+        setupEnvironment(props);
+        builder = GitHubBuilder.fromCredentials();
+
+        assertThat(builder.authorizationProvider, instanceOf(UserAuthorizationProvider.class));
+        assertThat(builder.authorizationProvider.getEncodedAuthorization(),
+                equalTo("Basic Ym9ndXMgbG9naW46Ym9ndXMgd2VhayBwYXNzd29yZA=="));
+        assertThat(((UserAuthorizationProvider) builder.authorizationProvider).getLogin(), equalTo("bogus login"));
+    }
+
+    @Test
+    public void testGitHubBuilderFromCredentialsWithPropertyFile() throws IOException {
+        // we disable this test for JDK 16+ as the current hacks in setupEnvironment() don't work with JDK 16+
+        Assume.assumeThat(Double.valueOf(System.getProperty("java.specification.version")), lessThan(16.0));
+        Assume.assumeFalse(SystemUtils.IS_OS_WINDOWS);
+
+        Map<String, String> props = new HashMap<String, String>();
+
+        // Clear the environment
+        setupEnvironment(props);
+        try {
+            GitHubBuilder.HOME_DIRECTORY = new File(getTestDirectory());
+            try {
+                GitHubBuilder builder = GitHubBuilder.fromCredentials();
+                fail();
+            } catch (Exception e) {
+                assertThat(e, instanceOf(IOException.class));
+                assertThat(e.getMessage(), equalTo("Failed to resolve credentials from ~/.github or the environment."));
+            }
+
+            props = new HashMap<String, String>();
+
+            props.put("endpoint", "bogus endpoint url");
+            props.put("oauth", "bogus oauth token string");
+
+            setupPropertyFile(props);
+
+            GitHubBuilder builder = GitHubBuilder.fromCredentials();
+
+            assertThat(builder.endpoint, equalTo("bogus endpoint url"));
+
+            assertThat(builder.authorizationProvider, instanceOf(UserAuthorizationProvider.class));
+            assertThat(builder.authorizationProvider.getEncodedAuthorization(),
+                    equalTo("token bogus oauth token string"));
+            assertThat(((UserAuthorizationProvider) builder.authorizationProvider).getLogin(), nullValue());
+
+            props.put("login", "bogus login");
+            setupPropertyFile(props);
+            builder = GitHubBuilder.fromCredentials();
+
+            assertThat(builder.authorizationProvider, instanceOf(UserAuthorizationProvider.class));
+            assertThat(builder.authorizationProvider.getEncodedAuthorization(),
+                    equalTo("token bogus oauth token string"));
+            assertThat(((UserAuthorizationProvider) builder.authorizationProvider).getLogin(), equalTo("bogus login"));
+
+            props.put("jwt", "bogus jwt token string");
+            setupPropertyFile(props);
+            builder = GitHubBuilder.fromCredentials();
+
+            assertThat(builder.authorizationProvider, not(instanceOf(UserAuthorizationProvider.class)));
+            assertThat(builder.authorizationProvider.getEncodedAuthorization(),
+                    equalTo("Bearer bogus jwt token string"));
+
+            props.put("password", "bogus weak password");
+            setupPropertyFile(props);
+            builder = GitHubBuilder.fromCredentials();
+
+            assertThat(builder.authorizationProvider, instanceOf(UserAuthorizationProvider.class));
+            assertThat(builder.authorizationProvider.getEncodedAuthorization(),
+                    equalTo("Basic Ym9ndXMgbG9naW46Ym9ndXMgd2VhayBwYXNzd29yZA=="));
+            assertThat(((UserAuthorizationProvider) builder.authorizationProvider).getLogin(), equalTo("bogus login"));
+        } finally {
+            GitHubBuilder.HOME_DIRECTORY = null;
+            File propertyFile = new File(getTestDirectory(), ".github");
+            propertyFile.delete();
+        }
+    }
+
+    private void setupPropertyFile(Map<String, String> props) throws IOException {
+        File propertyFile = new File(getTestDirectory(), ".github");
+        Properties properties = new Properties();
+        properties.putAll(props);
+        properties.store(new FileOutputStream(propertyFile), "");
+    }
+
+    private String getTestDirectory() {
+        return new File("target").getAbsolutePath();
+    }
+
+    @Test
+    public void testAnonymous() throws IOException {
+        // we disable this test for JDK 16+ as the current hacks in setupEnvironment() don't work with JDK 16+
+        Assume.assumeThat(Double.valueOf(System.getProperty("java.specification.version")), lessThan(16.0));
+
+        Map<String, String> props = new HashMap<String, String>();
+
+        props.put("endpoint", mockGitHub.apiServer().baseUrl());
+        setupEnvironment(props);
+
+        // No values present except endpoint
+        GitHubBuilder builder = GitHubBuilder
+                .fromEnvironment("customLogin", "customPassword", "customOauth", "endpoint");
+
+        assertThat(builder.endpoint, equalTo(mockGitHub.apiServer().baseUrl()));
+        assertThat(builder.authorizationProvider, sameInstance(AuthorizationProvider.ANONYMOUS));
+    }
+
     @Test
     public void testGithubBuilderWithAppInstallationToken() throws Exception {