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.

Sign up for GitHub

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

Jump to bottom

add support for dpop #454

Merged
merged 1 commit into from
May 14, 2024
Merged

add support for dpop #454

merged 1 commit into from
May 14, 2024

Conversation

duytiennguyen-okta
Copy link
Contributor

@duytiennguyen-okta duytiennguyen-okta commented May 8, 2024 •
edited
Loading

Summary

Fixes #

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

Go Version:
Os Version:
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

monde
monde reviewed May 13, 2024
View reviewed changes
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.

Seems like there sound be a README update or other documentation for this feature.

laura-rodriguez
laura-rodriguez reviewed May 13, 2024
View reviewed changes
Copy link
Collaborator

@laura-rodriguez laura-rodriguez left a comment

Choose a reason for hiding this comment

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

I see templates have been modified, but the actual client needs to be included to the PR? Also, make sure you're sending isDPoP: true (check tech design doc for user-agent details) via user-agent for tracking purposes.
I agree we should update the readme with at least a note that the SDK supports DPoP?

laura-rodriguez
laura-rodriguez reviewed May 13, 2024
View reviewed changes
.generator/templates/client.mustache Outdated
a.req.Header.Add("Authorization", "Bearer "+accessToken.(string))
nonce, hasNonce := a.tokenCache.Get(DpopAccessTokenNonce)
if !hasNonce {
a.req.Header.Add("Authorization", "Bearer "+accessToken.(string))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Maybe we can do the same as in line 198 and get the token type from the cache instead of hardcoding?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The risk of it is too many item getting cached and failure to retrieve token type despite everything else available is not something I would want. Obviously the argument for it will changed when there are more token type coming, but I think for now this should suffice

@duytiennguyen-okta
Copy link
Contributor Author

I see templates have been modified, but the actual client needs to be included to the PR? Also, make sure you're sending isDPoP: true (check tech design doc for user-agent details) via user-agent for tracking purposes. I agree we should update the readme with at least a note that the SDK supports DPoP?

I have added the user-agent header. In go, we only need to check in the template. The codegen will be done by github action

laura-rodriguez
laura-rodriguez reviewed May 14, 2024
View reviewed changes
README.md
@@ -934,6 +934,8 @@ When using a keypair, you won't need an API Token because the SDK will
request an access token for you. In order to use OAuth 2.0, construct a client
instance by passing the following parameters:

OAuth 2.0 flow is now support Dpop
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
OAuth 2.0 flow is now support Dpop
OAuth 2.0 flow now supports [DPoP](https://developer.okta.com/docs/guides/dpop/main/)

laura-rodriguez
laura-rodriguez approved these changes May 14, 2024
View reviewed changes
Copy link
Collaborator

@laura-rodriguez laura-rodriguez left a comment

Choose a reason for hiding this comment

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

Minor suggestion - LGTM

@duytiennguyen-okta duytiennguyen-okta merged commit 7692fb7 into master May 14, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monde monde monde left review comments

@laura-rodriguez laura-rodriguez laura-rodriguez approved these changes

@arvindkrishnakumar-okta arvindkrishnakumar-okta Awaiting requested review from arvindkrishnakumar-okta

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@duytiennguyen-okta @monde @laura-rodriguez