refactor: deprecating cookie and start sending token and expirations
#3573
Conversation
alsoba13
force-pushed
the
refactor/token-format-simplification
branch
3 times, most recently
from
a8509a9 to
deb1f91
Compare
alsoba13
changed the title
a and start sending and
alsoba13
changed the title
a and start sending andcookie and start sending token and expiresAt
alsoba13
force-pushed
the
refactor/token-format-simplification
branch
5 times, most recently
from
14a4cd1 to
345f4ba
Compare
alsoba13
changed the title
cookie and start sending token and expiresAtcookie and start sending token and expirations
alsoba13
force-pushed
the
refactor/token-format-simplification
branch
2 times, most recently
from
e092515 to
8e13415
Compare
| @@ -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. | |||
There was a problem hiding this comment.
perhaps we should explain why this is deprecated and if we have any plans on removing this soon
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| @@ -21,13 +21,27 @@ message GithubLoginRequest { | |||
| } | |||
|
|
|||
| message GithubLoginResponse { | |||
| // Deprecated | |||
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| message GithubRefreshRequest {} | ||
|
|
||
| message GithubRefreshResponse { | ||
| // Deprecated |
There was a problem hiding this comment.
Same idea as the above comment.
| @@ -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. | |||
There was a problem hiding this comment.
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
| if err != nil || sessionToken.Token == nil { | ||
| // This may be a legacy cookie. Legacy cookies are base64 encoded legacyGitSessionTokenCookie objects. | ||
| token, innerErr := decodeLegacyToken(decoded, key) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
force-pushed
the
refactor/token-format-simplification
branch
from
8e13415 to
fbe562a
Compare
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
GithubLoginResponseandGithubRefreshResponse:cookie: deprecated. In newer versions, cookie won't be created in the backend but in the frontendtoken: the current active tokentoken_expires_at: needed by the frontend to determine token expiryrefresh_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