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

Update scopes used in OAuth #382

Merged
merged 11 commits into from
Dec 15, 2023

Conversation

bocops
Copy link
Collaborator

@bocops bocops commented Dec 8, 2023

Description

  • makes all scope parameters in OAuth and App methods optional. Endpoints not receiving a scope parameter will default to granting "read" permission.

  • removes default parameter value from Scope constructor. Users will need to define a certain scope if they want to use it. Closes Align scope defaults with Mastodon API #116.

  • adds all scopes as defined by Mastodon, marks existing scopes FOLLOW and ALL as deprecated.

  • removes all internal usage of the now deprecated ALL scope. Closes Update OAuth scopes #143.

  • moves deduplication of scopes from a separate function into the one building the parameter string. This avoids unnecessarily throwing an IllegalArgumentException. Closes Change scope validation to avoid throwing IllegalArgumentException #381.

Type of Change

  • New feature

Breaking Changes

  • Scope constructor can no longer be called without explicitly enumerating scopes that should be requested.
  • Scope.NAME enum has been replaced with hierarchically arranged scope definitions in the class. Example: Scope.Name.READ becomes Scope.READ.ALL
  • Existing FOLLOW scope has been removed. Use individual child scopes of READ and WRITE instead.
  • Existing ALL scope has been removed. Use individual scopes necessary for your use case instead.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Mandatory Checklist

  • I ran gradle check and there were no errors reported
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or that my feature works
  • All tests pass locally with my changes
  • I have added KDoc documentation to all public methods

- makes all scope parameters in OAuth and App methods optional. Endpoints not receiving a scope parameter will default to granting "read" permission.
- removes default parameter value from Scope constructor. Users will need to define a certain scope if they want to use it.
Closes andregasser#116.

- adds all scopes as defined by Mastodon, marks existing scopes FOLLOW and ALL as deprecated.
- removes all internal usage of the now deprecated ALL scope.
Closes andregasser#143.

- moves deduplication of scopes from a separate function into the one building the parameter string. This avoids unnecessarily throwing an IllegalArgumentException.
Closes andregasser#381.
Copy link

codecov bot commented Dec 8, 2023

Codecov Report

Merging #382 (7f47c1c) into master (f14e7fd) will increase coverage by 0.25%.
The diff coverage is 69.49%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #382      +/-   ##
============================================
+ Coverage     48.08%   48.33%   +0.25%     
+ Complexity      542      541       -1     
============================================
  Files           140      140              
  Lines          3864     3906      +42     
  Branches        256      259       +3     
============================================
+ Hits           1858     1888      +30     
- Misses         1816     1825       +9     
- Partials        190      193       +3     
Files Coverage Δ
.../src/main/kotlin/social/bigbone/rx/RxAppMethods.kt 0.00% <0.00%> (ø)
...n/kotlin/social/bigbone/api/method/OAuthMethods.kt 94.66% <50.00%> (+0.22%) ⬆️
...ain/kotlin/social/bigbone/api/method/AppMethods.kt 73.68% <25.00%> (-5.27%) ⬇️
...rc/main/kotlin/social/bigbone/rx/RxOAuthMethods.kt 0.00% <0.00%> (ø)
...igbone/src/main/kotlin/social/bigbone/api/Scope.kt 82.60% <82.60%> (-17.40%) ⬇️

@PattaFeuFeu PattaFeuFeu added enhancement New feature or request breaking Incompatible with previous versions labels Dec 8, 2023
Copy link
Collaborator

@PattaFeuFeu PattaFeuFeu left a comment

Choose a reason for hiding this comment

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

I like what you’ve done to the place! 🧑‍🎨

I’ve tested it with a new application I created via the library and all seems to work fine.

Copy link
Owner

@andregasser andregasser left a comment

Choose a reason for hiding this comment

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

Overall, this is looking very good. Thanks! Just found something regarding the scopesquery string parameter that should be checked quickly.

- in integration tests, declare "full scope" once
- in unit tests, do not specify any Scope unless directly tested
- remove deprecated "follow" scope from the access token asset
- replace Name enum with hierarchical structure for a more intuitive access to individual scopes
- changing samples and USAGE.md to match

Examples:
- Scope(Scope.Name.READ) becomes Scope(Scope.READ.ALL)
- Scope(Scope.Name.ADMIN_READ_ACCOUNTS) becomes Scope(Scope.ADMIN.READ.ACCOUNTS)
@PattaFeuFeu PattaFeuFeu dismissed andregasser’s stale review December 15, 2023 00:13

scopes query parameter is correct, @bocops added a comment to code explaining what’s happening

Copy link
Collaborator

@PattaFeuFeu PattaFeuFeu left a comment

Choose a reason for hiding this comment

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

I like it! Great changes. 😊

I have added an optional suggestion in the comments to improve Scope definition conciseness a bit. I’d also be fine with merging this state though. 🚀

bocops and others added 5 commits December 15, 2023 09:54
Bumps com.gradle.enterprise from 3.15.1 to 3.16.

---
updated-dependencies:
- dependency-name: com.gradle.enterprise
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@PattaFeuFeu PattaFeuFeu merged commit 3bc604d into andregasser:master Dec 15, 2023
4 checks passed
@PattaFeuFeu
Copy link
Collaborator

@bocops Merged! Thanks for the efforts 🚀

Could you please make sure that other PRs that may no longer be necessary after these changes are closed? You’ve already linked the affected issues, so those should be fine I assume?

@bocops bocops deleted the 116_143_381_enhance_scope branch December 15, 2023 12:13
@bocops
Copy link
Collaborator Author

bocops commented Dec 15, 2023

Looks good regarding issues, thanks!

@andregasser the only remaining PR in this area is your #119 - I think that is no longer necessary. Can you close it as time permits?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Incompatible with previous versions enhancement New feature or request
Projects
None yet
3 participants