Add Github controller and jwt helper #66
Conversation
| Date now = new Date(nowMillis); | ||
|
|
||
| //We will sign our JWT with our private key | ||
| Key signingKey = get("custom_distribution_private_key.der"); |
There was a problem hiding this comment.
Should I push this too ?
There was a problem hiding this comment.
You shouldn't include secret keys unencrypted, so you'll want to encrypt the key in some way
There was a problem hiding this comment.
I am not too sure how to do that ? because this would be signed using rsa and the APP_ID so I am assuming if we keep the appID safe we can just push this ?
|
|
||
| public class GithubController { | ||
|
|
||
| private static String APP_IDENTIFIER=""; |
There was a problem hiding this comment.
Should I load this via environment variables ?
There was a problem hiding this comment.
Yes, it's always good to pull things into environment variables
There was a problem hiding this comment.
I put it in the application.properties, is that enough ?
|
|
||
| @PostMapping(path = "/event_handler") | ||
| public void handleEvent(@RequestBody String requestBodyString) throws Exception { | ||
| System.out.println("Received event handler event"); |
There was a problem hiding this comment.
Should the logger be used here instead of System.out?
| private String getAppIdentifier; | ||
|
|
||
| @PostMapping(path = "/event_handler") | ||
| public void handleEvent(@RequestBody String requestBodyString) throws Exception { |
There was a problem hiding this comment.
Same comment as the other pull-request, please use javadoc to document methods.
| @CrossOrigin("*") | ||
| public class GithubController { | ||
|
|
||
| @Value("${APP_IDENTIFIER}") |
There was a problem hiding this comment.
The value is read from the properties file. Does the user need to change this value when they run the application?
There was a problem hiding this comment.
If this is hosted no, if it is self-hosted then yes
There was a problem hiding this comment.
In the case where it is self-hosted, the user needs a way to change the APP_IDENTIFIER value then. I would recommend finding a way to read this value from an environment variable, or a user provided properties file, without having the user change the source code of the application.
| Date now = new Date(nowMillis); | ||
|
|
||
| //We will sign our JWT with our private key | ||
| Key signingKey = get("jenkins-custom-distribution-bot.der"); |
There was a problem hiding this comment.
Is this private key the same for all users of the application? Excuse my ignorance, but where does this key come from?
There was a problem hiding this comment.
This private key comes from the private key of the bot that one creates on github, so when you create an application on github you get a private key for that bot, which you can then sign and verify yourself
There was a problem hiding this comment.
Is the private key stored in the .der file? What is stored in the .der file?
|
|
||
| public class GithubJwtHelper { | ||
|
|
||
| static PrivateKey get(String filename) throws Exception { |
There was a problem hiding this comment.
Correct me if I am wrong. I think this method 1) encodes the key, then 2) it encrypts it. Is that right?
There was a problem hiding this comment.
Ok, then this explanation can be put in the javadoc.
|
These Pull requests can be opened later when we have concrete evidence that this is a feature that is useful for users |
This PR adds a Github Controller and the required JWT helper. The JWT helper takes care of the signing of the private key and the controller takes care of authenticating the application. We would like to keep this PR separate from the Github Services that will follow.