Feat/credential provider refresh

[MarcosCela]

Description

This PR attempts to solve: #941

[MarcosCela]

@bitwiseman Hi Liam!
Just as you said in #941, this adds a CredentialProvider. Right now the changes just ensure that credentials are always retrieved
from the CredentialProvider, but we are mising the refresh logic.

As I see it, the logic that the user will need to follow is:

• Create a JWT token, with a specified expiration time.
• With the JWT token, request an installation token, this installation token lives up to 1h.

We will need to either implement a JWTokenProvider in a similar fashion, leaving the user to provide a valid JWT token to use in a CredentialProvider, or alternatively implement all the logic in the library. I am going to do the first approach for now!

[bitwiseman]

@MarcosCela

If I understand correctly, we'll need to do the second approach in order for refresh to work for longer than the expiration time of the JWT Token. And JWT Tokens have a shorter life span than the GitHub App Tokens.

[MarcosCela]

@MarcosCela

If I understand correctly, we'll need to do the second approach in order for refresh to work for longer than the expiration time of the JWT Token. And JWT Tokens have a shorter life span than the GitHub App Tokens.

Sorry, bad explanation. As I see it right now we have 2 choices:

• Users of the library provide a JWTTokenProvider that abstracts the creation of the JWT token from the library. We would not need to include any external libraries.
• We implement the creation of the JWT token in the library, and users will just provide an application ID and a private key. This will need to include some external libraries:

 <dependency>
      <groupId>io.jsonwebtoken</groupId>
      <artifactId>jjwt-api</artifactId>
    </dependency>
    <dependency>
      <groupId>io.jsonwebtoken</groupId>
      <artifactId>jjwt-impl</artifactId>
    </dependency>
    <dependency>
      <groupId>io.jsonwebtoken</groupId>
      <artifactId>jjwt-jackson</artifactId>
    </dependency>

Will address the other issues, thank you for your time and insight!

[MarcosCela]

As it's right now, an example on how to use this will be as follows:

• First, you create a GitHubBuilder and configure it as follows:

// User is left with the responsibility to make sure that the passed credential provider always returns
        // a valid JWT token
        GitHub jwtAuthenticated = new GitHubBuilder().withCredentialProvider(() -> "a valid JWT token").build();

        // With the JWT authenticated client, we will be requesting installation apps
        GitHub gh = new GitHubBuilder()
                .withCredentialProvider(new OrgInstallationCredentialProvider("myOrganization", jwtAuthenticated))
                .build();

Where OrgInstallationCredentialProvider is an Oauth2 Token provider that uses a JWT authenticated client. With the JWT token, we request a new installation token when the current one has expired. Users need to provide valid tokens. E.g:

@Slf4j
@Component
@RequiredArgsConstructor
public class JWTTokens {

    private static final long MINUTES_10 = Duration.buildByMinutes(10).getMilliseconds();


    private final PrivateKey privateKey;

    public String token() {
        log.debug("Getting a new JWT token");
        return getJWT();
    }


    public String getJWT() {
        long nowMillis = System.currentTimeMillis();
        Date now = new Date(nowMillis);


        // Let's set the JWT Claims
        JwtBuilder builder = Jwts.builder()
            .setIssuedAt(now)
            .setIssuer("myApplicationId")
            .signWith(privateKey, SignatureAlgorithm.RS256);

        // if it has been specified, let's add the expiration
        if (MINUTES_10 > 0) {
            long expMillis = nowMillis + MINUTES_10;
            Date exp = new Date(expMillis);
            builder.setExpiration(exp);
        }

        // Builds the JWT and serializes it to a compact, URL-safe string
        return builder.compact();
    }

}

We could simplify this and provide the creation of the JWT token from a given private key and applicationId, but it would mean to add dependencies to the library. I think that it's better to let users chose what kind of JWT libraries they use.

We are of course missing tests.
What do you think?

Ready to review!

[bitwiseman]

@MarcosCela
Cloud you take a look at the changes I've made and make sure the make sense to you?

[MarcosCela]

@MarcosCela
Cloud you take a look at the changes I've made and make sure the make sense to you?

Definetly, I will take a look as soon as I can, thanks again for your time and dedication!!

[MarcosCela]

This looks extremely promising, to perform automatic access token refresh, I can do something like this:

    CredentialProvider prov = new JWTTokenProvider("applicationId", new File("/my/path.extension"));

    GitHub jwtTokenRequester = new GitHubBuilder()
        .withCredentialProvider(prov)
        .build();
        
    GitHub gh = new GitHubBuilder().withCredentialProvider(() -> {
        // Missing all the refresh token logic, this is requesting a new token all the time (for simplicity)
        return jwtTokenRequester.getApp().getInstallationById("installationId").getAccessTokenUrl();
    }).build();
        
    // From now on, I will just use "gh" for the rest of my program logic, that defers to the inlined
    // credential provider, that uses a second GitHub instance "jwtTokenRequester" to refresh the access token
    // sounds convoluted

With your changes I think that the client is extremely flexible in terms of authentication, looks finished to me!
Perhaps I can add an example of an implementation for the "inline" credential provider that requests an installation
token in the extras package?
Thanks again for your time!

[bitwiseman]

@MarcosCela

I wouldn't use this as code as the example. I really don't want people to do it the way you've shown above. I want them to implement the bind(GitHub) method. I definitely do not want people to inline credential providers. Maybe the right thing to do is have a separate interface or base class? Or make bind(GitHub) not default?

    CredentialProvider jwtProvider = new JWTTokenProvider("applicationId", new File("/my/path.extension"));

    GitHub gh = new GitHubBuilder().withCredentialProvider(new OrgInstallationCredentialProvider("testOrganization", jwtProvider)).build();

Oh, we need to change OrgInstallationCredentialProvider to AppInstallationCredentialProvider (or at least OrgAppInstallationCredentialProvider).

Also, what do you think of changing CredentialProvider to AuthorizationProvider as that is more accurate to what these are doing now - they are providing the Authorization header value.

[MarcosCela]

@MarcosCela

I wouldn't use this as code as the example. I really don't want people to do it the way you've shown above. I want them to implement the bind(GitHub) method. I definitely do not want people to inline credential providers. Maybe the right thing to do is have a separate interface or base class? Or make bind(GitHub) not default?

    CredentialProvider jwtProvider = new JWTTokenProvider("applicationId", new File("/my/path.extension"));

    GitHub gh = new GitHubBuilder().withCredentialProvider(new OrgInstallationCredentialProvider("testOrganization", jwtProvider)).build();

Oh, we need to change OrgInstallationCredentialProvider to AppInstallationCredentialProvider (or at least OrgAppInstallationCredentialProvider).

Also, what do you think of changing CredentialProvider to AuthorizationProvider as that is more accurate to what these are doing now - they are providing the Authorization header value.

100% agree on the inlining, it should have a different interface/class class. I will proceed to rename the name to AuthorizationProvider, and all implementations if required (e.g: AppInstallationAuthorizationProvider).
I will also give it another go and see what will be the easiest way to perform the automatic refresh with the bind method, I tried it before but could not understand it fully, will give it another go!
Thanks again!

[MarcosCela]

I applied your changes and tested it (successfully!) with the following snippet:

  AuthorizationProvider jwt = new JWTTokenProvider("12345",
            Paths.get("/location/to/my/key"));

        OrgAppInstallationAuthorizationProvider orgAUth = 
            new OrgAppInstallationAuthorizationProvider("my-organization", jwt);

        GitHub gh = new GitHubBuilder().withAuthorizationProvider(orgAUth)
            .build();
        System.out.println("gh.getRateLimit() = " + gh.getRateLimit());

Is this the way that you were referring to?

[bitwiseman]

@MarcosCela
To test JWT provider, maybe just add an assertion to the test that uses these.

[MarcosCela]

Tests added, what do you think?

[bitwiseman]

Very nice.

[timja]

any reason this is private? seems like I just have to re-implement this / copy it if I'm working with strings

[bitwiseman]

Probably better to create a constructor that takes a String.

[timja]

yes or that...