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

Enabling "Bearer" as an additional AuthorizationMode #307

Closed
wants to merge 10 commits into from

Conversation

outofjungle
Copy link
Contributor

@outofjungle outofjungle commented May 31, 2022

Summary

  • Added Bearer as an authorization mode in addition to SWSS and PrivateKey
  • Fixed unit tests

Fixes #

  • N/A

Type of PR

  • Bug Fix (non-breaking fixes to existing functionality)
  • New Feature (non-breaking changes that add new functionality)
  • Documentation update
  • Test Updates
  • Other (Please describe the type)

Test Information

  • My PR required test updates
    The fresh checkout of the code from master did not pass many tests. I was able to fix the unit tests, however I didn't have time to fix the integration tests.

Go Version: go version go1.18 darwin/amd64
Os Version: Darwin Kernel Version 21.5.0: Tue Apr 26 21:08:22 PDT 2022; root:xnu-8020.121.3~4/RELEASE_X86_64 x86_64
OpenAPI Spec Version:

Signoff

  • I have submitted a CLA for this PR
  • Each commit message explains what the commit does
  • I have updated documentation to explain what my PR does
  • My code is covered by tests if required
  • I ran make fmt on my code
  • I did not edit any automatically generated files

@arvindkrishnakumar-okta
Copy link

arvindkrishnakumar-okta commented May 31, 2022

@outofjungle Thanks for the PR!

Someone from our team will review this soon.

cc: @monde @bretterer

@monde monde self-assigned this May 31, 2022
Copy link
Collaborator

@monde monde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@outofjungle I can't take this PR with the changes made to the test files.

@@ -153,7 +153,6 @@ func (m *ApplicationResource) ListApplications(ctx context.Context, qp *query.Pa
apps[i] = &application[i]
}
return apps, resp, nil

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see the latest mvdan.cc/gofumpt@latest is causing all of these white space issues. Sad.

_, _, err := tests.NewClient(
context.TODO(),
okta.WithAuthorizationMode("SSWS"),
okta.WithOrgUrl("https://test.okta.com"),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our tests run in CI against a real Okta Org with these env variables:

  • OKTA_CLIENT_ORGURL
  • OKTA_CLIENT_TOKEN
  • OKTA_CLIENT_CLIENTID
  • OKTA_CLIENT_PRIVATEKEY

so overriding the config variables with fake values is unacceptable.

@monde
Copy link
Collaborator

monde commented Jun 1, 2022

@outofjungle I thought about this further. Bearer token is for OAuth which is specific to Okta applications. okta-sdk-golang is oriented to the management SDK. So if the SDK is put into Bearer token mode much of the rest of the sdk will get 401 /you do not have permission to access the feature you are requesting. Specifically the app associated with the token will only have access to these endpoints assuming it is also granted the scope for those endponts https://developer.okta.com/docs/guides/implement-oauth-for-okta/main/#scopes-and-supported-endpoints . This behavior should be documented in the README and part of your PR. Correct me if I'm misunderstanding your intent with this PR.

One other ask, we'd need you to submit a Okta Individual Contributor License Agreement https://developer.okta.com/cla/ as this is new behavior being added to the SDK.

@monde monde removed the request for review from bretterer June 1, 2022 16:18
@outofjungle
Copy link
Contributor Author

@monde Yes, your assumptions are correct. This is a new feature where I'm using an out-of-band logic to get a Bearer token for a user via scoped app. I will update the documentation and update the PR. I also emailed my signed CLA just now.

I will revert the test changes. Looks like I can't set them up correctly on my laptop and get them to pass.

@monde
Copy link
Collaborator

monde commented Jun 1, 2022

Awesome @outofjungle , thanks. Can you revert the make fmt changes, I know the PR template asks for it, but I'd prefer this PR not be cluttered with white space noise. I'm going to catch the make fmt changes in another PR.

@monde monde added the CLA Has Okta Individual Contributor License Agreement label Jun 1, 2022
@outofjungle
Copy link
Contributor Author

@monde Updated the README with a section for Bearer token. As many types of apps mint bearer token, I tried my best to document all that i'm aware of. Let me know if the documentation is acceptable.

Copy link
Collaborator

@monde monde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thank you @outofjungle

@monde
Copy link
Collaborator

monde commented Jun 2, 2022

@outofjungle I have to create a PR from the otka/okta-sdk-golang repo itself and cherry pick in your work due to the security we have on our CI runs. This will be released in v2.13.0 but I don't think that will happen until next week we have a couple of PRs in our api spec that are being worked through

@outofjungle
Copy link
Contributor Author

@monde Thanks for the approval and your update. Next week is totally fine with me :)

@monde
Copy link
Collaborator

monde commented Jun 21, 2022

@monde monde closed this Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Has Okta Individual Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants