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

MAJOR ISSUE - matching_token_for not considering custom attributes #1665

Closed
PhilippeChab opened this issue Aug 1, 2023 · 1 comment
Closed

Comments

@PhilippeChab
Copy link

Description

This version of the gem introduced a new functionality allowing us to specify custom attributes to store along the usual data in oauth tables. This is a great change that is most appreciated.

Unfortunately, it seems like some cases are not handled correctly, resulting in crashes - here, for instance. An other major issue that has been introduced is matching_token_for not considering custom attributes. This is a big problem, since a client can possibly receive a token for the wrong tenant_id, resulting in data from different tenant_id being mixed on the client side. It is particularly troublesome in find_or_create_for when using the config reuse_access_token.

Steps to reproduce

Using the authorization flow:

  • Add a custom attribute to doorkeeper config, say tenant_id
  • Toggle reuse_access_token config
  • Generate a token for both tenant_id
  • Generate an access code with tenant_id A
  • Retrieve an access token with the previously generated access code

Expected behavior

The access token returned should be for the same tenant_id

Actual behavior

An access token can be returned for an other tenant_id

cc @JeremyC-za @nbulaj

@ashkulz
Copy link

ashkulz commented Aug 3, 2023

@PhilippeChab I think we need to introduce another configuration option, because you cannot use all the custom attributes directly.

In our case, we can allow access to multiple tenants so it's actually tenant_ids on our side. We'll need to use that configuration option to filter inside matching_token_for

@JeremyC-za I guess we haven't hit yet but is still important to handle. Not sure if we want to handle it in #1660 or a separate PR though 🤔

@nbulaj nbulaj closed this as completed in 58f68b4 Apr 3, 2024
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

No branches or pull requests

2 participants