OAuth2: don't use oauth when an empty token is set

[kayrus]

resolves #190

[majormoses]

Would it make sense to print something even if it's only an info or debug level log message to "warn" people. I see the value in making it optional but in all of my use cases across multiple orgs they would be needed and this does not give any feedback to the user that they are not properly configured. Also I would like to point out that while it may be true that its not needed in some small or edgecases an unauthenticated user request gets a max of 60 requests in an hour due to their rate limiting which for many use cases is not enough.

[kayrus]

Could you please provide the output of the case, when the token is required? Which HTTP code does github return? I believe terraform should print the error message in case in the auth is required.
As for the 60 requests in an hour, is it true for GitHub Enterprise as well? Do you suggest to add a warning box in the documentation?

[joebowbeer]

@kayrus the doc says (accurately) that they both are optional, in the sense that while they must be provided, they can also be sourced from environment variables (GITHUB_TOKEN, GITHUB_ORGANIZATION).

Marking them optional in the API allows me to use environment variables without complaint from my IDE.

This is different from saying that a token is optional in order to use the API. That seems like a different change. In my experience a token is required.

[majormoses]

Could you please provide the output of the case, when the token is required?

I have not done it recently but anytime you do anything to a private repo without auth you will get a 404 indicating it does not exist when really its lack of authentication. This is similar to how aws gives a 403 to people obscuring if its lack of permission or missing with s3.

in the sense that while they must be provided, but they can also be sourced from environment variables (GITHUB_TOKEN, GITHUB_ORGANIZATION).

You can just make an ENV var with the prefix of TF_ for those env vars and it works out of the box as that's how I am using it. The benefit of leaving them as "required" is that if the env var is not present it prompts for it. I do agree its technically optional but I think that there are far more use cases where its wanted needed and you can always create a token without any oauth scopes granted.

I am not entirely opposed to allowing it to be optional but I think given that it will more likely negatively impact use cases than it helps I think it minimally needs to be printed out to warn the user that no auth is detected.

[majormoses]

I can't say for sure how it works for github enterprise as I dont currently have one to look at but it seems that it can be enabled for authenticated users: https://help.github.com/en/enterprise/2.16/admin/installation/configuring-rate-limits my gut feeling is that if github enterprise allows unauthenticated requests at all then its probably enabled. You could probably hit:

$ curl https://api.github.com/rate_limit
{
  "resources": {
    "core": {
      "limit": 60,
      "remaining": 60,
      "reset": 1550867832
    },
    "search": {
      "limit": 10,
      "remaining": 10,
      "reset": 1550864292
    },
    "graphql": {
      "limit": 0,
      "remaining": 0,
      "reset": 1550867832
    },
    "integration_manifest": {
      "limit": 5000,
      "remaining": 5000,
      "reset": 1550867832
    }
  },
  "rate": {
    "limit": 60,
    "remaining": 60,
    "reset": 1550867832
  }
}

Replacing the hostname with your github enterprise url

[kayrus]

Here is what I get with the GitHub enterprise.

$ curl https://github.local/api/v3/rate_limit
{
  "message": "Rate limiting is not enabled.",
  "documentation_url": "https://developer.github.com/enterprise/2.14/v3/rate_limit/#get-your-current-rate-limit-status"
}

I am not entirely opposed to allowing it to be optional but I think given that it will more likely negatively impact use cases than it helps I think it minimally needs to be printed out to warn the user that no auth is detected.

What about keeping the github/config.go changes only? When a user explicitly set these parameters to an empty string, the oauth won't be used.

UPD: this is bizarre. Calling the https://api.github.com/users/user/keys is counted by the rate_limit, but https://github.com/user.keys - doesn't 😕

[majormoses]

That would work for me, allow people to opt into disabling authentication as its I think more common to need auth then not. If someone explicitly configures no auth and they need it that's on them.

[majormoses]

We should probably update the docs to indicate that setting this to an empty string will disable oauth

[kayrus]

Does it look fine?

Setting the token to an empty string will lead the GitHub provider to work in an anonymous mode with the corresponding API rate limits.

[joebowbeer]

@majormoses Marking them optional in the API allows me to use environment variables without complaint from my IDE. Otherwise, the IDE is prompting me to create some means other than the env vars for providing these (even though the env vars will still work). For the token, especially, the env var is the best option.

The provider and doc should be in agreement.

[majormoses]

The provider and doc should be in agreement.

Terraform docs or Github's docs? Personally I think that its ok to put guardrails in place that are helpful to users even at the cost of not matching the default of the upstream but that is an opinion and neither are right or wrong. I think this should be up to the community consensus but I don't really like introducing breaking changes that will harm more users than it helps is a good idea. With the proposed implementation setting it to an empty string as a default will work in your editor and you can always override it via an ENV var.

[joebowbeer]

@majormoses How is downgrading from Required to Optional a breaking change?

Assigning the token to "" does not work as you describe. It does placate the IDE, but the empty string is not be replaced by GITHUB_TOKEN.

[majormoses]

Are you using the required TF_VAR_ prefix for terraform to see environment variables? You keep saying it without the prefixes and it's not clear if that's why its not working for you. With the new behavior you could configure something like this:

# configure the provider
provider "github" {
  version      = "1.3"
  token        = "${var.github_token}"
  organization = "${var.github_organization}"
}

variable "github_organization" {
  description = "The github org you want to manage"
  default     = "<REDACTED>"
}

variable "github_token" {
  description = "export TF_VAR_github_token=$MY_TOKEN"
  default = ""
}

$ export TF_VAR_github_token=''<REDACTED>"
# run some terraform command that requires auth and you will see it be set properly
# if you wish to see the current behavior with this setup and setting the env var to an empty string and
# setting a valid token in the default and you you will get a:
# * data.github_team.<REDACTED>: data.github_team.<REDACTED>: GET https://api.github.com/orgs/<REDACTED>/teams?per_page=10: 401 Bad credentials []

By still "requiring" it but discarding if it's empty seems the best compromise to me as it allows you to opt in of using it without auth but for the 99% of use cases will get prompted for credentials should they forget rather than getting a misleading error.

It's breaking in the sense of providing false information back to the user when their authentication is not set and instead tells them that the resource does not exist for example or other rate limit issues outlined without helpful feedback.

[joebowbeerxealth]

I'm using GITHUB_TOKEN as documented by the GitHub provider. If the token is absent then GITHUB_TOKEN is used. (In this sense, token is optional.)

GITHUB_TOKEN is also used in at least one place in the terraform AWS provider code, by the way.

Thanks for pointing out that I can bypass GITHUB_TOKEN by using a different environment variable. I think your suggestion works well in situations where the user may want to run without a token. This option is not available when using GITHUB_TOKEN and running without a token is not something I want to do. I am just annoyed that the IDE thinks token is required.

[majormoses]

Thanks for pointing out that I can bypass GITHUB_TOKEN by using a different environment variable. I think your suggestion works well in situations where the user may want to run without a token. This option is not available when using GITHUB_TOKEN and running without a token is not something I want to do.

By creating your own variable you can choose exactly how you want to handle it, as I would not set an empty string (added that specifically to show its possible) it will require it to come from the ENV or a prompt otherwise it will fail. I think this would also satisfy your editors confusion but I can't say for certain.

GITHUB_TOKEN is also used in at least one place in the terraform AWS provider code, by the way.

Did not know that, from a "quick" search it is only used here other than tests and documentation for the referenced code. It does not use this provider at all, it just uses the same variable to make an auth request. I'd argue it should allowed it to come via a terraform config key or an env var and should not dictate that but that's IMHO out of scope of the discussion as it does not use the provider being referenced here. I think it's worth bringing that up on the aws provider to add flexibility.

I am just annoyed that the IDE thinks token is required.

What IDE? As I said I think your problem could be addressed with or without this change. I don't think that our motivation for making it optional should be because of an IDE.

[joebowbeer]

@majormoses As far as I know the IDE is any IDE with intellisense, which will complain because token and organization are declared Required in this source file, which is different from how they are declared in this documentation, which I thought was the motivation for #190.

[majormoses]

As far as I know the IDE is any IDE with intellisense

I don't see this issue with VScode which has intellisense, but then again as I am configuring it differently I am probably sidestepping your problem entirely.

which will complain because token and organization are declared Required in this source file, which is different from how they are declared in this documentation, which I thought was the motivation for #190.

The motivation for #190 from what I see is to allow you to use it without anything defined for a real use case such as querying a public team on a public github setup. That being said I don't really think there are many real use cases where this actually makes sense as you will very likely hit the 60 requests an hour limit without specifying authentication. It's true that there is a docs mismatch and this happens to bring it into alignment but I see that as a symptom rather than the root of the issue IMHO.

I think this looks good in its current state after some updates to the docs to indicate that its required but could be an empty string which disables authentication and uses anonymous calls.

[majormoses]

UPD: this is bizarre. Calling the https://api.github.com/users/user/keys is counted by the rate_limit, but https://github.com/user.keys - doesn't

I thought about this and I suspect the reason for this is that https://api.github.com/users/majormoses/keys is an API request where https://github.com/majormoses.keys is made against the UI (which in turn makes the appropriate api requests) and therefore are not subject to the same API rate limits. As everything in this module is API driven I still think that a vast majority of use cases really will need authentication in order to be useful even if all the requests are read only to public entities. Just my $0.02.

@kayrus

[kayrus]

@majormoses nevertheless there are cases when github enterprise is used.

Could you please give your feedback on the doc text I mentioned in the https://github.com/terraform-providers/terraform-provider-github/pull/191#issuecomment-466530400 ?

[majormoses]

That doc update works for me.

[kayrus]

@majormoses kindly ping

[majormoses]

@kayrus I can give it a look over but I am not a maintainer for this project just someone who uses it heavily.

[majormoses]

LGTM

[kayrus]

@majormoses there is no maintainer list in the README.md and I don't know who can merge it.

[majormoses]

@radeksimko can you take a look?

[kayrus]

@apparentlymart @aeschright any chance to merge this?

[majormoses]

Im wondering if now that we have null in terraform 0.12 if it might be better to require it to be null instead of a string, unfortunately this would break backwards compatibility with < 0.12 though 🤔

[majormoses]

I thought more about it and I think its important that its a manual opt in for anonymous mode.

[megan07]

Hi @kayrus !

Thank you for this PR! There have been many requests for this, and I think there are some different ways we can go about solving it. After talking to the team, we thought it might be nice to add an anonymous bool on the provider block and validate that it’s either one or the other.

I’ve done a similar thing with organization in order to make it optional, so you can take a look and maybe follow a similar format for this (https://github.com/terraform-providers/terraform-provider-github/pull/242). Or if you don't have the time or inclination to make these changes, let me know, and we can try and pick up the changes ourselves so we can get this merged.

Let me know if you have questions or feedback, or if I can help with anything further.

Thanks!
Megan

[kayrus]

@megan07 acked.

[kayrus]

@megan07 kindly ping

[megan07]

Hi @kayrus !

This looks good! Would you be able to add a few acceptance tests, please? Here are a few things it might be nice to test:

• token and anonymous not set
• no token and anonymous set
• token and anonymous not set
• no token and anonymous not set

Please let me know if you don’t have time to do these tests and I would be happy to write them.

Thanks!

[kayrus]

@megan07 feel free to add tests

[tracypholmes]

Closing in favor of #255.