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

alts: add client authorization util library #6529

Merged
merged 2 commits into from
Dec 18, 2019

Conversation

jiangtaoli2016
Copy link
Contributor

Add a simple authorization utility library.

@ejona86

@jiangtaoli2016 jiangtaoli2016 self-assigned this Dec 17, 2019
if (altsCtx == null) {
return Status.NOT_FOUND.withDescription("Peer ALTS AuthContext not found");
}
for (String serviceAccount : expectedServiceAccounts) {
Copy link
Member

Choose a reason for hiding this comment

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

if (expectedServiceAccounts.contains(altsCtx.getPeerServiceAccount())) {
  return Status.OK;
}

public static Status clientAuthorizationCheck(
ServerCall<?, ?> call, ImmutableList<String> expectedServiceAccounts) {
AltsAuthContext altsCtx =
(AltsAuthContext) call.getAttributes().get(AltsProtocolNegotiator.AUTH_CONTEXT_KEY);
Copy link
Member

Choose a reason for hiding this comment

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

Why isn't AUTH_CONTEXT_KEY a Attributes.Key<AltsAuthContext>?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This because extractPeerObject() returns an Object, rather than AltAuthContext. Internally, we have a different AuthContext class. We need to use share the same API for returning the peer object. Let me know if you have better ideas.


@Override
public void request(int numMessages) {
throw new AssertionError("Should not be called");
Copy link
Member

Choose a reason for hiding this comment

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

A trick you can do for these is to extend ForwardingServerCall and then implement delegate() to throw like you're doing here. This is fine, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack on the trick. Let's keep the current FakeServerCall if you don't mind.

@jiangtaoli2016
Copy link
Contributor Author

thanks @ejona86 @elharo for comments.

@ejona86 Please take a look again.

Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

It's a bit weird to require a List when a Set would work (which Collection would allow), but this seems fine.

@jiangtaoli2016
Copy link
Contributor Author

Changed to Collection instead of List.

@jiangtaoli2016 jiangtaoli2016 merged commit 04e1c9d into grpc:master Dec 18, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Mar 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants