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

Stop auto-creation of AccessGrant when a matching token is found if custom access token attributes are used #1660

Merged

Conversation

JeremyC-za
Copy link
Contributor

Summary

There are two changes in this PR, discussed in more detail below:

  1. Prevent new access grants being created automatically when a matching token is found if custom_access_token_attributes are used.
  2. Added another config option revoke_previous_authorization_code_token to mirror revoke_previous_client_credentials_token.

Change 1:

What happens currently is that when you authorize an application, and you already have a valid access token (not expired or revoked) for that application, then a new access grant is created immediately. This happens in AuthorizationsController#render_success, which checks for matching_token? and then calls authorize_response, which creates the new grant.

This is a problem if custom_access_token_attributes are used, since those attributes from the existing token aren't copied into the new grant. In my case, I need to display the custom attributes (tenant ids) from the existing token so I can allow the user to edit them if necessary before the new access grant is created. I did this by loading the custom attributes from the existing token into the @pre_auth object, which is the accessible in the view, and can be used as needed.

I also needed the existing token to be revoked once a token is generated from the new access grant, which leads to the next change.

Change 2:

We already have a config option revoke_previous_client_credentials_token. That's only available for the client_credentials grant type though. I need the same behaviour for the authorization_code grant type, so I've added another config option for it.

There is a TODO to make revoke_previous_client_credentials_token more generic for the other flows, but I don't really understand the other ones and I just need this grant type, so I opted add a new config option rather than refactor the existing one.

@JeremyC-za
Copy link
Contributor Author

@nbulaj if you could review when you have time please 🙏

@JeremyC-za
Copy link
Contributor Author

@nbulaj I've done some refactoring. Let me know what you think.

@JeremyC-za
Copy link
Contributor Author

@PhilippeChab sorry for the slow response - I've had a look at the changes you're suggesting and the issue you logged and I can try to make some changes. I just can't promise when I'll be able to get to it though, my hands are a bit full at the moment.

Copy link
Contributor Author

@JeremyC-za JeremyC-za left a comment

Choose a reason for hiding this comment

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

@PhilippeChab I've pushed some changes which I think should handle the scenario you mentioned. Let me know what you think.

cc @nbulaj

@@ -65,6 +67,13 @@ def form_post_response?
response_mode == "form_post"
end

def load_custom_attributes_from_token(matching_token)

Choose a reason for hiding this comment

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

Why do we override the custom attributes sent in the request by the ones in the token?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my response here.

Also, I think it would be incorrect usage to send that custom attributes in the params here (like you mentioned). This only gets called from AuthorizationsController#new, and since I've changed can_authorize_response? to always return false if custom attributes are used, this will always either render errors, or :new. In other words, if we did find a matching token (regardless of whether we matched the custom attributes or not), this would still only render :new and never do anything with that token - so there isn't any risk of a token being auto-created with another token's custom attributes.

Choose a reason for hiding this comment

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

I think the link is not pointing to the correct reference, but as I said in the other thread, I think this is something that you should handle on your end and not in doorkeeper directly.

lib/doorkeeper/models/access_token_mixin.rb Outdated Show resolved Hide resolved
app/controllers/doorkeeper/authorizations_controller.rb Outdated Show resolved Hide resolved
lib/doorkeeper/models/access_token_mixin.rb Outdated Show resolved Hide resolved
lib/doorkeeper/models/access_token_mixin.rb Outdated Show resolved Hide resolved
@nbulaj
Copy link
Member

nbulaj commented Oct 12, 2023

Hey @JeremyC-za . What is the state of this PR? Do we need to do more work on it?

@JeremyC-za
Copy link
Contributor Author

Hey @nbulaj, it's almost ready. There are a couple things I want to look into (which may not end up needing code changes), I just haven't been able to find the time lately. I intend on getting to it by the end of next week though.

Copy link
Contributor Author

@JeremyC-za JeremyC-za left a comment

Choose a reason for hiding this comment

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

@PhilippeChab I've responded to your comments, let me know what you think.

@nbulaj In my opinion this is ready to be reviewed and hopefully merged. Thanks.

app/controllers/doorkeeper/authorizations_controller.rb Outdated Show resolved Hide resolved
@@ -65,6 +67,13 @@ def form_post_response?
response_mode == "form_post"
end

def load_custom_attributes_from_token(matching_token)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See my response here.

Also, I think it would be incorrect usage to send that custom attributes in the params here (like you mentioned). This only gets called from AuthorizationsController#new, and since I've changed can_authorize_response? to always return false if custom attributes are used, this will always either render errors, or :new. In other words, if we did find a matching token (regardless of whether we matched the custom attributes or not), this would still only render :new and never do anything with that token - so there isn't any risk of a token being auto-created with another token's custom attributes.

@ashkulz
Copy link

ashkulz commented Nov 8, 2023

@nbulaj would it be possible for you to review this?

@JeremyC-za
Copy link
Contributor Author

@nbulaj @PhilippeChab Apologies for the radio silence. I've pushed a change so the custom attributes are no longer copied into the access grant automatically when a matching token is found (as per this conversation). I think that was all that was todo, but let me know if I've missed something.

A recap of what this PR includes at this point in time:

  • AccessTokenMixin#find_matching_token now considers custom attributes, which should resolve MAJOR ISSUE - matching_token_for not considering custom attributes #1665.
  • When using custom_access_token_attributes, an AccessGrant and code will no longer be automatically created when a matching token is found (due to the changes added in AuthorizationsController#render_success).
  • Introduced revoke_previous_client_credentials_token config option (unchanged from Change 2 in PR description).

Copy link

@PhilippeChab PhilippeChab left a comment

Choose a reason for hiding this comment

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

I think this is good to go once CI is fixed for 7.1

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.

LGTM 👍

We can ignore truffle-ruby-head for Rails 7.1 I think. Have to make it experimental BTW

@nbulaj
Copy link
Member

nbulaj commented Mar 29, 2024

@JeremyC-za can I ask you please to squash the commits and add a changelog entry? 🙏 Thanks!

fixes doorkeeper-gem#1665), and introduces `revoke_previous_client_credentials_token` config option.
@JeremyC-za JeremyC-za force-pushed the custom_attributes_multitenancy_patch branch from 42d9489 to 58f68b4 Compare April 2, 2024 14:58
@JeremyC-za
Copy link
Contributor Author

@nbulaj done 👍🏻

@nbulaj nbulaj merged commit 50c9300 into doorkeeper-gem:main Apr 3, 2024
20 of 22 checks passed
@nbulaj
Copy link
Member

nbulaj commented Apr 3, 2024

Thanks a lot @JeremyC-za @PhilippeChab

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.

4 participants