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.

Sign up for GitHub

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

Jump to bottom

Implement resource owner credentials #7424

Merged
merged 6 commits into from
Feb 23, 2019
Merged

Implement resource owner credentials #7424

merged 6 commits into from
Feb 23, 2019

Conversation

Blacksmoke16
Copy link
Member

@Blacksmoke16 Blacksmoke16 commented Feb 13, 2019 •
edited
Loading

Resolves #7086

  • Orders methods in order they appear in the RFC
  • Implements get_access_token_using_resource_owner_credentials
  • Adds "Content-Type" => "application/x-www-form-urlencoded" to better adhere to the RFC.

I think it would be nice to test the get_access_token_using_* methods create the proper request. However not sure how to best handle that without setting up a mock HTTP server...

wooster0
wooster0 reviewed Feb 13, 2019
View reviewed changes
src/oauth2/client.cr Outdated Show resolved Hide resolved
@straight-shoota
Copy link
Member

However not sure how to best handle that without setting up a mock HTTP server...

There is nothing wrong with that. OAuth is an HTTP-based protocol, so in order to test it, you'll need an HTTP server.
You could get around going through the network stack by modifying get_access_token in some way to make it not strictly rely on HTTP::Client.get, but I don't think that's worth it.
Just spin up an HTTP server for testing. Examples for this can be found in the HTTP specs.

@asterite
Copy link
Member

It would be nice not having to spin up an HTTP::Server for testing what request should be generated. We need some kind of thing that we can mock that receives a request and returns a response. Maybe that concept is called a Transport in Go, I'm not sure.

In any case, for now I guess the only way is spinning up an HTTP::Server...

@Blacksmoke16
Copy link
Member Author

Blacksmoke16 commented Feb 13, 2019 •
edited
Loading

👍 I can maybe look into doing that in another PR.

I used https://github.com/manastech/webmock.cr in the past. Was pretty slick.

@straight-shoota
Copy link
Member

@asterite Agreed. But we don't have a mockable HTTP::Client yet. It's one of the features suggested in #6011

@Blacksmoke16
Copy link
Member Author

@straight-shoota @asterite I added some specs for the getting an access token. Not sure what else could be done to make it better, but i'm open to ideas.

@straight-shoota
Copy link
Member

I'm sorry, I thought #7197 was already merged into master, but it isn't. Please take a look at the changes in this PR to avoid leaking fibers and sockets from the spec.

@Blacksmoke16
Copy link
Member Author

@straight-shoota Done. I created the http/spec_helper in my branch as well and used the setup there to reuse some code.

Shouldn't conflict assuming no further changes are made to it.

@asterite asterite added this to the 0.28.0 milestone Feb 22, 2019
@asterite asterite merged commit 2225609 into crystal-lang:master Feb 23, 2019
@Blacksmoke16 Blacksmoke16 deleted the oauth2-password-grant branch February 23, 2019 16:32
urde-graven pushed a commit to urde-graven/crystal that referenced this pull request Feb 25, 2019
@Blacksmoke16
Implement resource owner credentials (crystal-lang#7424)
d981a95 
* Implement resource owner credentials

* Fix link name

* get_access_token_using_*

* remove headers from response

* Address comments

* Display timeout value in exception
urde-graven pushed a commit to urde-graven/crystal that referenced this pull request Feb 27, 2019
Merge branch 'fixes'
16033a1 
* fixes:
  Foreign exceptions: basic support
  Added support for `.so' libraries, fixed segfault, small bugs
  OptionParser: optional options and arguments shifting
  XML: workaround for bug in libxml2 2.9.9 (crystal-lang#7477)
  Implement resource owner credentials (crystal-lang#7424)
  Implement #annotations (crystal-lang#7326)
  Handle signals in a separate fiber
  Compiler: reactively compute a union's type, and check for missing types
  Compiler: fix as? casting when target doesn't have a type yet
  Compiler: fix as casting when target doesn't have a type yet
  Compiler: give pare error when assigning a constant inside a multiassign
  Format: fix indent of nested array elements (crystal-lang#7450)
  Disable double write buffering in OpenSSL sockets (crystal-lang#7460)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wooster0 wooster0 wooster0 left review comments

@asterite asterite asterite approved these changes

@straight-shoota straight-shoota straight-shoota approved these changes

Assignees
No one assigned
Labels
kind:feature topic:stdlib:networking
Projects
None yet
Milestone
0.28.0
Development

Successfully merging this pull request may close these issues.
4 participants
@Blacksmoke16 @straight-shoota @asterite @wooster0