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

Added oauth2_settings.RESOURCE_SERVER_INTROSPECTION_RESPONSE_FIELD setting to let po… #1347

Closed

Conversation

fijemax
Copy link

@fijemax fijemax commented Oct 21, 2023

Fixes #1325

Description of the Change

Checklist

  • PR only contains one change (considered splitting up PR)
  • unit-test added
  • documentation updated
  • CHANGELOG.md updated (only for user relevant changes)
  • author name in AUTHORS

…sibility to change field used

Signed-off-by: Fillau <jm.fillau@gmail.com>
@fijemax fijemax force-pushed the feat/add-resource-server-subject-field branch from 39ed5e0 to d2d57cf Compare October 21, 2023 12:37
@fijemax fijemax changed the title Added oauth2_settings.RESOURCE_SERVER_SUBJECT_FIELD setting to let po… Added oauth2_settings.RESOURCE_SERVER_INTROSPECTION_RESPONSE_FIELD setting to let po… Oct 21, 2023
@codecov
Copy link

codecov bot commented Oct 21, 2023

Codecov Report

Merging #1347 (bef53ce) into master (4c13679) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1347   +/-   ##
=======================================
  Coverage   97.54%   97.54%           
=======================================
  Files          32       32           
  Lines        2120     2120           
=======================================
  Hits         2068     2068           
  Misses         52       52           
Files Coverage Δ
oauth2_provider/oauth2_validators.py 94.09% <100.00%> (ø)
oauth2_provider/settings.py 100.00% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@dopry
Copy link
Contributor

dopry commented Oct 21, 2023

@fijemax, I think I see your need here. The user is being fetched or created here. The current implementation uses username as the key field. Username may not be set as it's an optional in the response and frankly we don't know the required fields on someones usermodel so this approach is a bit naieve.

I think this approach might work for your use case, but at the moment I don't think I have enough context to be confident this is a good fix that will work for the wider community. I'm gonna do some digging around this method so I better understand how it's used. Can you please provide more detail on your use case in your original issue to provide more context?

@n2ygk, do you have thoughts?

@n2ygk
Copy link
Member

n2ygk commented Oct 23, 2023

@dopry @fijemax I have not been following this issue. Isn't getting the sub or other added claims described here? Or here for userinfo.

@n2ygk
Copy link
Member

n2ygk commented May 7, 2024

Isn't this accomplished by #1325?

@fijemax
Copy link
Author

fijemax commented May 8, 2024

Hello,
What do you mean ?

@n2ygk
Copy link
Member

n2ygk commented May 13, 2024

Hello, What do you mean ?

@fijemax See #1325 which allows setting the username field value. This looks to me to address the same issue you are addressing here.

@n2ygk
Copy link
Member

n2ygk commented May 13, 2024

@fijemax please edit the PR description. I've initialized it with the PR template.

@n2ygk
Copy link
Member

n2ygk commented May 20, 2024

I believe this feature is resolved by #1325

@n2ygk n2ygk closed this May 20, 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

Successfully merging this pull request may close these issues.

Add the possibility to map user info with user model field.
3 participants