Uh oh!
There was an error while loading. Please reload this page.
Add OAuth support for JIRA - #16
Conversation
MorganEPatch
commented
Aug 9, 2017
Step 1 of this document contains all the information on setting up your consumer secret and private key. https://developer.atlassian.com/cloud/jira/platform/jira-rest-api-oauth-authentication/ |
squat
left a comment
There was a problem hiding this comment.
Looks very good. Just a few small points.
| // getJIRAHTTPClient obtains an access token (either from configuration | ||
| // or from an OAuth handshake) and creates an HTTP client that uses the | ||
| // token, which can be used to configure a JIRA client. | ||
| func getJIRAHTTPClient(config cfg.Config) (*http.Client, error) { |
There was a problem hiding this comment.
This creates a new Jira HTTP client, right? Let's call the the func newJiraHTTPClient. In general, Go does not use getters. Typically, either a struct is accessible as a field or we create a new struct.
| // getOauthConfig parses a private key and consumer key from the | ||
| // configuration, and creates an OAuth configuration which can | ||
| // be used to begin a handshake. | ||
| func getOauthConfig(config cfg.Config) (oauth1.Config, error) { |
There was a problem hiding this comment.
Same here, this should really just be called func oauthConfig(...) or func newOauthConfig(...)
| } | ||
| client.Authentication.SetBasicAuth(config.GetConfigString("jira-user"), config.GetConfigString("jira-pass")) | ||
| if config.GetConfigString("jira-user") != "" || config.GetConfigString("jira-pass") != "" { |
There was a problem hiding this comment.
This is a little funny because it's checking the opposite of line 59. Looks like this should really be an else clause. It will require a little reformatting and will require you to have two:
client, err:= jira.NewClient(...)but I think it is a little easier to read. At the end of the day, both of the approaches will be equally performant, but I think this one will be more maintainable.
| jPass := c.cmdConfig.GetString("jira-pass") | ||
| if jPass == "" { | ||
| fmt.Print("Enter your JIRA password: ") |
There was a problem hiding this comment.
huh funny, fmt.Print is almost never used. I had to double check that it was a real func.
There was a problem hiding this comment.
Really? That's how I've always done asking for input in a CLI program. Is it usually done differently in Go?
There was a problem hiding this comment.
Nono, just that normally people use fmt.Println or fmt.Printf, but simply Print gets relatively little use.
| return errors.New("JIRA access token secret required") | ||
| } | ||
| c.log.Debug("Using OAuth; will perform handshake") |
There was a problem hiding this comment.
The handshake is not performed here, so I think this debug statement might be a little confusing when you're actually digging through oauth problems.
squat
left a comment
There was a problem hiding this comment.
Looks great! three small nits and we can merge.
| fmt.Print("Enter your JIRA password: ") | ||
| bytePass, err := terminal.ReadPassword(int(syscall.Stdin)) | ||
| if err != nil { | ||
| return errors.New("Jira password required") |
| "github.com/dghubble/oauth1" | ||
| ) | ||
| // getJIRAHTTPClient obtains an access token (either from configuration |
There was a problem hiding this comment.
Please update this comment func name.
| return oauthConfig.Client(ctx, tok), nil | ||
| } | ||
| // getOauthConfig parses a private key and consumer key from the |
There was a problem hiding this comment.
update the func name in the comment.
MorganEPatch
commented
Aug 11, 2017
@squat Fixed. |
If a JIRA username or password are provided, both are necessary and Basic Authentication is used. If neither are provided, OAuth is used instead. If an access token and secret are stored in the config file, they're used and a client is created. Otherwise, creating the JIRA client initiates an OAuth handshake. In both cases, the consumer secret and private key filename must be stored in the configuration file.
Depends on #15.
If a JIRA username or password are provided, Basic Authentication is still used, but otherwise, OAuth is used. If an access token is provided in the configuration file, it is used; otherwise, a token is requested via OAuth handshake, which requires that the consumer secret and private key be provided in the configuration file. The access token is saved in the config file at the end.
@squat