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

examples: add an example for OAuth #10560

Merged
merged 4 commits into from
Sep 18, 2023
Merged

Conversation

sanjaypujare
Copy link
Contributor

No description provided.

@sanjaypujare sanjaypujare requested a review from temawi September 15, 2023 05:41
@Override
public <ReqT, RespT> ServerCall.Listener<ReqT> interceptCall(ServerCall<ReqT, RespT> serverCall,
Metadata metadata, ServerCallHandler<ReqT, RespT> serverCallHandler) {
String value = metadata.get(Constant.AUTHORIZATION_METADATA_KEY);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would change "value" to something like "authorizationHeader".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}
}

serverCall.close(status, new Metadata());
Copy link
Contributor

@temawi temawi Sep 15, 2023

Choose a reason for hiding this comment

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

Add a comment about closing the server call and not allowing any other processing to be done on the request because verification of the header failed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 34 to 36
private Constant() {
throw new AssertionError();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Don't really need this...

Copy link
Contributor Author

@sanjaypujare sanjaypujare Sep 15, 2023

Choose a reason for hiding this comment

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



/**
* Refreshes access token by simply appending ":+1" to the previous value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Mention what a real implementation would generally have to do here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

/**
* Construct client for accessing GreeterGrpc server using the existing channel.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "an"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean "Construct a client..." ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Heh, I meant "using an existing channel"

But now that you mention it, your suggestion should be done as well 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines +28 to +30
* This interceptor gets the OAuth2 access token from metadata, verifies it and sets the client
* identifier obtained from the token into the context. The one check it does on the access token
* is that the token has been refreshed at least once.
Copy link
Contributor

Choose a reason for hiding this comment

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

Talk about what a real implementation would generally need to do to verify the token?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines +19 to +20
import com.google.auth.oauth2.AccessToken;
import com.google.auth.oauth2.OAuth2Credentials;
Copy link
Contributor

Choose a reason for hiding this comment

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

We are relying on Google client APIs here. As a user this would make me think that either:

  • gRPC only supports oauth if I couple my app with Google libraries
  • This example is specific to Google libraries and I don't have a good example of what to do if I don't use Google libraries

Explain somewhere (maybe in README.md) what the deal is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. @ejona86 and I talked about it. I'll add some explanation in the README

Copy link
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
Contributor

@temawi temawi left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together, it looks good!

I left some thoughts in comments, mostly just about comments.

examples/example-oauth/README.md Outdated Show resolved Hide resolved
@sanjaypujare sanjaypujare merged commit 7d9b76e into grpc:master Sep 18, 2023
6 checks passed
@sanjaypujare sanjaypujare deleted the oauth-example branch September 18, 2023 16:56
DNVindhya pushed a commit to DNVindhya/grpc-java that referenced this pull request Oct 5, 2023
@Serenitychic
Copy link

😄

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 19, 2024
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