-
Notifications
You must be signed in to change notification settings - Fork 705
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.
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
Handle grpc authentication errors #5297
Handle grpc authentication errors #5297
Conversation
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
✅ Deploy Preview for kubeapps-dev canceled.Built without sensitive environment variables
|
if (e.constructor === UnauthorizedError) { | ||
dispatch(actions.auth.logoutByAuthenticationError()); | ||
} else { | ||
dispatch( | ||
errorInstalledPackage( | ||
new FetchWarning( | ||
"this package has missing information, some actions might not be available.", | ||
), | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is actions.auth.logoutByAuthenticationError()
repeated in many places.
I was wondering if we could refactor that to be generic with something like e.g. (pseudo-code):
export function handledErrorAction(err: ActionType<any>, e: any){
if (e.constructor === UnauthorizedError) {
return actions.auth.logoutByAuthenticationError();
} else {
return err;
}
}
So that invocations can be almost a one-liner like e.g.:
dispatch(
handledErrorAction(errorInstalledPackage(new FetchError("Unable to list apps", [e])), e)
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, thanks! Will do!
Also, I'd like to explore the grpc interceptors at some point. Don't know if they can fit our main goal here, though. https://grpc.io/blog/grpc-web-interceptor/
return new NotFoundError(msg); | ||
case grpc.Code.AlreadyExists: | ||
return new ConflictError(msg); | ||
case grpc.Code.InvalidArgument: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InvalidArgument
does not look like an internal server error? Maybe an error like e.g. UnprocessableEntity
fits better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not, of course. This is tricky, as we aren't really actually using those error objects... so I just defaulted them to "server error" with no specific criteria.
Let's create error objects grouping some of them, as we may want to have them in the future. See what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with creating error objects that extend CustomError
so that we can have some groups.
case grpc.Code.FailedPrecondition: | ||
case grpc.Code.Internal: | ||
//TODO(agamez): this code shouldn't be returned by the API, but it is | ||
if (["credentials", "unauthorized"].some(p => msg?.toLowerCase()?.includes(p))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit weak, but I guess it is the only thing we can do until all error codes from backend are harmonized.
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com> Conflicts: dashboard/src/shared/PackagesService.ts
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
Signed-off-by: Antonio Gamez Diaz <agamez@vmware.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
To investigate: using goharbor.com chart repo+ credentials + kicks me out, verify which grpc code we are returning when adding repos. |
Tested the case, and it is returning a GRPC status 9 (Failed precondition).
That wraps the HTTP 401 returned by Harbor, indeed. Maybe we need to process the 401 error in the backend and map it correctly. If it is a 401 that Harbor sends, I would return a grpc status 7 (Permission denied) or 3 (Invalid argument). |
Description of the change
As reported at #5295, the axios interceptor handling the logout does not work anymore (bc all the requests are handled by the grpc client).
This PR is to intercept the auth-related errors to force a programmatic logout.
Benefits
If an auth error happens, we will log the user out.
Possible drawbacks
Maybe we don't want to kick the user out in some points, but we can fine-tune it later.
Applicable issues
Additional information
Problem: I've found some grpc calls NOT returning the proper status code... this forced me to add some ugly workarounds looking for certain keywords to appear to detect a true auth error.
Demo:
WIP as there are still several test cases to fix