Add Support for Per-App Scope Validation

[KazWolfe]

First off, my apologies if this project isn't accepting feature requests or this is a weird-ish one. I'm still actively learning Ruby so I may be going about this all wrong.

I have an application which permits other users to create OAuth applications, where different applications may have different scopes approved for their use (stored in the scopes column of the applications table). In order to facilitate this, I borrowed the existing logic from DeviceGrantMixin for system-level scope enforcement:

  validate :validate_scopes_approved

  def validate_scopes_approved
    scopes_valid = scopes.present? && Doorkeeper::OAuth::Helpers::ScopeChecker.valid?(
      scope_str: scopes.to_s,
      server_scopes: Doorkeeper.config.scopes,
      app_scopes: application.scopes,
      grant_type: Doorkeeper::DeviceAuthorizationGrant::OAuth::DEVICE_CODE
    )

    errors.add(:scopes, :invalid_scope) unless scopes_valid
  end

This, however, nets in a 500 exception when an application attempts to use an invalid scope. While this does block the action as intended, this does not behave in an expected manner (e.g. the nice error message that shows when client_id is missing). I've also been able to reproduce this 500 behavior when passing in a fully-invalid scope while enforce_configured_scopes is set.

I've attempted to trace this a bit further, and it appears as though this is caused by the fact that DeviceAuthorizationRequest is not actually aware of scope authorization. Unfortunately, it seems difficult (if even possible? I'm honestly not sure how) to monkeypatch the appropriate validation into that class. Seeing as Doorkeeper proper respects this for its Authorization Code flows, I'd like to see if it's possible to have this added as a feature to the gem. Alternatively, if there's a better way to handle this case (or a way to patch this without forking the library), I'm absolutely willing to implement that instead.

Thanks!

[marco-nicola]

Hi, no worries, in fact, issues and PRs are very welcome and this one doesn't look weird at all!

I first want to be sure to understand everything correctly. I'll start with simple and basic things, that you might already know, or have already tried; then I might need further feedback before going deeper.

Doorkeeper provides its own mechanism to handle scopes, briefly documented here: https://doorkeeper.gitbook.io/guides/ruby-on-rails/scopes

Let's suppose we configure scopes "foo", "bar" and "baz", and enable the enforcement of configured scopes.

When creating or modifying Doorkeeper::Application models, their scopes are validated against that configuration: this should be expected and correct.

Let's create a new Application, called "MyApp", with scopes "foo bar" (excluding "baz").

Now, if a client performs a Device Authorization Request, specifying other unknown/invalid scopes (e.g. "qux"), the response seems to be a 422. That's surely better than a 500, but a first point of discussion might be whether it should rather be a 401 (or even something else).

Here we get closer to your point: let's do another Device Authorization Request, with the client ID of "MyApp" and the scope "baz". That scope is globally valid, but it's not included in the scopes of "MyApp".

Currently, the access is granted. A new Doorkeeper::AccessToken record is created, and it contains the request's scope ("foo").

As far as I understand the problem, it seems desirable to detect the mismatch between the request's scope and the related Application's scope. Is that correct?

If so, I'm quite certain there is currently no place where such a match is performed (at least, surely not on this gem). That's not on purpose: simply me and my colleagues rarely find ourselves playing too much with scopes. But now that this scenario has been highlighted, it looks like an important feature is missing.

A first noninvasive possibility comes up to my mind: let the client be authorized at this step, and delegate the scope's match to the controllers for any subsequent API (or page) request. For example, that might be a before-action performed on the ApplicationController. This might be a quick temporary implementation, but surely not a very nice actual solution.

It seems more appropriate to reject Device Authorization Requests with non-matching scopes in the first place.

Before going further, I'd first like to check with you if my verbose examples make sense and really match your situation and needs.

Meanwhile, I assign myself some homework: here's a bunch of questions that might help us, and other readers, find the right fixes and solutions.

• Since scopes and Applications strictly belong to the "core" Doorkeeper, is there an obvious/recommended/canonical way to perform this strict match? Perhaps, already implemented for other grant mechanisms?
• Can we find a standardized behavior among RFCs? (They "invented" everything in the first place!)
• And, finally, is this gem actually lacking some features? (Since device authorization requests are specific to this gem, I think so!)

I'll be back with a clearer mind after some more digging in code and docs. I hope to hear from you soon!

[KazWolfe]

Thanks for taking a look at this! Hopefully the below will be helpful to your side, I've done a tiny bit of digging to try to patch this in my own app's code, eheh.

As far as I understand the problem, it seems desirable to detect the mismatch between the request's scope and the related Application's scope. Is that correct?

Precisely. Doorkeeper itself will perform this validation (spec here) on pre-authorization and refuses to even show the user the consent dialog if a scope is invalid.

As far as I can tell, the ScopeChecker will validate scopes in the following order:

• If no scopes are present, scopes are considered valid.
• If app scopes are present (for authorization_code, from client.scopes), check requested scopes against app scopes.
• If app scopes are NOT present, check against server scopes (for authorization_code, from Doorkeeper.configuration.scopes).
• Additionally check against grant_type

It seems more appropriate to reject Device Authorization Requests with non-matching scopes in the first place.

This makes sense, would align perfectly with my use case, and appears to be what Doorkeeper itself does.

Since scopes and Applications strictly belong to the "core" Doorkeeper, is there an obvious/recommended/canonical way to perform this strict match? Perhaps, already implemented for other grant mechanisms?

Indeed! In fact, Device Authorization Requests already calls Doorkeeper's own checker, so I think this would really just be a case of what seems to be the following change plus some other accounting for error messages and all:

      def scopes_match_configured
        unless Doorkeeper::OAuth::Helpers::ScopeChecker.valid?(
          scope_str: scopes.to_s,
          server_scopes: Doorkeeper.config.scopes,
          app_scopes: application.scopes
          grant_type: Doorkeeper::DeviceAuthorizationGrant::OAuth::DEVICE_CODE
        )
          errors.add(:scopes, :not_match_configured)
        end
      end

Looking at the code, it seems like DeviceGrantMixin copied from Doorkeeper's native ApplicationMixin, as the validation code seems pretty similar. This would make sense given the implementation here as well, as Doorkeeper Core will enforce an application's permitted scopes regardless of enforce_configured_scopes's setting while this gem is a bit more lenient. In Doorkeeper Core, enforce_configured_scopes only seems to take effect when an Application is sent off for validation. I'd assume in general that DeviceGrantMixin behaves like this gem's version of a PreAuthorization?

Can we find a standardized behavior among RFCs? (They "invented" everything in the first place!)

Ah, standards. As far as I can tell, the standards leave "is a scope valid" up to the application to decide. The best reference implementation I can give you is Doorkeeper Base, unfortunately!

That said, for actual error handling, RFC 8628 section 3.2 links back to RFC 6749 section 5.2, so the requesting device would get the same invalid_scope that base Doorkeeper already sends.

And, finally, is this gem actually lacking some features?

I'd agree that this is a missing feature, albeit a minor one and (as you pointed out) easy enough to work around. I can definitely understand how this didn't come up in your team's own use case, though. The idea of constraining scopes based on application (especially for user-made applications) seems to be relatively rare in the OAuth space. I really thank you guys for even taking a look at this request!