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

Add read:me OAuth 2.0 scope, allowing more limited access to user data #29087

Merged
merged 1 commit into from Apr 23, 2024

Conversation

ThisIsMissEm
Copy link
Contributor

@ThisIsMissEm ThisIsMissEm commented Feb 4, 2024

This allows applications to request a much more limited scope of information about the current user than that which the scopes of read or read:accounts would give.

NOTE: We don't currently allow GET /api/v1/accounts/:id where :id is the current users' id, since this would require asserting permissions in the AccountsController#show method.

My thinking here is to land this in 4.3, and then in 4.4, we can make that the default OAuth 2.0 scope, so by default applications only get access to GET /api/v1/accounts/verify_credentials nothing more.

@ThisIsMissEm
Copy link
Contributor Author

This pull request is complementary to #27142 which removed needing scopes to verify the OAuth 2.0 Application's credentials

@ThisIsMissEm ThisIsMissEm changed the title Add read:me OAuth 2.0 scope, allowing only reading the current users Add read:me OAuth 2.0 scope, allowing more limited access to user data Feb 4, 2024
Copy link

codecov bot commented Feb 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.01%. Comparing base (86500e3) to head (10ee849).
Report is 594 commits behind head on main.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #29087      +/-   ##
==========================================
- Coverage   85.01%   85.01%   -0.01%     
==========================================
  Files        1059     1060       +1     
  Lines       28277    28285       +8     
  Branches     4538     4537       -1     
==========================================
+ Hits        24040    24046       +6     
- Misses       3074     3075       +1     
- Partials     1163     1164       +1     

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

@ThisIsMissEm
Copy link
Contributor Author

I totally just realised I forgot to add the localisation strings for this new scope. Maybe we should have some specs that test "can I do the oauth flow using this scope" ?

@ClearlyClaire
Copy link
Contributor

I totally just realised I forgot to add the localisation strings for this new scope. Maybe we should have some specs that test "can I do the oauth flow using this scope" ?

Missing localization strings do not actually prevent from performing the OAuth flow, so if the goal is to prevent missing translations, we want something different. Possibly integrated in i18n-tasks.

@ThisIsMissEm
Copy link
Contributor Author

For this scope to be useful / used, we'll need to implement #24099 such that clients can discover what scopes are available to them to use (falling back to "minimal version supported scopes" if that document can't be requested)

@renchap renchap added this to the 4.3.0 milestone Mar 25, 2024
… CredentialAccount

This allows applications to request much more limited scope to the current user than that which `read` or `read:accounts` gives.
Copy link
Contributor Author

@ThisIsMissEm ThisIsMissEm left a comment

Choose a reason for hiding this comment

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

Have rebased, and left two comments relating to either expanding endpoints for read:me beyond verify account credentials, and adding tests for when that scope should NOT apply.

@ClearlyClaire ClearlyClaire added this pull request to the merge queue Apr 23, 2024
Merged via the queue into mastodon:main with commit 049b159 Apr 23, 2024
29 checks passed
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

4 participants