-
Notifications
You must be signed in to change notification settings - Fork 16
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
Update scopes used in OAuth #382
Conversation
- 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.
Codecov Report
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
|
There was a problem hiding this 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.
bigbone/src/integrationTest/kotlin/social/bigbone/v300/V300StatusMethodsIntegrationTest.kt
Outdated
Show resolved
Hide resolved
bigbone/src/test/kotlin/social/bigbone/api/method/AppMethodsTest.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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 scopes
query 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
…token match those in OAuth URL.
- 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)
scopes query parameter is correct, @bocops added a comment to code explaining what’s happening
There was a problem hiding this 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. 🚀
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] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@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? |
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? |
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
Breaking Changes
Scope.NAME
enum has been replaced with hierarchically arranged scope definitions in the class. Example:Scope.Name.READ
becomesScope.READ.ALL
FOLLOW
scope has been removed. Use individual child scopes ofREAD
andWRITE
instead.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
gradle check
and there were no errors reported