Conversation
| } | ||
| if usage.StatusCode() != http.StatusOK { | ||
| if u.tokenClient.GetTokenType() == auth.APIKey && usage.StatusCode() == http.StatusForbidden { | ||
| u.logger.Warn().Msg("API Key may have expired. Please see the CloudQuery Console see the expiration status.") |
There was a problem hiding this comment.
I think you could also get a 403 if it's invalid, so maybe we can say
API Key invalid or may have expired. Please see the CloudQuery Console for details.
Better would be to return a specific message from the API if it has expired, and then telling the user so, but this would already be an improvement
There was a problem hiding this comment.
Agreed, That a server side message would be the best. But this at least gives users an initial troubleshooting step
There was a problem hiding this comment.
I'm also unsure of the security implications of revealing whether the key is valid/invalid/expired. I am not aware of any other APIs that will tell you if the the credentials are valid, but expired
There was a problem hiding this comment.
If the API key is invalid (ie they typed it in wrong or added an extra space) looking at the console doesn't tell them anything
There was a problem hiding this comment.
I have seen other APIs that will tell you it's expired. I think it's okay, there's nothing an attacker can do with it anyway, and it's 99.9% of the time going to be useful to the legitimate owner to get that message
There was a problem hiding this comment.
There was a problem hiding this comment.
So it seems like we already have a custom error message for expired tokens https://github.com/cloudquery/cloud/blob/b830bf0f69a3e070b9ace1556f03e79d30eaf034/platform/internal/response/response.go#L129C1-L130C1
There was a problem hiding this comment.
What's left to do here? Do we print the correct message or not?
|
Closing as stale. Please re-open if needed |
4 participants
Summary
Use the following steps to ensure your PR is ready to be reviewed
go fmtto format your code 🖊golangci-lint run🚨 (install golangci-lint here)