Skip to content

Fix authorized_user ADC without explicit scopes. #229

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aebrahim
Copy link

Fix a bug where if no scopes are passed in, application default credentials fail in the authorized_user flow.

>>> credentials_app_default()
NULL
>>> credentials_app_default(scopes=c("https://www.googleapis.com/auth/cloud-platform"))
<Token>

Fix a bug where if no scopes are passed in, application default credentials fail in the authorized_user flow.

```
>>> credentials_app_default()
NULL
>>> credentials_app_default(scopes=c("https://www.googleapis.com/auth/cloud-platform"))
<Token>
```
@jennybc
Copy link
Member

jennybc commented Jan 30, 2023

This flow, where a user token is discovered via the ADC strategy is one I know very little about and I've gotten the sense that it's basically deprecated (?). Therefore I also don't have a strong sense of how it should work. I.e., maybe it's "correct" that the user should need to explicitly specify a scope here.

Can you provide a bit more context on the usage that leads to this PR? This code has been in place with no complaint for quite a while, so it would be good to discuss more before a change.

Another solution might be to make "https://www.googleapis.com/auth/cloud-platform" the default value of scopes in this function:

credentials_app_default <- function(scopes = "https://www.googleapis.com/auth/cloud-platform",
                                    ...,
                                    subject = NULL) { ... }

@aebrahim
Copy link
Author

Thank you @jennybc for the review!

As far as GCP goes, I'm not sure if it's deprecated generally because it seems to be a pretty well supported method for performing operations on your local machine with gcloud. For example, this is documented here without any mention of deprecation.

I think this is quite surprising behavior, because I expected that the function would succeed by default in the example I provided. Moreover, this behavior seems to be a bug to me because it is expressed as an incorrectly performed NULL check, as the business logic below shows that the login was going to proceed anyways with the https://www.googleapis.com/auth/cloud.platform scope regardless of what was passed in.

I think the behavior of changing the default scope value of scopes as you suggested is also an equally good solution - whatever makes the most sense for y'all.

@jennybc
Copy link
Member

jennybc commented Jan 30, 2023

I think most R users have a pure R flow, which is why I have so little intuition for how gcloud leaves things. But we do try to make gargle play nicely with gcloud, so it's helpful to hear when it does not.

I have already finished pre-flight checks for a release that is time-sensitive, but I anticipate doing a patch release again rather quickly. I will address this before the patch release, probably by changing the default scopes in this function.

@aebrahim
Copy link
Author

Thank you @jennybc, that's perfect! Please feel free to close this PR

aebrahim added a commit to aebrahim/gargle that referenced this pull request Apr 28, 2023
Replaces r-lib#229.

This fixes a bug where if no scopes are passed in, application default
credentials fail in the authorized_user flow.

```
>>> credentials_app_default()
NULL
>>> credentials_app_default(scopes=c("https://www.googleapis.com/auth/cloud-platform"))
<Token>
```
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.

2 participants