Skip to content

Expose AuthContext in C##9632

Merged
jtattermusch merged 1 commit into
grpc:masterfrom
jtattermusch:csharp_server_side_auth
Mar 7, 2017
Merged

Expose AuthContext in C##9632
jtattermusch merged 1 commit into
grpc:masterfrom
jtattermusch:csharp_server_side_auth

Conversation

@jtattermusch
Copy link
Copy Markdown
Contributor

Expose C core's auth context in C#, ready for initial review.

@jboeuf for general sanity
@apolcyn for C#

@nathanielmanistaatgoogle FYI

@jtattermusch
Copy link
Copy Markdown
Contributor Author

@apolcyn this should be ready for review.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra blank line here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

Comment thread src/csharp/Grpc.Core/AuthProperty.cs Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: s/"string of value"/"string value"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment thread src/csharp/Grpc.Core/AuthProperty.cs Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: s/"and instance"/"an instance"`

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: IMO only but maybe simpler as:

                if (!propertiesDict.ContainsKey(authProperty.Name))
                {
                    propertiesDict[authProperty.Name] = new List<AuthProperty>();
                }
                propertiesDict[authProperty.Name].Add(authProperty);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small wording nits on summary comments. s/a/an

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@jtattermusch jtattermusch force-pushed the csharp_server_side_auth branch from ada5104 to 31cf5b2 Compare February 11, 2017 02:27
Copy link
Copy Markdown
Contributor Author

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments addressed.

Copy link
Copy Markdown
Contributor

@apolcyn apolcyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comments I accidentally left out - I think. sorry ignore if already saw these

Comment thread src/csharp/Grpc.Core/AuthProperty.cs Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

always safe to assume the value read out is UTF8 encoded?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: for comment, 'Gets the string value of the property as a UTF8 decoding of the value's bytes.' ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me that's an implementation detail. Normally, the auth property will be created by gRPC and users will just read string values and from that perspective, the original encoding is not really important.

Comment thread src/csharp/Grpc.Core/AuthProperty.cs Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since it looks like AuthProperty only created within grpc and passed up to the app, does the Create method here need to be exposed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is useful for testing, otherwise there is no way for the users to create instances of AuthProperty.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For that, need the public constructor for AuthContext too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd better not expose the public constructor for AuthContext for now. Once a constructor is published, it's hard to change the internals of the auth context if changes are necessary. We can later add the constructor once the existing implementation proves to be working well for people.

Comment thread src/csharp/Grpc.Core/AuthProperty.cs Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: for comment, 'Gets the string value of the property as a UTF8 decoding of the value's bytes.' ?

Comment thread src/csharp/Grpc.Core/AuthContext.cs Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more idiomatic to use the Enumerable<AuthProperty>?

one thought on API is it seems AuthContext could be more like a dictionary.

basically Dictionary<string, List<Property>> could be more like Dictionary<string, List<Value>>?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, ideally properties should be a read only multi-map, but unfortunately C# libraries don't have a type for multimap nor they have the right type that would allow use to preserve immutability. So what I'm exposing is my best attempt to make the API clear and preserve all the properties I want it to have.

Comment thread src/csharp/Grpc.Core/AuthProperty.cs Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For that, need the public constructor for AuthContext too?

@jtattermusch
Copy link
Copy Markdown
Contributor Author

I believe I've addressed/answered all comments.
Also rebased against the latest master and added a note that AuthContext/AuthProperty api is experimental for now (so we have a bit of freedom to make adjustments to the API if needed).
Btw, master now has a high-level doc about server side auth (docs/server_side_auth.md)

PTAL.

@jtattermusch
Copy link
Copy Markdown
Contributor Author

@jtattermusch
Copy link
Copy Markdown
Contributor Author

jtattermusch commented Mar 6, 2017

@dgquintas FYI I'd like to merge this one before cutting the 1.2.x release branch (I need a final LGTM)

Copy link
Copy Markdown
Contributor

@apolcyn apolcyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dgquintas
Copy link
Copy Markdown
Contributor

@jtattermusch in that case add the "release blocker" label

@jtattermusch jtattermusch merged commit efe7572 into grpc:master Mar 7, 2017
@lock lock Bot locked as resolved and limited conversation to collaborators Jan 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants