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

Add support for scopes to terraform login #26239

Merged
merged 4 commits into from
Sep 16, 2020
Merged

Conversation

jceresini
Copy link
Contributor

@jceresini jceresini commented Sep 14, 2020

Support was previously added in terraform_svchost (hashicorp/terraform-svchost#5)

This PR pulls in that version of terraform_svchost and adds support to terraform to include the scope(s) in the URL generated by terraform login [hostname]

Fixes #25354

@codecov
Copy link

codecov bot commented Sep 14, 2020

Codecov Report

Merging #26239 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
command/login.go 51.18% <100.00%> (+0.23%) ⬆️
dag/walk.go 92.54% <0.00%> (-0.63%) ⬇️

Copy link
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

Thanks for working on this, @jceresini! I edited the PR description to link to the relevant issue.

Rereading Martin's comment on the issue, I think there are a couple of steps remaining for this implementation:

(This is done)

(Should also do this in the password grant type handler for consistency, even though the only server we allow for that right now doesn't use it.)

TODO

  • Add a new permutation to the terraform login tests to verify that the scope passes through correctly when discovery includes it, and is omitted when discovery doesn't include it.

TODO


Unless any other issues come up, I think we should be able to merge this once those two things are addressed. Hope that makes sense!

@jceresini
Copy link
Contributor Author

@alisdair I believe that is now addressed. I made separate tests for validating that the scopes made it to the disco.OAuthClient because I thought that would be easier to understand.

Copy link
Contributor

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

Thanks! 🎉

@alisdair alisdair merged commit b5ef33b into hashicorp:master Sep 16, 2020
@ghost
Copy link

ghost commented Oct 17, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for scopes in terraform login / login.v1 protocol
2 participants