-
Notifications
You must be signed in to change notification settings - Fork 0
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
okta client package #2
Conversation
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.
Once #3 is merged there will be a populated go.mod
and go.sum
where your files can merge into.
a335e52
to
4e83d58
Compare
4e83d58
to
8a2e449
Compare
providers/okta/okta.go
Outdated
*okta.APIClient | ||
} | ||
|
||
func NewClient(orgURL string, clientID string, jwkBytes []byte, scopes ...string) (*Client, error) { |
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.
instead of accepting a byte slice; should this be a jose.JSONWebKey
arg? As a consumer of this package I wouldn't immediately know what to provide if its a byte slice.
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.
Probably; although I considered part of the value of this to be that the consumer doesn't have to worry about the specific type; we'd probably be better off having them provide an rsa.PrivateKey
instead, but then the consumer must import go-jose or equivalent
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.
Upon further thought, I'm wondering if it's worth having this module here at all if we're going to strip this out. What we really need to initialize the client is an string containing an RSA private key in PEM format, but what we are supplied is a JWK. I spent a good chunk of a day reading okta docs figuring out how to use this to hit the API but could've been up and running in minutes with the solution as it is. The function signature used here is taken almost entirely from one of our existing codebases that I didn't know about at the time.
If we're going to make consumers perform the processing from JWK -> PEM and then expose a client with 1-2 functions then as a consumer I'd rather just do this entirely in the other application because it becomes a needless dependency.
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've added some utility functions to handle converting a JWK from bytes to a PEM string. The NewClientFromJWKBytes
constructor has been created using that; the existing constructor repurposed to accept the key as a string.
This provides common boilerplate for initializing an okta client using a JWK. A FuncOpts style builder is used to specify limit options for queries.