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

Remove OAuth token from query params in API requests #180

Merged

Conversation

shivanesabharwal
Copy link
Contributor

  • we were redundantly setting the OAuth Token in query parameter via BigQueryRequestUserAgentInitializer#initializeBigqueryRequest.
  • The HttpRequestTimeoutInitializer which builds the HttpRequest was already setting it in the header of the request, which is the accepted method.
  • remove the call to setOauthToken and associated code which is no longer required.
  • in service of http://b/294919784

this PR was reverted because we did not do a full integration test between Helltool and BigQuery OAuth with these changes, and we wanted to unblock other PRs. Now, I'm revisiting this issue

@shivanesabharwal
Copy link
Contributor Author

Looking to merge this again... Reverted because we were cutting a new release before this was tested.

Copy link
Member

@tjbanghart tjbanghart left a comment

Choose a reason for hiding this comment

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

LGTM

@goomrw goomrw force-pushed the revert-176-revert-171-shivane/removeOAuthTokenFromQueryParams branch from b299a4c to d91f423 Compare January 13, 2024 00:15
Looker accesses this private API via reflection, so
we need to keep it.  This doesn't revert the
fundamental point of this PR, which is to stop
setting the OAuth token as a BigQuery API parameter.
@goomrw goomrw merged commit 2378a7e into main Jan 17, 2024
2 checks passed
@goomrw goomrw deleted the revert-176-revert-171-shivane/removeOAuthTokenFromQueryParams branch January 17, 2024 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants