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

refactor: deprecating cookie and start sending token and expirations #3573

Merged
merged 1 commit into from
Oct 11, 2024
Merged

refactor: deprecating cookie and start sending token and expirations #3573

merged 1 commit into from
Oct 11, 2024

Conversation

alsoba13
Copy link
Contributor

@alsoba13 alsoba13 commented Sep 19, 2024 •
edited
Loading

Cookie format apparently introduces a bit of a unwanted complexity. In this PR we deprecate current cookie field in favor of the minimum set of data needed for holding GitHub session.

As a result, we maintain the following body for the GithubLoginResponse and GithubRefreshResponse:

  • cookie: deprecated. In newer versions, cookie won't be created in the backend but in the frontend
  • token: the current active token
  • token_expires_at: needed by the frontend to determine token expiry
  • refresh_token_expires_at: needed by the frontend to determine if refresh token is expired and a new login is required.

Details on expected frontend changes can be found here: grafana/explore-profiles#187

@alsoba13 alsoba13 force-pushed the refactor/token-format-simplification branch 3 times, most recently from a8509a9 to deb1f91 Compare September 19, 2024 08:34
@alsoba13 alsoba13 changed the title refactor: deprecating and start sending and refactor: deprecating a and start sending and Sep 19, 2024
@alsoba13 alsoba13 changed the title refactor: deprecating a and start sending and refactor: deprecating cookie and start sending token and expiresAt Sep 19, 2024
@alsoba13 alsoba13 force-pushed the refactor/token-format-simplification branch 5 times, most recently from 14a4cd1 to 345f4ba Compare September 20, 2024 08:58
@alsoba13 alsoba13 changed the title refactor: deprecating cookie and start sending token and expiresAt refactor: deprecating cookie and start sending token and expirations Sep 20, 2024
@alsoba13 alsoba13 mentioned this pull request Sep 23, 2024
Open
@alsoba13 alsoba13 marked this pull request as ready for review September 23, 2024 07:35
@alsoba13 alsoba13 requested a review from a team as a code owner September 23, 2024 07:35
@alsoba13 alsoba13 force-pushed the refactor/token-format-simplification branch 2 times, most recently from e092515 to 8e13415 Compare September 25, 2024 16:27
@alsoba13 alsoba13 self-assigned this Sep 27, 2024
aleks-p
aleks-p approved these changes Oct 9, 2024
View reviewed changes
Copy link
Contributor

@aleks-p aleks-p left a comment

Choose a reason for hiding this comment

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

Looks good to me. Any thoughts @bryanhuhta ?

pkg/querier/vcs/token.go
@@ -102,14 +107,14 @@ func tokenFromRequest(ctx context.Context, req connect.AnyRequest) (*oauth2.Toke
return token, nil
}

// encodeToken encrypts then base64 encodes an OAuth token.
func encodeToken(token *oauth2.Token, key []byte) (*http.Cookie, error) {
// Deprecated: encodeTokenInCookie creates a cookie by encrypting then base64 encoding an OAuth token.
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps we should explain why this is deprecated and if we have any plans on removing this soon

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, agree. In the past I've seen people leave target dates/releases of when something can get removed. I.e.

Remove this after the v1.11 release

I've found this to be useful because it helps signal to other readers (or myself later) that this bit of code is living on borrowed time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done by linking the frontend issue that does that. It's difficult to estimate a release version where this is not needed and can be deleted, as I have no idea on the priority of the frontend part to be completed

bryanhuhta
bryanhuhta approved these changes Oct 9, 2024
View reviewed changes
Copy link
Contributor

@bryanhuhta bryanhuhta left a comment

Choose a reason for hiding this comment

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

LGTM, a few remarks but nothing blocking.

api/vcs/v1/vcs.proto
@@ -21,13 +21,27 @@ message GithubLoginRequest {
}

message GithubLoginResponse {
// Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's expand on why this is deprecated and possible link to some issues surrounding the deprecation decision. This includes the fact that for now cookie will still have the right value, but at some point in the future, we're going to release a version where cookie isn't used.

This isn't an API that we advertise as supported, so the audience isn't the general public, but rather us down the road.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

api/vcs/v1/vcs.proto
}

message GithubRefreshRequest {}

message GithubRefreshResponse {
// Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

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

Same idea as the above comment.

pkg/querier/vcs/token.go
@@ -102,14 +107,14 @@ func tokenFromRequest(ctx context.Context, req connect.AnyRequest) (*oauth2.Toke
return token, nil
}

// encodeToken encrypts then base64 encodes an OAuth token.
func encodeToken(token *oauth2.Token, key []byte) (*http.Cookie, error) {
// Deprecated: encodeTokenInCookie creates a cookie by encrypting then base64 encoding an OAuth token.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, agree. In the past I've seen people leave target dates/releases of when something can get removed. I.e.

Remove this after the v1.11 release

I've found this to be useful because it helps signal to other readers (or myself later) that this bit of code is living on borrowed time.

pkg/querier/vcs/token.go Outdated
Comment on lines 148 to 150
if err != nil || sessionToken.Token == nil {
// This may be a legacy cookie. Legacy cookies are base64 encoded legacyGitSessionTokenCookie objects.
token, innerErr := decodeLegacyToken(decoded, key)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a blocker, but I think we can finally move away from this. It's safe to say that no one is using the OG session cookie and if they are, they're gonna have to re-auth. We also want to reduce the number "legacy" versions of this cookie we have floating around haha.

Copy link
Contributor Author

@alsoba13 alsoba13 Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In fact, I got rid of the legacy OG session data decoding.

Legacy here refers to the recently deprecated cookie that the PR address. This decode function now works with new format and has this fallback for deprecated cookies. I will rename the function and rewrite the comment to be more clear!

Edit: I did change mentions to legacy tokens to deprecated tokens.

@alsoba13 alsoba13 force-pushed the refactor/token-format-simplification branch from 8e13415 to fbe562a Compare October 11, 2024 10:05
@alsoba13 alsoba13 enabled auto-merge (squash) October 11, 2024 15:25
@alsoba13 alsoba13 merged commit 4f45c61 into main Oct 11, 2024
18 checks passed
@alsoba13 alsoba13 deleted the refactor/token-format-simplification branch October 11, 2024 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aleks-p aleks-p aleks-p approved these changes

@bryanhuhta bryanhuhta bryanhuhta approved these changes

Assignees

@alsoba13 alsoba13

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alsoba13 @aleks-p @bryanhuhta