Skip to content

Fix authorized_user ADC without explicit scopes. #229

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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>
```
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants