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

Set default OAuth scope to READ #119

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andregasser
Copy link
Owner

@andregasser andregasser commented Jan 20, 2023

I have set the default Oauth scope to "read" where it was used. It would be nice, if we could test this somehow. Any ideas?

@andregasser andregasser linked an issue Jan 20, 2023 that may be closed by this pull request
@andregasser andregasser marked this pull request as draft January 20, 2023 17:37
@codecov
Copy link

codecov bot commented Jan 20, 2023

Codecov Report

Merging #119 (ec0993d) into master (95aad6b) will increase coverage by 0.05%.
The diff coverage is 62.50%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #119      +/-   ##
============================================
+ Coverage     44.97%   45.02%   +0.05%     
  Complexity      174      174              
============================================
  Files            67       67              
  Lines          1452     1457       +5     
  Branches         83       83              
============================================
+ Hits            653      656       +3     
- Misses          777      779       +2     
  Partials         22       22              
Impacted Files Coverage Δ
.../src/main/kotlin/social/bigbone/rx/RxAppMethods.kt 0.00% <0.00%> (ø)
...ain/kotlin/social/bigbone/api/method/AppMethods.kt 82.35% <0.00%> (ø)
...n/kotlin/social/bigbone/api/method/OAuthMethods.kt 96.15% <83.33%> (-3.85%) ⬇️

@bocops
Copy link
Collaborator

bocops commented Jan 23, 2023

I'm not sure we really can unit test this. We can't even check (outside of tests) whether the matching scope is granted before performing a request, because scopes are not returned by any of the OAuth endpoints. We could store whatever scope value our client is built with and then use that before attempting any requests - but then we would need to duplicate the whole logic of what individual scope is sufficient for what operation. That's probably not worth it?

This might be worth an integration test, though, after #100 is done: Set up two clients with different scopes, then perform the same operation with both and expect one to fail and one to go through.

@bocops
Copy link
Collaborator

bocops commented Jan 23, 2023

Regarding the actual change - Mastodon documentation reads as if Scope.READ is implied if no scope is defined in the request. If that's the case, we could alternatively not define any specific default values ourselves, and in fact make the parameter nullable (with a null default?). Not sure if that would be better than what we currently do, just mentioning the option. :)

@andregasser
Copy link
Owner Author

Yes, making the parameter nullable with a default value makes sense to me :-)

@bocops
Copy link
Collaborator

bocops commented Dec 7, 2023

@andregasser are you still going to merge this? If so, I will wait before attempting #143 (which should go in before 2.0.0). If not, I can do this in combination with #143.

@andregasser
Copy link
Owner Author

@bocops What do you mean exactly? I am not working on this atm 😊 Feel free to continue with your work. 👍🏻

@bocops
Copy link
Collaborator

bocops commented Dec 7, 2023

#116 is currently assigned to you, and this draft PR is linked to it. If you unassign youself from 116, I can do it in combination with some other issues.

@andregasser
Copy link
Owner Author

Hello @bocops Ah, got it. Sure, I have reassigned #116 to you. Thank you 👍

@bocops bocops mentioned this pull request Dec 15, 2023
5 tasks
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.

Align scope defaults with Mastodon API
2 participants