- 
                Notifications
    
You must be signed in to change notification settings  - Fork 30
 
github: GitHub app installation auth support #203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| 
               | 
          ||
| static AccessTokenProvider fromAuth(Auth auth, String baseUrl, String installationRepo, GitSecretService secretService) { | ||
| 
               | 
          ||
| if (auth instanceof Auth.AccessTokenAuth tokenAuth) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think with sealed interfaces we could use switch with pattern matching here?
switch (auth) {
  case Auth.AccessToken -> ...
  case Auth.AppInstallationAuth -> ...
  ...etc
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had that in mind when first adding the sealed classes...but that's a JDK21 feature. It's preview in JDK17.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, right. We should move to 21 sometime soon:)
| default -> throw new IllegalArgumentException("Unsupported auth type: " + authType.getKey()); | ||
| }; | ||
| 
               | 
          ||
| Auth a = Utils.getObjectMapper().convertValue(authType.getValue(), authClass); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is private static final ObjectMapper objectMapper already?
Also, GitHubTaskV2 should use @Inject-able ObjectMapper (provided by https://github.com/walmartlabs/concord/blob/master/runtime/v2/runner/src/main/java/com/walmartlabs/concord/runtime/v2/runner/guice/ObjectMapperProvider.java)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's definitely too many ObjectMapper's being instantitated. Easy to clean that up.
Regarding injection, easy enough to do for runtime-v2, then throw in a method to whip one up in runtime-v1. However, I have a concern that there's no protection from some other task going ham enabling or disabling features causing side effects. Say, changing timestamp handling, or adding custom deserializers. The should copy it, but it seem pretty easy for accidents to happen.
| String token = getString(in, GITHUB_AUTH_ACCESSTOKEN, null); | ||
| 
               | 
          ||
| if (auth.isEmpty()) { | ||
| log.warn("Using deprecated 'accessToken' input. Please use 'auth.accessToken.token' instead."); | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any other properties in auth.accessToken besides .token? Why not just auth.accessToken?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is not other expected properties. Treating the auth value as a Map makes it a bit easier to handle it similarly to the other auth types and run it through the same convertValue call and then use with the returned AccessTokenProvider. Otherwise we need more routes of logic handle just a plain ol' String value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should prioritize the way it looks in the yml but no strong feelings in this specific case. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair approach. Addressed in latest commit.
| } | ||
| 
               | 
          ||
| @Override | ||
| public TaskResult execute(Variables input) { | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep yep yep 👍
5c68427    to
    3f5e11c      
    Compare
  
    
Add support for GitHub API authentication via GitHub app installation (versus on-behalf-of user tokens).
New
authinput replaces old singleaccessTokeninput.A public method is added for just generating an access token.
App-generate access tokens can be used with the
gittask.App-generated access tokens are good for 10 minutes.