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

Added Assertion Flow #249

Closed
wants to merge 14 commits into from
Closed

Added Assertion Flow #249

wants to merge 14 commits into from

Conversation

tsov
Copy link

@tsov tsov commented Jun 25, 2013

Here a simple implementation for an Assertion Flow for Doorkeeper.

This lets you define a method to authenticate users coming from 3rd Party applications like facebook, twitter, etc.

Example:

Doorkeeper.configure do
    resource_owner_from_assertion do
        facebook = URI.parse('https://graph.facebook.com/me?access_token=' + params[:assertion])
        response = Net::HTTP.get_response(facebook)
        user_data = JSON.parse(response.body)
        User.find_by_facebook_id(user_data['id'])
    end
end

@tsov tsov mentioned this pull request Jun 25, 2013
@foFox
Copy link

foFox commented Jun 26, 2013

+1

Max Hoffmann and others added 4 commits June 26, 2013 17:30
@aaronrenner
Copy link

Could this feature be added to doorkeeper? It would be really helpful.

@tsov
Copy link
Author

tsov commented Feb 8, 2014

Could we add this feature to doorkeeper? It is quite helpful to people that require 3rd party authentication.

@jbye
Copy link

jbye commented Apr 1, 2014

+1

@tute
Copy link
Contributor

tute commented Apr 14, 2014

@tsov could you please rebase on top of current master, and we work it out? Thanks for your work!

@tsov
Copy link
Author

tsov commented Apr 18, 2014

Will do now, thanks!

@pciacka
Copy link

pciacka commented Apr 22, 2014

Easter passed so now time to merge :) 👍

Conflicts:
	spec/dummy/db/schema.rb
	spec/support/helpers/url_helper.rb
@@ -16,7 +17,7 @@ def authorization_strategy(strategy)
end

def token_strategy(strategy)
get_strategy strategy, %w[password client_credentials authorization_code refresh_token]
get_strategy strategy, %w[password client_credentials authorization_code refresh_token assertion]

Choose a reason for hiding this comment

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

Line is too long. [103/80]

@alankehoe
Copy link

👍

@tsov
Copy link
Author

tsov commented Apr 27, 2014

@tute this should be good to go now, unless you have some more feedback. Let me know


def request
# TODO: For now OAuth::PasswordAccessTokenRequest is reused for the Assertion Flow. In need of
# OAuth::AssertionAccessTokenRequest in future
Copy link
Contributor

Choose a reason for hiding this comment

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

How exactly would an AssertionAccessTokenRequest be different than a PasswordAccessTokenRequest?

Copy link
Author

Choose a reason for hiding this comment

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

There shouldn't be any differences in the PasswordAccessTokenRequest and the potential AssertionAccessTokenRequest to be honest, but I think by using the PasswordAccessTokenRequest for the assertion flow, the name didn't suite as much anymore. What do you think? I could also just remove the TODO comment

t.string "token", :null => false
t.string "refresh_token"
t.integer "expires_in"
t.datetime "revoked_at"

Choose a reason for hiding this comment

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

Prefer single-quoted strings when you don't need string interpolation or special symbols.

@tsov
Copy link
Author

tsov commented May 3, 2014

Ok, sorry about the schema file, that's absolutely my fault! I saw you guys changed from double quotation marks to single, and that you use the new hash syntax. I will change it now!

@tsov
Copy link
Author

tsov commented May 3, 2014

Ok, lets see if this passes, then this PR should be good to go :)

@tute
Copy link
Contributor

tute commented May 3, 2014

This would be an extension: http://tools.ietf.org/html/rfc6749#section-4.5. I wouldn't like to add extensions into doorkeeper, but I'd love it to be extendable. We should be able to make extensions in separate projects, and add a wiki page to this project linking to them.

I don't think this code belongs to this repository, will leave open until we sort it out.

Thank you very much for your work.

@tute tute added the refactor label May 3, 2014
@tsov
Copy link
Author

tsov commented May 3, 2014

Ok, that seems reasonable! But I also think assertion should be part of doorkeeper, maybe if we followed the proper standard this could be added? draft-ietf-oauth-assertions-16
Let me know :)

@foFox
Copy link

foFox commented May 3, 2014

This seems ready for master 😊

@tute
Copy link
Contributor

tute commented May 3, 2014

I'm happy to work with you in a gem that extends doorkeeper with the new grant type. Could be hosted in https://github.com/doorkeeper-gem or in your own account, as you'd like.

@tsov
Copy link
Author

tsov commented May 3, 2014

That's sounds promising and like a nice solution for the future. I think it should be available in the doorkeeper-gem github organization for others to see too!

@tute
Copy link
Contributor

tute commented May 3, 2014

Cool, you have access to https://github.com/doorkeeper-gem/doorkeeper-assertion-grant. I'm not yet sure how the code/tests will look like, so let's sort it out! :)
Closing this PR for now, this code will be useful for the new repo though.
Thank you!

@tute tute closed this May 3, 2014
@tsov
Copy link
Author

tsov commented May 3, 2014

Thanks & sounds like a plan!
We'll work it out :)

@tute tute mentioned this pull request May 8, 2014
@tute
Copy link
Contributor

tute commented May 23, 2014

Hey all, today I worked on a draft to extend doorkeeper with this assertion grant. Tests are passing in https://github.com/doorkeeper-gem/doorkeeper-grants_assertion. Need to polish up some code and write documentation, but tests are green.

@tsov could you please test it out, addding the gem to your Gemfile and calling the resource by assertion? Thank you!

@tsov
Copy link
Author

tsov commented May 23, 2014

@tute sweet!
Just tested it and I can confirm it works.
I created a pull request to add guide step to remind people to add the additional grant flow manually.

@MarkMurphy
Copy link

I dont understand how this would differentiate between several third-party integrations? eg. What if I wanted to allow Facebook and Twitter access tokens?

@tsov
Copy link
Author

tsov commented Sep 22, 2014

Well it is up to you to implement the behaviour. Maybe with an extra parameter (e.g. assertion_type=facebook). Remember you have access to the params variable within the resource_owner_from_assertion block.

@MarkMurphy
Copy link

@tsov It would be nice to be able to do:

?grant_type=facebook

or

?grant_type=twitter

@tsov
Copy link
Author

tsov commented Sep 22, 2014

That would not conform to the OAuth 2.0 specs, as grant_type is a reserved parameter.
Again, this part is up to the developer/implementor.

e.g.

Doorkeeper.configure do
    resource_owner_from_assertion do
        CustomAuthenticator.retrieve_user params[:assertion_type], params[:assertion]
    end
end

where params[:assertion_type] could be facebook, twitter, google or whatever you want to support.
Also, remember that the assertion feature is not part of doorkeeper and only accessible through this gem https://github.com/doorkeeper-gem/doorkeeper-grants_assertion

Hope this helps

@christopherhein
Copy link

@MarkMurphy good examples of this would be from Google's OAuth provider – https://developers.google.com/accounts/docs/OAuth2ServiceAccount where they allow you to create an encrypted JWT that requires special params to be passed in much like any of the other OAuth flows see: https://developers.google.com/accounts/docs/OAuth2ServiceAccount#formingclaimset

@MarkMurphy
Copy link

@tsov @christopherhein Thanks guys, makes sense.

@gianlucadonato
Copy link

With doorkeeper-grants_assertion gem (v0.0.1) and Doorkeeper v1.4.0 I get this error:

NoMethodError in Doorkeeper::TokensController#create
undefined method `resource_owner_from_assertion' for #Doorkeeper::TokensController

Works instead with doorkeeper v1.3.1. Any chance to fix this in the new release?

@finalight
Copy link

hi, i also have the same issue with using the latest version of doorkeeper

any solutions for this issue??

@StarWar
Copy link

StarWar commented Jul 26, 2015

How to issue token for a particular app in assertion flow?

Currently I'm using facebook token to issue access_token and it works without passing client_id and client_secret.
I want to issue token only if correct client_id and client_secret is provided.

@adityamajeti
Copy link

I'm using gem 'doorkeeper-grants_assertion', git: 'https://github.com/Badiapp/doorkeeper-grants_assertion' which has dependency of doorkeeper >= 4.0.0 if i'm using doorkeeper >= 4.0.0 i'm unable to generate access_token if i use doorkeeper >= 4.0.0.rc1 i can generate access_token but unable to call assertion method in doorkeeper.rb

could any one suggest me which version of doorkeeper is best and i'm using mongodb as my db

@dagumak
Copy link

dagumak commented Sep 16, 2018

@tute Sorry to bring up an old topic, but is there a more official way to deal hook in OmniAuth so we can authenticate with Facebook, Twitter, or Google?

@nbulaj
Copy link
Member

nbulaj commented Sep 16, 2018

@dagumak
Copy link

dagumak commented Sep 16, 2018

@nbulaj Nothing wrong with it, but I do have questions. In this example, the authentication just passes through and does not alter the database (ex, creation of a new user, etc...): https://github.com/doorkeeper-gem/doorkeeper-grants_assertion/wiki

If I were previously using Devise and password grant and if I wanted to add Google login then how would user record creation and everything else work? My initial assumption would be to check if the user exists by google_id then email on authentication before creating a user record.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet