-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Conversation
if (altsCtx == null) { | ||
return Status.NOT_FOUND.withDescription("Peer ALTS AuthContext not found"); | ||
} | ||
for (String serviceAccount : expectedServiceAccounts) { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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>
?
There was a problem hiding this comment.
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"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
3288402
to
044a53d
Compare
There was a problem hiding this 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.
Changed to Collection instead of List. |
Add a simple authorization utility library.
@ejona86