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.

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

Use HTTP code to figure out appropriate GrpcError if no grpc-status header exists #421

Closed
rohitramkumar opened this issue Jan 15, 2021 · 2 comments

Comments

@rohitramkumar
Copy link

rohitramkumar commented Jan 15, 2021

I have a setup where my Dart frontend is calling out to an API fronted by Envoy. The Envoy performs a JWT + RBAC check before forwarding requests to the backend. If the JWT is not valid or the user is denied as per the RBAC config, then Envoy returns a 403 but it does not return the grpc-status header. As far as I can tell, the underlying libraries here are specifically looking for this header to figure out what kind of GrpcError to return (which seems reasonable). However, in cases where no header is returned, I feel it should look to the HTTP code to make the decision. Right now, the transport layer simply calls Grpc.unavailable() if the HTTP code is not 200 [1]. Since this code seems to be already looking at headers, can we make this logic a bit smarter?

Specifically, I'm thinking that if HTTP code is not 200, then have a large switch statement that maps the HTTP code to the proper GrpcError. Would be happy to send a PR if this sounds reasoanable. FWIW, there seems to be some precedent for a change like this (grpc/grpc-web#619).

[1] https://github.com/grpc/grpc-dart/blob/master/lib/src/client/transport/xhr_transport.dart#L118

@LeFrosch
Copy link
Contributor

I’m currently facing a similar issue. My dart client tries to connect to a server which also uses JWT based authentication. The client uses an API which returns a stream of results but if the authentication fails no error gets thrown at all. The ResponseStream is just empty.
I think it would be more intuitive to throw an error in this case instead of returning an empty stream. The responsible code snippet can be found here. I’m sorry if this problem is caused by me, I’m quite new to grpc and dart.

@mraleph
Copy link
Member

mraleph commented Mar 18, 2021

Yeah it seems we are not doing enough checking when receiving responses (see also #458 which is caused by lack of checks in the parser that handles native gRPC responses).

We would be happy to take PRs that fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants