Skip to content

feat: Add RemoteOAuth Token helper to refresh access_token from cloud environment#1866

Merged
kodiakhq[bot] merged 17 commits intomainfrom
feat/cq-cloud-remote-oauth-token
Aug 12, 2024
Merged

feat: Add RemoteOAuth Token helper to refresh access_token from cloud environment#1866
kodiakhq[bot] merged 17 commits intomainfrom
feat/cq-cloud-remote-oauth-token

Conversation

@disq
Copy link
Copy Markdown
Member

@disq disq commented Aug 11, 2024

@github-actions github-actions Bot added the feat label Aug 11, 2024
Comment thread auth/options.go Outdated
Comment thread auth/options.go Outdated
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Aug 11, 2024

⏱️ Benchmark results

  • Glob-8 ns/op: 92.57

@disq disq marked this pull request as ready for review August 12, 2024 07:38
@disq disq requested review from a team and marianogappa August 12, 2024 07:38
Copy link
Copy Markdown
Contributor

@marianogappa marianogappa left a comment

Choose a reason for hiding this comment

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

Just optional comments. Originally I had more to say but suddenly TokenWithContext got simplified, and the tests I was needing magically appeared 🤣

Comment thread helpers/remoteoauth/token.go Outdated
Comment thread helpers/remoteoauth/token.go Outdated
Comment thread helpers/remoteoauth/token.go Outdated
Comment thread helpers/remoteoauth/token.go Outdated
Comment thread helpers/remoteoauth/token.go Outdated
Comment thread helpers/remoteoauth/token.go Outdated
Comment thread helpers/remoteoauth/tokenoptions.go Outdated
@disq disq changed the title feat: RemoteOAuthToken to refresh access_token from cloud environment feat: Add RemoteOAuth Token helper to refresh access_token from cloud environment Aug 12, 2024
@github-actions github-actions Bot added feat and removed feat labels Aug 12, 2024
@disq disq requested a review from erezrokah August 12, 2024 09:19
Copy link
Copy Markdown
Member

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

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

Nice helper to have, added a few comments

Comment thread helpers/remoteoauth/tokenoptions.go Outdated
Comment thread helpers/remoteoauth/token.go Outdated
"golang.org/x/oauth2"
)

type Token struct {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

oauth2 has a ReuseTokenSource (or ReuseTokenSourceWithExpiry) where you can pass the initial token and a interface implementation of Token().
That will save us from implementing all the locking involved, valid and expiry logic

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

All token-refreshing methods in oauth2 require client credentials to work and they use a refresh_token to do so. They are not usable for this use case on their own.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

As another point, we want to default to the client-provided token only if cloud env is not set (because plugins don't have the expires_at field in their spec as they assume the user-provided token would be valid for the lifetime of the plugin). That's why the remoteToken exists, if cloud env is set we override the spec-specified token with a cloud one and can have the actual expiry timestamp.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

... I'll check if something can be done within the limitations of the things I've outlined in the above two comments.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think it saves us from anything, the code is simple and about the same as the official implementation. Wrapping it in another ReuseTokenSource will just make it complicated. Furthermore, removing expired will remove timeNow access and will make testing harder or slower.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

All token-refreshing methods in oauth2 require client credentials to work and they use a refresh_token to do so. They are not usable for this use case on their own.

Looks like ReuseTokenSource doesn't use a refresh token it calls .Token() on the provided src interface https://github.com/golang/oauth2/blob/b52af7d5b4e39d5bb1ee067d8aa110fcce9e4cc7/oauth2.go#L310

You can even pass the initial token as nil if you'd like, e.g. ReuseTokenSource(nil, customTokenSourceThatGetsNewTokenFromCloud)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Refresh token logic is here https://github.com/golang/oauth2/blob/b52af7d5b4e39d5bb1ee067d8aa110fcce9e4cc7/oauth2.go#L250, it creates a tokenRefresher (which is an implementation of TokenSource interface).
What I'm suggesting is that we do the same with our own TokenSource interface implementation

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

if cloud env is not set (because plugins don't have the expires_at field in their spec as they assume the user-provided token would be valid for the lifetime of the plugin)

We can:

  1. Set the expiry to a very long time in the future not in a cloud environment
  2. Have our own TokenSource interface always return the client provided token not in a cloud environment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done in 3e909a8

Comment thread helpers/remoteoauth/token.go Outdated
@disq disq requested review from erezrokah and marianogappa August 12, 2024 14:46
Comment thread helpers/remoteoauth/token.go
Comment thread helpers/remoteoauth/token.go Outdated
@disq disq added the automerge label Aug 12, 2024
@kodiakhq kodiakhq Bot merged commit bcd9081 into main Aug 12, 2024
@kodiakhq kodiakhq Bot deleted the feat/cq-cloud-remote-oauth-token branch August 12, 2024 16:00
kodiakhq Bot pushed a commit that referenced this pull request Aug 13, 2024
🤖 I have created a release *beep* *boop*
---


## [4.60.0](v4.59.0...v4.60.0) (2024-08-12)


### Features

* Add RemoteOAuth Token helper to refresh `access_token` from cloud environment ([#1866](#1866)) ([bcd9081](bcd9081))


### Bug Fixes

* **deps:** Update module github.com/cloudquery/plugin-pb-go to v1.22.0 ([#1864](#1864)) ([382f980](382f980))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants