Skip to content
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

Merged
merged 12 commits into from
Feb 14, 2025
Merged

okta client package #2

merged 12 commits into from
Feb 14, 2025

Conversation

justinrubek
Copy link
Contributor

@justinrubek justinrubek commented Feb 10, 2025

This provides common boilerplate for initializing an okta client using a JWK. A FuncOpts style builder is used to specify limit options for queries.

@justinrubek justinrubek marked this pull request as ready for review February 11, 2025 16:37
Copy link
Member

@dskatz dskatz left a 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.

@justinrubek justinrubek changed the title okta/client: initial commit provider module Feb 11, 2025
@justinrubek justinrubek changed the title provider module okta client package Feb 11, 2025
@justinrubek justinrubek marked this pull request as draft February 11, 2025 21:07
@justinrubek justinrubek requested a review from dskatz February 11, 2025 21:19
@justinrubek justinrubek marked this pull request as ready for review February 11, 2025 21:19
providers/okta/okta.go Show resolved Hide resolved
*okta.APIClient
}

func NewClient(orgURL string, clientID string, jwkBytes []byte, scopes ...string) (*Client, error) {
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

@justinrubek justinrubek Feb 12, 2025

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.

Copy link
Contributor Author

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.

providers/okta/okta.go Outdated Show resolved Hide resolved
providers/okta/okta.go Show resolved Hide resolved
@justinrubek justinrubek merged commit 038ac56 into main Feb 14, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants