-
-
Notifications
You must be signed in to change notification settings - Fork 479
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
TODO: Provider - Device code #640
base: master
Are you sure you want to change the base?
Conversation
This PR can not be included or merged if the issues are not fixed. I'm leery of a custom login flow that doesn't cite any RFC;s being added to the lib anyway, since it's a new attack vector. What RFC's does this implement? This seems like a custom flow and as a one-off pr for an unknown process flow with no RFC or standardization, I'm inclined to reject it anyway. Edit: I looked at the original link and I'm not sure this is a good fit for the lib until we support JWT's fully for clients. |
Of course but I'm still working on it locally in a project to test the APIs.
It implements this draft flow https://tools.ietf.org/html/draft-ietf-oauth-device-flow-13
This flow doesn't require JWT so not sure what the blocker is there. |
Its architectural. If you look at the token types and the generator it looks like some test shims exist for JWT that point out the problems in the WebApp client, but they are otherwise dead code as they are only referenced in tests. For example, grant types etc are currently assumed (and hard coded), so that architectural work will need to be done to support multiple, in order to support both this device stuff and the JWT. |
Okay, if there is going to be a architecture change then I'm happy to pause this PR and mark it dependent on that change. Is there an issue I can reference ? |
The change hasn't happened yet, its just something I noticed the other day so I'm thinking we should work to common goals. The grant_type for example is hard coded but for JTW you need a very different value because an assert needs to happen that the value was used. We need to be able to support a different type based on the type of token being used; if you want to do that work as part of your PR it would help with support for both. |
FYI we're at draft 15 now. |
@thedrow no worries, I will do so. |
Finalized as RFC8628 : https://tools.ietf.org/html/rfc8628 |
I'l restart this work with the final RFC :) |
@jcampbell05 Ping? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rebase and let us know if you got the bandwitdh to move this forward! great job so far!
@jcampbell05 Is that a no? |
No lets close it and open an issue - I've ran out of bandwidth at the moment but this can serve as a POC at least for anyone wanting to pick this back up If I have time in the future then can open a new PR |
I will push on your branch. I will take on this :) |
@auvipy Is this still being actively worked on? |
no, need to come back to this, are you interested? |
for consuming and providing OAuth 2.0 IETF13 Draft. | ||
""" | ||
|
||
from __future__ import absolute_import, unicode_literals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from __future__ import absolute_import, unicode_literals |
This module is an implementation of various logic needed | ||
for consuming and providing OAuth 2.0 Draft IETF 13. | ||
""" | ||
from __future__ import absolute_import, unicode_literals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from __future__ import absolute_import, unicode_literals |
@@ -0,0 +1,81 @@ | |||
# -*- coding: utf-8 -*- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# -*- coding: utf-8 -*- |
This module is an implementation of various logic needed | ||
for consuming and providing OAuth 2.0 Draft IETF 13. | ||
""" | ||
from __future__ import absolute_import, unicode_literals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from __future__ import absolute_import, unicode_literals |
@@ -0,0 +1,8 @@ | |||
# -*- coding: utf-8 -*- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# -*- coding: utf-8 -*- |
@@ -1,6 +1,6 @@ | |||
# -*- coding: utf-8 -*- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# -*- coding: utf-8 -*- |
@@ -1,32 +1,36 @@ | |||
# -*- coding: utf-8 -*- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# -*- coding: utf-8 -*- |
@@ -1,6 +1,6 @@ | |||
# -*- coding: utf-8 -*- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# -*- coding: utf-8 -*- |
@@ -0,0 +1,174 @@ | |||
# -*- coding: utf-8 -*- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# -*- coding: utf-8 -*- |
@@ -0,0 +1,174 @@ | |||
# -*- coding: utf-8 -*- | |||
from __future__ import absolute_import, unicode_literals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from __future__ import absolute_import, unicode_literals |
This PR focuses on the server implementation of #626 and leaves the client implementation for a future PR. Since its in a draft I have placed it in a draft folder to indicate this so those who need it can use it with the awareness it may change in the future and will be re-named when the specification is stable.
Device Code is a new authorization flow which allows a client to be authenticated by a user for a device with limited or no input.
I based this implementation on the authorization grant type since this flow shares some similarities but I utilized some concepts for the client grant type where this flow differs.
To understand how to implement the new classes and flow for a server here is an overflow of the flow and how a server would use them:
create_authorization_response
is called. A DeviceToken should be passed in with the expiry date configured by the server as well as a RequestValidator configured to return the verification url and polling interval.create_verification_response
which uses the validator to check that code exists.create_verification_response
which will this time ask the validator to update the authorized state.create_token_response
method. The validator's validate_token_request will validate the device token is authorized and should implement all of the error codes from the RFC (including rate limiting if needed).Once authorized this method will use the create_token method on the BearerToken to create a new Access Token, ask the validator to save it and then invalidate the old device code.