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

Support multiple redirect_uris when creating OAuth 2.0 Applications #29192

Conversation

ThisIsMissEm
Copy link
Contributor

@ThisIsMissEm ThisIsMissEm commented Feb 13, 2024

This brings us more inline with the dynamic client registration specification, where you can create a single application with multiple redirect URIs, see the example request at: https://datatracker.ietf.org/doc/html/rfc7591#section-3.1

This works around some weirdness in doorkeeper, where it only has a redirect_uri field on an Application, but it does some weird "join multiple values with a \n for the database" thing.

Arguably, this could be considered a breaking change because it introduces some kinda weird behavior, but it is technically the "correct" behavior with regards to OAuth 2.0...

This is currently a draft as it requires test coverage to be written, and I have had the time to write said tests yet.

Copy link

codecov bot commented Feb 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.01%. Comparing base (86500e3) to head (c43497a).
Report is 665 commits behind head on main.

❗ Current head c43497a differs from pull request most recent head d8cc3f3. Consider uploading reports for the commit d8cc3f3 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #29192      +/-   ##
==========================================
- Coverage   85.01%   85.01%   -0.01%     
==========================================
  Files        1059     1060       +1     
  Lines       28277    28288      +11     
  Branches     4538     4537       -1     
==========================================
+ Hits        24040    24049       +9     
- Misses       3074     3075       +1     
- Partials     1163     1164       +1     

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

@ThisIsMissEm ThisIsMissEm marked this pull request as ready for review May 1, 2024 16:18
@ThisIsMissEm ThisIsMissEm marked this pull request as draft May 1, 2024 16:18
@ThisIsMissEm ThisIsMissEm force-pushed the feat/support-multiple-redirect_uris-for-apps branch from c43497a to 69d1781 Compare May 2, 2024 15:39
@ThisIsMissEm ThisIsMissEm marked this pull request as ready for review May 2, 2024 15:42
@ThisIsMissEm ThisIsMissEm changed the title WIP: Support multiple redirect_uris when creating OAuth 2.0 Applications Support multiple redirect_uris when creating OAuth 2.0 Applications May 2, 2024
@ThisIsMissEm ThisIsMissEm force-pushed the feat/support-multiple-redirect_uris-for-apps branch from 69d1781 to d8cc3f3 Compare May 2, 2024 15:45
@ThisIsMissEm
Copy link
Contributor Author

The following issue was discovered whilst working on the tests: #30152

@ThisIsMissEm ThisIsMissEm force-pushed the feat/support-multiple-redirect_uris-for-apps branch from 7de288b to e35590f Compare May 7, 2024 21:50
ClearlyClaire
ClearlyClaire previously approved these changes May 10, 2024
@ThisIsMissEm
Copy link
Contributor Author

Working on a few additional changes after noticing some gaps when authoring the documentation for this PR.

@ThisIsMissEm
Copy link
Contributor Author

ThisIsMissEm commented May 15, 2024

@ClearlyClaire okay, we're good — I've added:

  • exposing the redirect_uris property from /api/v1/apps/verify_credentials (we had started to expose client_id and scopes in Feature: Allow oauth application introspection without read scope #27142 )
  • added test case to ensure /api/v1/apps/verify_credentials never exposes client_secret (just in case the controller is changed in the future)
  • Moved the redirect_uris method from the serializer to the Doorkeeper::Application extension (I feel like this is cleaner)

ClearlyClaire
ClearlyClaire previously approved these changes May 16, 2024
Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

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

Looks good to me although I'm confused why this is going back on client_id being introspectable.

@ClearlyClaire ClearlyClaire added this pull request to the merge queue May 17, 2024
Merged via the queue into mastodon:main with commit 2da2a1d May 17, 2024
30 checks passed
@ThisIsMissEm ThisIsMissEm deleted the feat/support-multiple-redirect_uris-for-apps branch May 17, 2024 14:35
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.

None yet

3 participants