fix: cloud session refresh using refresh token #507
Conversation
vet Summary ReportThis report is generated by vet Policy Checks
Malicious Package AnalysisMalicious package analysis was performed using SafeDep Cloud API Malicious Package Analysis Report
Changed PackagesChanged Packages
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #507 +/- ##
==========================================
- Coverage 17.31% 17.28% -0.04%
==========================================
Files 175 175
Lines 16790 16849 +59
==========================================
+ Hits 2908 2913 +5
- Misses 13659 13710 +51
- Partials 223 226 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cmd/cloud/utils.go
Outdated
| // is less than equal to 10 hrs or not(Here 10hrs was taken because it was observer that most of the JWT token had similar expiry time) | ||
| // 2. It will parse the JWT token to get the expiry time and then check for it expiry | ||
| func checkIfNewAccessTokenRequired(config auth.Config) (bool, error) { | ||
| if time.Since(config.CloudAccessTokenUpdatedAt) <= 10*time.Hour { |
There was a problem hiding this comment.
Whats the rationale of using 10 hour as the window? Especially since later in the code we are actually checking the JWT exp
There was a problem hiding this comment.
The reason for having this two level of checks was to leverage the CloudAccessTokenUpdatedAt field that we store on the user side .. so In this case
- Using
CloudAccessTokenUpdatedAtfield is current time is less than 10 hrs when the last time access token was updated .. we don't go ahead to check the JWT check token .. - If the first check is not true then only we check for JWT token expiry
The reason to add the first 10 hour window check was if it is not expired there will be no need to parse JWT token and check for it expiry .
But as I was thinking more on this . I believe JWT expiry check is more future proof ..
So let me know if we want to keep both the checks or just the JWT expiry check ?
Signed-off-by: sachin maurya <sachin.maurya7666@gmail.com>
Signed-off-by: sachin maurya <sachin.maurya7666@gmail.com>
Signed-off-by: sachin maurya <sachin.maurya7666@gmail.com>
1c7cbf8 to
e93e5dd
Compare
cmd/cloud/main.go
Outdated
| @@ -41,6 +42,10 @@ func NewCloudCommand() *cobra.Command { | |||
| if tenantDomain != "" { | |||
| auth.SetRuntimeCloudTenant(tenantDomain) | |||
| } | |||
| err := auth.RefreshAccessToken() | |||
There was a problem hiding this comment.
@slayer321 Have you verified the flow? It seems to me this will cause problem because it will run even first time when a user does vet cloud login and will fail, show the error and cause a poor user experience. The first time user will likely be confused with the error message.
There was a problem hiding this comment.
yes, I check the flow. But miss the part what if user running the command for the fist time.. In my case I started with the quickstart command.. and while running login command I didn't got error . But yes if there is new user and running the login command it will be poor user experience.
I'm thinking we can add a check here to skip the login and quickstart for calling RefreshAccessToken function.
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| body, _ := io.ReadAll(resp.Body) |
There was a problem hiding this comment.
Is this required, especially since we have deferred resp.Body.Close()?
There was a problem hiding this comment.
I think yes, though we are deferring the resp.Body.Close() which will close stream at the end of the function.. but what if we didn't get http statusCode as StatusOK ... in that case we just want to read the response body and return the error .
…or refreshaccesstoken check Signed-off-by: sachin maurya <sachin.maurya7666@gmail.com>
Signed-off-by: sachin maurya <sachin.maurya7666@gmail.com>
fixes #398
two conditions are check before getting new access token