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

Add support for dynamic scopes #1739

Merged
merged 3 commits into from
Oct 30, 2024

Conversation

stanhu
Copy link
Contributor

@stanhu stanhu commented Oct 9, 2024

This commit adds support for dynamic scopes, which are disabled by default.

As discussed in keycloak/keycloak#8486, a dynamic scope notation is in the form:

<static-part>:<variable-part>

The objective of this feature is to have a static part of the scope that represents an entity and a variable part that identifies the entity.

For example, a scope of user:1 could be interpreted as allowing access to perform actions of user 1. A wildcard (*) is allowed in the variable part, such as user:*. This scope allows the request to perform actions as any users.

Dynamic scopes can be enabled via:

Doorkeeper.configure do
  enable_dynamic_scopes
end

A custom delimiter can also be configured:

Doorkeeper.configure do
  enable_dynamic_scopes(delimiter: '-')
end

Relates to #431

@stanhu stanhu force-pushed the sh-support-dynamic-scopes branch 3 times, most recently from 3287cfb to af40ef9 Compare October 9, 2024 21:26
@stanhu
Copy link
Contributor Author

stanhu commented Oct 9, 2024

@nbulaj What do you think of this pull request? Also I'm not sure how best to fix the Code Climate errors. I tried to disable the method-count check for just this file, but it seems that Code Climate can't do that. https://docs.codeclimate.com/docs/excluding-files-and-folders says:

Exclusions can only be made at the global level (excluding code from all analysis) or at the plugin-level (excluding code only from specific third-party plugins). Currently, exclusions cannot be made for individual maintainability checks.

@grzesiek
Copy link

+1

@samg
Copy link

samg commented Oct 22, 2024

We're long time users of Doorkeeper and having the ability to use dynamic scopes in Doorkeeper would be very helpful to the projects we have planned. 😄 Are there any plans to merge this and include it in the gem?

@nbulaj
Copy link
Member

nbulaj commented Oct 23, 2024

hey @stanhu 👋 Sounds interesting, but I wasn't able to find some RFC. So it looks like a custom addition to the spec. Do we have any docs to reference? Except keycloack 😄

@stanhu
Copy link
Contributor Author

stanhu commented Oct 23, 2024

@nbulaj I don't think there is a spec governing how scopes are handled; I think that is an application-specific decision. I will keep looking.

@stanhu
Copy link
Contributor Author

stanhu commented Oct 23, 2024

As https://datatracker.ietf.org/doc/html/rfc6749#page-23 mentions:

The strings are defined by the authorization server.

RabbitMQ, for example, supports wildcard OAuth2 scopes: https://www.rabbitmq.com/docs/oauth2#scope-translation

@nbulaj
Copy link
Member

nbulaj commented Oct 24, 2024

Anyway I like it 👍

@nbulaj
Copy link
Member

nbulaj commented Oct 24, 2024

I see tests failing but seems to be related to something else 🤔 Looks like something change in Rails

@stanhu
Copy link
Contributor Author

stanhu commented Oct 24, 2024

The tests are failing due to https://github.com/doorkeeper-gem/doorkeeper//commit/b4bd6803147934d41d0d19642ef8e97d48984090. Rails is showing the exception because ActionDispatch::Request.show_exception? is returning true since older Rails versions expect that false is used: https://github.com/rails/rails/blob/v7.0.8.4/actionpack/lib/action_dispatch/http/request.rb#L186

rails/rails#45867 deprecated false in favor of :all, :none, and :rescuable for Rails 7.1.

@stanhu
Copy link
Contributor Author

stanhu commented Oct 24, 2024

#1742 fixes the broken Rails tests.

@nbulaj
Copy link
Member

nbulaj commented Oct 25, 2024

Can we rebase now @stanhu ? Above one is merged now 🙇

This commit adds support for dynamic scopes, which are disabled by
default.

As discussed in keycloak/keycloak#8486,
a dynamic scope notation is in the form:

<static-part>:<variable-part>

The objective of this feature is to have a static part of the scope
that represents an entity and a variable part that identifies the
entity.

For example, a scope of `user:1` could be interpreted as allowing
access to perform actions of user 1. A wildcard (`*`) is allowed in
the variable part, such as `user:*`. This scope allows the request
to perform actions as any users.

Dynamic scopes can be enabled via:

```ruby
Doorkeeper.configure do
  enable_dynamic_scopes
end
```

A custom delimiter can also be configured:

```ruby
Doorkeeper.configure do
  enable_dynamic_scopes(delimiter: '-')
end
```

Relates to doorkeeper-gem#431
@stanhu
Copy link
Contributor Author

stanhu commented Oct 25, 2024

@nbulaj Done, thanks!

Code Climate is flagging this file because it has more than
20 methods.
@stanhu
Copy link
Contributor Author

stanhu commented Oct 28, 2024

@nbulaj Friendly ping: could you take a look at this pull request?

Copy link
Member

@nbulaj nbulaj left a comment

Choose a reason for hiding this comment

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

Just checked, awesome work, LGTM 👍

@nbulaj
Copy link
Member

nbulaj commented Oct 29, 2024

Can we please add a changelog entry? 🙏

@stanhu
Copy link
Contributor Author

stanhu commented Oct 29, 2024

@nbulaj Done!

@nbulaj nbulaj merged commit 5057044 into doorkeeper-gem:main Oct 30, 2024
22 checks passed
@stanhu
Copy link
Contributor Author

stanhu commented Oct 30, 2024

@nbulaj Thank you for the merge! Would you mind tagging a new release?

@nbulaj
Copy link
Member

nbulaj commented Oct 30, 2024

Will review rest of the opened MRs tomorrow and yes, will prepare a release. Maybe more can be included as well

@ThisIsMissEm
Copy link
Contributor

I wonder if we could support nested scopes in a similar way? e.g., write:account is covered by the write:account and write scopes — we currently have a sizeable amount of code in mastodon to support this.

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.

5 participants