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

Support for additional claims in the JWT tokens, needed for Google's Identity-Aware Proxy #570

Closed
wants to merge 4 commits into from

Conversation

kalugny
Copy link

@kalugny kalugny commented Feb 7, 2019

Hi and thanks for an excellent package!

We're using Google Cloud's Identity-Aware Proxy (IAP) for which httr is almost compatible.
The only discrepancy is that IAP requires an additional "claim" on the JWT.

This pull requests:

  1. Adds support for additional claims on the JWT, which you can pass as additional parameters to oauth_service_token
  2. Supports getting the authentication header from the id_token if access_token is not set (which is the case for IAP tokens)

Using this for IAP is now very simple:

token <- oauth_service_token(
  endpoint = oauth_endpoints("google"),
  secrets = jsonlite::fromJSON('/path/to/credentials.json'),
  target_audience = iap_oauth_client_id
)

@kalugny
Copy link
Author

kalugny commented Feb 7, 2019

@kalugny
Copy link
Author

kalugny commented Feb 20, 2019

@hadley any comments? Should I change something?
It's a simple extension that would support more use-cases.

Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

Can you please add a bullet to the top of NEWS.md? It should briefly describe the change and end with (@yourname, #issuenumber).

DESCRIPTION Outdated
@@ -1,6 +1,6 @@
Package: httr
Title: Tools for Working with URLs and HTTP
Version: 1.4.0.9000
Version: 1.4.1.9000
Copy link
Member

Choose a reason for hiding this comment

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

Please don't change the version number

@@ -42,6 +43,7 @@ init_oauth_service_account <- function(secrets, scope = NULL, sub = NULL) {
#' 00:00:00 UTC, January 1, 1970. This value has a maximum of 1 hour from
#' the issued time.
#' @param duration Duration of token, in seconds.
#' @param ... any additional claims for the token.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#' @param ... any additional claims for the token.
#' @param ... Any additional claims for the token.

@@ -301,6 +301,7 @@ Token2.0 <- R6::R6Class("Token2.0", inherit = Token, list(
#' @inheritParams oauth2.0_token
#' @inheritParams jwt_signature
#' @param secrets Secrets loaded from JSON file, downloaded from console.
#' @param ... any additional claims for the token
Copy link
Member

Choose a reason for hiding this comment

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

I think you can remove this line since we're already inheriting docs from jwt_signature?

@@ -311,7 +312,7 @@ Token2.0 <- R6::R6Class("Token2.0", inherit = Token, list(
#'
#' token <- oauth_service_token(endpoint, secrets, scope)
#' }
oauth_service_token <- function(endpoint, secrets, scope = NULL, sub = NULL) {
oauth_service_token <- function(endpoint, secrets, scope = NULL, sub = NULL, ...) {
Copy link
Member

Choose a reason for hiding this comment

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

Using ... here seems a bit broad. I'd think it should be claims or similar?

@hadley
Copy link
Member

hadley commented Oct 31, 2023

httr has been superseded in favour of httr2, so is no longer under active development. Thanks for using httr, thanks for contributing, and my apologies that your contribution never made it into the package.

@hadley hadley closed this Oct 31, 2023
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