#1066: Revert #967 which incorrectly breaks API.

[n2ygk]

Fixes #1066

Description of the Change

PR #967 should never have been merged as it changes the signature of a published API.

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.67%. Comparing base (250120d) to head (827db80).
Report is 215 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1068      +/-   ##
==========================================
- Coverage   96.70%   96.67%   -0.03%     
==========================================
  Files          31       31              
  Lines        1791     1777      -14     
==========================================
- Hits         1732     1718      -14     
  Misses         59       59              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Natureshadow]

@n2ygk Thanks for looking into this!

By now, we have a compatibility layer downstream. If you are interested, isntead of removing the feature the PR introduced, I could try to make a PR adding this compatibility layer to django-oauth-toolkit itself.

On the other hand, I think the feature was implemented incompletely, because publishing all available claims in the discovery info might be a privacy or security risk for some applications. So I think the API should provide for a new validator method, e.g. get_discovery_claims, which can override the list of published claims.

Additionally, the validator implementation should be free to return plain data instead of a callable, like before (both should be supported).

I could make these additions on top of @AndreaGreco's PR.

What do you think?

[n2ygk]

@n2ygk Thanks for looking into this!

By now, we have a compatibility layer downstream. If you are interested, isntead of removing the feature the PR introduced, I could try to make a PR adding this compatibility layer to django-oauth-toolkit itself.

On the other hand, I think the feature was implemented incompletely, because publishing all available claims in the discovery info might be a privacy or security risk for some applications. So I think the API should provide for a new validator method, e.g. get_discovery_claims, which can override the list of published claims.

Additionally, the validator implementation should be free to return plain data instead of a callable, like before (both should be supported).

I could make these additions on top of @AndreaGreco's PR.

What do you think?

It was done incorrectly and the release has been out such a short time that we can just put it back the way it was for now. No need to add compat layer.

[n2ygk]

@Natureshadow Also, feel free to start a new issue/PR. I don't quite understand what you are getting at but would love to review it. I didn't want that conversation to be in this thread though;-)