protobuf-net / protobuf-net.Grpc

GRPC bindings for protobuf-net and grpc-dotnet
Other
867 stars 114 forks source link

Is support for the [Authorize] attribute on the method level possible? #39

Open christiansparre opened 4 years ago

christiansparre commented 4 years ago

I was playing around with the code first approach, really like that over .proto files :)

But I ran into an issue with authorization when adding the [Authorize] attribute to a method in the service implementation. It would simply skip it. It works as expected if you add it on the class level.

It seems that on https://github.com/protobuf-net/protobuf-net.Grpc/blob/927a0ebe23c2f05a6f5bda3a4fa2d7d19db2e382/src/protobuf-net.Grpc.AspNetCore/ServicesExtensions.cs#L63 it uses the interface member to find custom attributes as opposed to the way it does in on the service type where it's the implementation that is used.

I made it work by adding this, finding attributes for the implemented method also

var implementedMethod = typeof(TService).GetMethod(stub.Method.Name, stub.Method.GetParameters()
    .Select(s => s.ParameterType)
    .ToArray());

if (implementedMethod != null) metadata.AddRange(implementedMethod.GetCustomAttributes(inherit: true));

but I'm pretty sure that's a very quick and naive way to fix it 😋 and there is certainly a more robust way to support it.

I was not able to find another way in the TryBind method to access the actual MethodInfo of the implemented type.

mgravell commented 4 years ago

To be very explicit about the scenario here, can you provide an example of the thing that you want to work (but which currently doesn't)? For adding tests, etc - and just to make sure that we're talking 100% the same thing.

On Sun, 10 Nov 2019, 09:23 Christian Sparre, notifications@github.com wrote:

I was playing around with the code first approach, really like that over .proto files :)

But I ran into an issue with authorization when adding the [Authorize] attribute to a method in the service implementation. It would simply skip it. It works as expected if you add it on the class level.

It seems that on https://github.com/protobuf-net/protobuf-net.Grpc/blob/927a0ebe23c2f05a6f5bda3a4fa2d7d19db2e382/src/protobuf-net.Grpc.AspNetCore/ServicesExtensions.cs#L63 it uses the interface member to find custom attributes as opposed to the way it does in on the service type where it's the implementation that is used.

I made it work by adding this, finding attributes for the implemented method also

var implementedMethod = typeof(TService).GetMethod(stub.Method.Name, stub.Method.GetParameters()

.Select(s => s.ParameterType)

.ToArray());

if (implementedMethod != null) metadata.AddRange(implementedMethod.GetCustomAttributes(inherit: true));

but I'm pretty sure that's a very quick and naive way to fix it 😋 and there is certainly a more robust way to support it.

I was not able to find another way in the TryBind method to access the actual MethodInfo of the implemented type.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/protobuf-net/protobuf-net.Grpc/issues/39?email_source=notifications&email_token=AAAEHMCEFFJUTLZQV6WD2X3QS7HJLA5CNFSM4JLLS34KYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HYHLKLQ, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAEHMDEJ46RHI45OSDCIMTQS7HJLANCNFSM4JLLS34A .

christiansparre commented 4 years ago

Sure. I have a "modified version" here https://github.com/christiansparre/protobuf-net.Grpc/tree/christiansparre/authattr of the Client_CS and Server_CS samples. See this commit https://github.com/christiansparre/protobuf-net.Grpc/commit/8fe4dcdbcdf94886dd0f0c84c79e5197b402ae89

Where I have modified the MyCalculator to include the [Authorize] attribute and changed ServicesExtensions.Binder.TryBind to get attributes from MyCalculator instead of only from ICalculator

Without the changes to ServicesExtensions.Binder.TryBind authorization only works with [Authorize] placed on the MyCalculator class and not when placed on MultiplyAsync method. Placing [Authorize] on the method has no effect at all.

Does that cover it?

ronnyek commented 4 years ago

I've placed an authorize attribute in both places, and tried to test a client sending no credentials, and was totally able to bypass authorization...

If this is functionality that's supposed to work today, is there any documentation or samples that demonstrate authentication with protobuf-net.grpc?

mgravell commented 4 years ago

This probably needs investigation. It is on the backlog. It is considered important.

On Thu, 27 Feb 2020, 18:05 Weston, notifications@github.com wrote:

I've placed an authorize attribute in both places, and tried to test a client sending no credentials, and was totally able to bypass authorization...

If this is functionality that's supposed to work today, is there any documentation or samples that demonstrate authentication with protobuf-net.grpc?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/protobuf-net/protobuf-net.Grpc/issues/39?email_source=notifications&email_token=AAAEHMHNEWXCJAQQVV226RDRE76ILA5CNFSM4JLLS34KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENFLEBA#issuecomment-592097796, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAEHMHLNP4XJYEAMEYVDYDRE76ILANCNFSM4JLLS34A .

mgravell commented 4 years ago

please see https://github.com/protobuf-net/protobuf-net.Grpc/pull/68 - I need to validate it etc, but: I'm working on it!

mgravell commented 4 years ago

This should now work on the nuget feed: https://www.myget.org/F/protobuf-net/api/v3/index.json

christiansparre commented 4 years ago

I have just played around with it and it looks great. I have tried different combinations of the [Authorize] attribute, including [AllowAnonymous]. Have also tried authorization with policies and get the correct status codes etc. 👍

mgravell commented 4 years ago

great! I really appreciate you taking the time to validate; this will be in the next drop, which I'll do as soon as I've done a full pass of sanity checks / release notes etc

ronnyek commented 4 years ago

Would you mind letting this thread know when it should be available on the nuget.org feed? Is there some other way to know?

ronnyek commented 4 years ago

Still pending for official nuget release? Is there a version I should be waiting for, or just the next release to nuget should have functional authorization?

mgravell commented 4 years ago

I'll push 1.0.36 to GA; thanks

On Wed, 18 Mar 2020 at 15:43, Weston notifications@github.com wrote:

Still pending for official nuget release? Is there a version I should be waiting for, or just the next release to nuget should have functional authorization?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/protobuf-net/protobuf-net.Grpc/issues/39#issuecomment-600697087, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAEHMFMU3DSSO67B5PK3BDRIDTZJANCNFSM4JLLS34A .

-- Regards,

Marc

ronnyek commented 4 years ago

So I see the nuget was pushed... what's the best way to provide the credentials to the protobuf-net.grpc client? I'm trying to integration test some stuff where services use calls user context, and am unable to set auth token with the following code, but naturally unit testing I'm not using ssl, so it throws an error. Is this the best way, or should I be doing something different?

private static GrpcChannel CreateAuthenticatedChannel(string address, string token)
        {
            var credentials = CallCredentials.FromInterceptor((context, metadata) =>
            {
                if (!string.IsNullOrEmpty(token))
                {
                    metadata.Add("Authorization", $"Bearer {token}");
                }
                return Task.CompletedTask;
            });

            // SslCredentials is used here because this channel is using TLS.
            // CallCredentials can't be used with ChannelCredentials.Insecure on non-TLS channels.
            var channel = GrpcChannel.ForAddress(address, new GrpcChannelOptions
            {
                Credentials = ChannelCredentials.Create(new SslCredentials(), credentials)
            });
            return channel;
        }

Throws:

System.InvalidOperationException : Channel is configured with secure channel credentials and can't use a HttpClient with a 'http' scheme.

Update: I thought trying to pass ChannelCredentials.Inseucre for the channel credentials, and leave the CallCredentials, but that fails because because insecure is incompatible with call credentials

Dunge commented 3 years ago

Hello, I know this is an old issue, but this is supposed to be resolved and working right?

The [Authorize] tag doesn't seems to have any effect, on class or method level for me.

I have a ASP.Net 5 app with both MVC and gRPC. The AddAuthentication/AddAuthorization tag are added before AddGrpc/AddMvc, and UseAuthentication/UseAuthorization before the UseEndpoints.

When using [Authorize] on a MVC controller, it returns 401 Unauthorized if no bearer token is passed, and works if the token is passed.

When using [Authorize] on the gRPC service, it always goes through. If passing a bearer token, I can actually see the claims using CallContext.ServerCallContext.GetHttpContext().User.Claims.

I finally managed to get a Unauthorized error by adding a"logged" policy under AddAuthorization and then configuring the gRPC service using endpoints.MapGrpcService<IService>().RequireAuthorization("logged").

But this block the whole service, the [Authorize] tag doesn't seems to have any effect, My goal would be to block everything except the "Login" method. Do I need to create two separate services?

Dunge commented 3 years ago

I solved it. My error was mapping the service to the interface instead of the real class implementing it in the MapGrpcService method.