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

#1204: Improve token introspection configuration #1262

Merged
merged 1 commit into from May 23, 2019

Conversation

nbulaj
Copy link
Member

@nbulaj nbulaj commented May 23, 2019

Improve token introspection configuration (provide access to authorized client and authorized token)

@nbulaj nbulaj merged commit 21d0cfd into master May 23, 2019
@nbulaj nbulaj deleted the allow_to_check_authorized_token_in_introspection branch May 23, 2019 10:28
@linhdangduy
Copy link
Contributor

linhdangduy commented May 24, 2019

@nbulaj
Sorry, it is merged pull requeset but here is my thought about current token introspection configuration:

  1. NoMethodError for nil class:
    I see that authorized_client and authorized_token parameters could be nil depend on the request send to introspection endpoint.
    Suggestion: Doorkeeper should remind users when using these instance at this configuration (quite confuse to user!?)

    • check nil before using these two params
    • Or using authorized_client, authorized_token with &, for examples:
      authorized_client&.protected_resource? and authorized_token&.some_method?
      If not using &, NoMethodError - undefined method for nil:NilClass exception can occur.
  2. Introspection Response:
    You may think with this configuration, user could config to check scopes of authorized_token params. So if authorized_token doesn't have sufficient scopes (does not contain sufficient privileges), the response will be 200 status and body is active: false.
    But in this case, here in rfc7662 states that it returns 401 status code.

If the protected resource uses an OAuth 2.0 bearer token to authorize its call to the introspection endpoint and the token used for authorization does not contain sufficient privileges or is otherwise invalid for this request, the authorization server responds with an HTTP 401 code...

@nbulaj
Copy link
Member Author

nbulaj commented May 24, 2019

That's are great points, @linhdangduy ! Would you like to propose a PR to fix them? I can do it myself if not.

  1. I'm for Or using authorized_client, authorized_token with &

  2. Introspection Response: yep, we need to return 401 in case user authorized with token and allow_token_introspection isn't passed.

@linhdangduy
Copy link
Contributor

linhdangduy commented May 24, 2019

@nbulaj I've learned a lot while reading doorkeeper and your feedback at my pull request.
I would like to create a pull request. I'm thinking more about (2)

@linhdangduy
Copy link
Contributor

linhdangduy commented May 24, 2019

@nbulaj
Following is my proposing for token introspection endpoint, looking forward to hearing from you.

Proposing

The request can be authorized by using two method client credentials or token:

  • client credentials: (solved)

    def active?
    if authorized_client
    valid_token? && token_introspection_allowed?(authorized_client.application)
    else

    after authorizing credentials, 👆the config will be run and if it's fail, response is 200 status and {active: false}

  • token: at authorizing step: (proposing)

    elsif authorized_token
    # Requested bearer token authorization
    #
    # If the protected resource uses an OAuth 2.0 bearer token to authorize
    # its call to the introspection endpoint and the token used for
    # authorization does not contain sufficient privileges or is otherwise
    # invalid for this request, the authorization server responds with an
    # HTTP 401 code as described in Section 3 of OAuth 2.0 Bearer Token
    # Usage [RFC6750].
    #
    @error = :invalid_token if authorized_token_matches_introspected? || !authorized_token.accessible?

    after checking authorized_token_matches_introspected? and accessible?, the config should be run at this step. If it's fail, request is unauthorized, response will be 401.

As upon describing, response is different with different authorization method, I think we could separating two config for these two method: UPD below 👇

# for `token`
allow_token_introspection_by_authorized_token do |token, authorized_token|
end
# for `client credentials`
allow_token_introspection_by_client do |token, authorized_client|
end

Pros:

  • Right responding (as RFC7662), and providing two authorization method.
  • Don't need & anymore, the arguments pass to configs always present.
  • Transparent to user, configuration will be simply to understand and setup.

Cons:

  • Two configurations.

Some thought about current Code:

else
valid_token? && token_introspection_allowed?(authorized_token&.application)
end

I'm not for the authorized_token&.application as the client send to config block. client param is should only used for authorized_client, not application of authorized_token.
Because at here, the request is authorized by authorized_token method, so if using only one configuration, authorized_token method will accidentally read the config of client_credential method (for example: token.application == authorized_client)

UPD1 Oh, sorry to make thing complicated, I think we can just use only one configuration. (need )
UPD2 I will create a pull request!

@linhdangduy
Copy link
Contributor

linhdangduy commented Jun 10, 2019

@nbulaj
After some update of allow_token_introspection configuration, my team really want to use this configuration to enable introspection function correctly.
when does doorkeeper's team plan to release v5.2.0? Hope it could be release soon.
Thank you.

@nbulaj
Copy link
Member Author

nbulaj commented Jun 10, 2019

Hi @linhdangduy . There are no concrete release date, we are watching for opened / critical issues / PRs and make a decision on top of that data. Very approximately - mid of June if we wouldn't catch any big issues.

@nbulaj
Copy link
Member Author

nbulaj commented Jun 17, 2019

Hi @linhdangduy . You can try to use doorkeeper 5.2.0rc2 from rubygems and report any issues you faced 🎉

@linhdangduy
Copy link
Contributor

@nbulaj Sure, I will try it. Thank you very much ❤️

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.

None yet

2 participants