-
Notifications
You must be signed in to change notification settings - Fork 434
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
BearerTokenAuthenticationToken not converter to JwtAuthenticationToken #343
Comments
Thanks @skjolber , I'll have a look after I return from vacation. |
@jvmlet
We're using a filter between 2 and 3 to do request-response-logging and generic security checks in 2 (i.e. fully authenticated). What do you think about splitting up the |
Have a look at base class of |
Not really? |
And if I expose keys of GrpcContext as public (from security interceptor)and you can get the values in your events listeners? |
Aiming for something easier to understand and that does things the same way as Spring security, the security interceptor should be split? More specifically, the roles of
Seems like |
@jvmlet what do you think about splitting the security interceptor? |
@skjolber , sorry for the late response. Can you please elaborate more on the logic you you are after between authenticate and authorize? |
@jvmlet so for spring web in the above 1-3:
There is two possible places to put the request-response logging, some like to do this for all requests from the internet, others only want those with a valid token (of any sort). For both one wants to add security context (i.e. user) when available. So the security context matters even if the actual endpoint accessed is open. A key point is to avoid unwanted deserialization of objects for requests without an authorization. This reduces the work load from "spam from internet" and at the same time stops evil parties from taking advantage of deserialization bugs. While this does not protect from evil users whom have authorization, for closed systems like ours this is a big improvement. |
I do not think the custom voter will work. |
@jvmlet if you're onboard with the motivation / pattern, should I move on with a PR? This probably is a breaking change; new major version material. |
@skjolber , few clarifications :
If this is the reason you need custom logic between Also, when you say this ( So, assuming you still need to execute custom logic between 1,2 and 3 and you are going to setup your custom interceptor, I assume you will NOT setup listener, because till then the message will be deserialized already and this it what you want to avoid. And if you don't setup listener, then you have access to Anyway, if we move on with PR, I don't see this change as a breaking change... |
Use of request response logging might depend on environment, tracing headers, service/method, payload size, service maturity and so on, so requests may or may not be logged. I am not sure how the protobuf-to-JSON is implemented, but it does not really need to create the underlying objects? I'll try to make a PR and hopefully things will be more clear then. |
@jvmlet So to align the gRPC interceptors more to the web filter chain (in the above stacktrace), and move away from the deprected AbstractSecurityInterceptor, what do you think about splitting the current implementation into multiple filters/interceptors like this? |
Seems that in services which are open,
BearerTokenAuthenticationToken
flows through to the service, in the security context.The right behaviour would be to convert any present
BearerTokenAuthenticationToken
toJwtAuthenticationToken
.Compare spring web with SecurityInterceptor; in spring web the conversion always happens, but in the gRPC equivalent it depends on the method metadata.
The text was updated successfully, but these errors were encountered: