Skip to content

Fix refresh tokens with dynamic scopes - #1754

Merged
nbulaj merged 1 commit into
doorkeeper-gem:mainfrom
stanhu:sh-fix-refresh-token-with-dynamic-scopes
Dec 6, 2024
Merged

Fix refresh tokens with dynamic scopes#1754
nbulaj merged 1 commit into
doorkeeper-gem:mainfrom
stanhu:sh-fix-refresh-token-with-dynamic-scopes

Conversation

@stanhu

@stanhu stanhu commented Dec 4, 2024

Copy link
Copy Markdown
Contributor

Summary

Suppose your OAuth application supports the scopes public user:*. Previously a refresh token with the public user:1 scope returned a scope of public by default, unless user:1 were explicitly requested. This happened because Doorkeeper::Oauth::BaseRequest does a set intersection:

"public user:*" & "public user:1" = "public"

With dynamic scopes, this doesn't work because the wildcard user:* doesn't match user:1.

To fix this, introduce Scopes#allowed and deprecate the use of &.

@stanhu
stanhu force-pushed the sh-fix-refresh-token-with-dynamic-scopes branch from fa75b04 to 707b393 Compare December 4, 2024 22:18
@stanhu

stanhu commented Dec 4, 2024

Copy link
Copy Markdown
Contributor Author

@nbulaj Just one small fix we ran into while trying to use dynamic scopes. Could you review?

Comment thread lib/doorkeeper/oauth/scopes.rb
@stanhu
stanhu force-pushed the sh-fix-refresh-token-with-dynamic-scopes branch from 707b393 to 8be888e Compare December 5, 2024 13:22
Suppose your OAuth application supports the scopes `public user:*`.
Previously a refresh token with the `public user:1` scope returned a
scope of `public` by default, unless `user:1` were explicitly
requested. This happened because `Doorkeeper::Oauth::BaseRequest` does
a set intersection:

"public user:*" & "public user:1" = "public"

With dynamic scopes, this doesn't work because the wildcard `user:*`
doesn't match `user:1`.

To fix this, introduce `Scopes#allowed` and deprecate the use of `&`.
@stanhu
stanhu force-pushed the sh-fix-refresh-token-with-dynamic-scopes branch from 8be888e to e2b8dab Compare December 5, 2024 13:23

@nbulaj nbulaj left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks! 🙇

@nbulaj
nbulaj merged commit 36e41c1 into doorkeeper-gem:main Dec 6, 2024
@stanhu

stanhu commented Dec 6, 2024

Copy link
Copy Markdown
Contributor Author

@nbulaj Thanks so much again! Sorry to bother you again, but would you be so kind to tag a new release?

Thanks again for your work on this project.

@nbulaj

nbulaj commented Dec 9, 2024

Copy link
Copy Markdown
Member

Hey @stanhu , already released ☝️

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.

2 participants