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

Issue #228: Fix authorization in gRPC Calls #229

Merged
merged 7 commits into from
Aug 27, 2021
Merged

Issue #228: Fix authorization in gRPC Calls #229

merged 7 commits into from
Aug 27, 2021

Conversation

razorao
Copy link
Contributor

@razorao razorao commented Aug 17, 2021

  • Authorization in gRPC calls was broken because project-id was being extracted from the URL and compared against the user's project-id.
  • Implemented a custom UnaryServerAuthInterceptor and serviceAuthFuncOverride interface which declares the AuthFuncOverride function. If the server (read controller) implements the interface function, the override function is called with the request body and the full function name.
  • This implementation is similar to grpc_auth.UnaryServerInterceptor, except that the override function gets the request body.
  • Added the file metro/service/web/server.go which defines methods to extract project-id from the request body.
  • The project-id is extracted from the resource-name such as topic-name, subscription-name using regex capturing-group.

@codecov-commenter
Copy link

codecov-commenter commented Aug 17, 2021

Codecov Report

Merging #229 (799f004) into master (e5dc6fd) will increase coverage by 0.24%.
The diff coverage is 73.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #229      +/-   ##
==========================================
+ Coverage   41.82%   42.07%   +0.24%     
==========================================
  Files          98       99       +1     
  Lines        4368     4409      +41     
==========================================
+ Hits         1827     1855      +28     
- Misses       2385     2398      +13     
  Partials      156      156              
Flag Coverage Δ
integration 28.10% <ø> (ø)
unittests 38.60% <73.33%> (+0.27%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
service/web/adminserver.go 27.63% <ø> (ø)
service/web/publisherserver.go 80.00% <0.00%> (ø)
service/web/subscriberserver.go 57.54% <0.00%> (ø)
internal/interceptors/auth.go 76.78% <44.44%> (-15.95%) ⬇️
service/web/server.go 90.00% <90.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e5dc6fd...799f004. Read the comment docs.

@razorao razorao added ready-for-review bug Something isn't working labels Aug 17, 2021
@razorao razorao removed the bug Something isn't working label Aug 18, 2021
service/web/server.go Outdated Show resolved Hide resolved
service/web/server.go Outdated Show resolved Hide resolved
service/web/server_test.go Outdated Show resolved Hide resolved
service/web/subscriberserver.go Outdated Show resolved Hide resolved
internal/interceptors/auth_test.go Outdated Show resolved Hide resolved
@shivasishdas shivasishdas linked an issue Aug 19, 2021 that may be closed by this pull request
	--Wrote a custom auth interceptor which calls custom AuthFuncOverride with request body
	--Custom version of AuthFuncOverride accepting request body
	--Modified implementations of AuthFuncOverride in admin, publisher and subscriber server to accept request body
	--New file: metro/service/web/server.go having the implementation of fetching the project id from request payload
	--Fetch project-id from request payload using resources' name and extracted using regex capturing group
	--Reverted code commented during testing
Copy link
Contributor

@shivasishdas shivasishdas left a comment

Choose a reason for hiding this comment

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

LGTM

@razorao razorao merged commit 3bb4708 into master Aug 27, 2021
@razorao razorao deleted the grpc_auth branch August 27, 2021 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Authorization in GRPC
4 participants