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

Fix basic auth on OAuth2 client credentials flow #485

Merged
merged 7 commits into from
Dec 4, 2018
Merged

Fix basic auth on OAuth2 client credentials flow #485

merged 7 commits into from
Dec 4, 2018

Conversation

peterhartman
Copy link
Contributor

It looks like init_oauth2.0 is not passing use_basic_auth onwards. I tried (and failed) to create a test for the client credentials flow using httpbin. I can't find a public API suitable for testing against unfortunately.

@peterhartman
Copy link
Contributor Author

I finally managed to create a test using a dummy vimeo user. Phew!

I have confirmed that the test fails without my proposed change.

@chicofish

This comment has been minimized.

@cderv

This comment has been minimized.

@chicofish

This comment has been minimized.

@cderv

This comment has been minimized.

@chicofish

This comment has been minimized.

@GregoireCoppens

This comment has been minimized.

@peterhartman

This comment has been minimized.

@peterhartman

This comment has been minimized.

@hadley

This comment has been minimized.

@hadley
Copy link
Member

hadley commented Nov 22, 2018

@peterhartman for future reference, if you need me to look into a PR, including a brief summary of why it's important would be extremely helpful. It seems like this is a major frustration, but I read a lot of issues and it's easy for important stuff to get lost in the mix if it's not clearly labelled.

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 NEWS? It should briefly describe the change and end with (@yourname, #issuenumber).

@@ -25,6 +25,34 @@ test_that("oauth2.0 signing works", {
)
})

test_that("oauth2.0 basic auth on client credentials works", {
Copy link
Member

Choose a reason for hiding this comment

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

I'm worried that this is too fragile to include in a test - it will break if vimeo is down. Can you please instead update the vimeo demo? (updating it to use oauth2.0, including changing the name of the file)

Copy link
Member

Choose a reason for hiding this comment

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

i.e. fix #491

@hadley hadley merged commit d17a69a into r-lib:master Dec 4, 2018
@hadley
Copy link
Member

hadley commented Dec 4, 2018

Thanks for your help with this @peterhartman!

@peterhartman peterhartman deleted the peterhartman-oauth2-clientcredentials branch December 13, 2020 19:32
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.

5 participants