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 API scope "loved" that can only be requested by a designated client #9584

Closed
wants to merge 4 commits into from

Conversation

cl8n
Copy link
Member

@cl8n cl8n commented Dec 3, 2022

for #9451, splitting this part off.

this is my proposal for authorizing a single known client to have special permissions over the API. similar to the concept of _lio endpoints but using existing API setup bc it's easier to implement on both ends & document

the actual requests will come in later PR, but basically this will be used by https://loved.sh to associate Loved forum polls to their respective beatmaps/gamemodes/authors/etc. -- it will be done automatically after opening the polls each round

if this is merged as-is, LOVED_OAUTH_CLIENT_ID should be set to 7 on production.

@grumd
Copy link

grumd commented Dec 6, 2022

I wonder if osu-pps.com can also get some custom endpoints, because right now I do quite a lot of requests every day to recalculate all the data it needs. If it was an SQL query, that would be way faster I reckon.

@peppy
Copy link
Member

peppy commented Dec 6, 2022

I wonder if osu-pps.com can also get some custom endpoints, because right now I do quite a lot of requests every day to recalculate all the data it needs. If it was an SQL query, that would be way faster I reckon.

The only way that is going to happen is if graphql is implemented (see #7351).

@grumd
Copy link

grumd commented Dec 6, 2022

Makes total sense, I'll make sure to follow that issue, thanks

@notbakaneko notbakaneko self-requested a review January 17, 2023 11:16
@cl8n cl8n force-pushed the loved-poll-api-scope branch from 0335516 to d443b4e Compare May 20, 2023 06:05
Copy link
Collaborator

@notbakaneko notbakaneko left a comment

Choose a reason for hiding this comment

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

What's the issue with using the authorization code grant for this?

@peppy
Copy link
Member

peppy commented Aug 22, 2023

@cl8n you're probably going to have to give an answer to #9584 (review).

Bumping for visibility.

@peppy peppy added the type:api label Aug 22, 2023
@notbakaneko
Copy link
Collaborator

Additionally, we don't really want to be adding more special clients if possible, especially if we don't own both ends.

@cl8n
Copy link
Member Author

cl8n commented Aug 29, 2023

What's the issue with using the authorization code grant for this?

no issue in the sense that I can make it work like that. but in the end it would just be myself authorizing once and then forever refreshing that one token for any automated process performed like this, so I thought client credentials was a more fitting scheme

Additionally, we don't really want to be adding more special clients if possible, especially if we don't own both ends.

the endpoints I plan to add for this are not useful for, and should not be used by, anything except the one program they're designed to be an interface for. so that is why I chose to write in a specific client ID here

I'm open to other suggestions for how to authenticate for these endpoints, but I don't really have any better ideas myself.

@peppy
Copy link
Member

peppy commented Aug 30, 2023

I'm not sure on the correct oauth way to structure this, but here's a few pointers:

  • I think this is best suited to client credentials (which is already implemented) right? It's a single server component authorising, not users. So I'm confused about the suggestion to use authorisation grant
  • I think we will want elevated API privileges for some user groups. If we don't want to be adding more oauth scopes, then is the better way forward to add group checks local to API endpoint implementations? ie. all forum poll endpoints would require use of client credentials, and the calling user to be in a specific phpbb_group.

@nanaya
Copy link
Collaborator

nanaya commented Aug 30, 2023

Scope should be for limiting what the token can do from the client perspective, not for server side authorisation. Additionally, * here stops being actually * makes things confusing.

The permission should be checked on client level instead. Similar to user group but for oauth client.

As for client credential acting as a user, I don't think that should be a thing (although it already kind of is with delegate scope). There's authorization code for that. Creating the initial access and refresh token is a bit involved but not impossible and only need to be done once as long it keeps refreshed regularly. Or alternatively a way to create a personal access token which is just authorization code without extra steps.

As for token with access to users' group, a new scope which gives authenticated user full group access would be appropriate.

@cl8n
Copy link
Member Author

cl8n commented Sep 14, 2023

Scope should be for limiting what the token can do from the client perspective, not for server side authorisation.

fair enough, but to be clear I got this type of idea from the existing chat scope 🤔
I'll leave it as a normal scope then, and the endpoints will be responsible for authorisation.

As for client credential acting as a user, I don't think that should be a thing

well me neither but point I was trying to make is I don't think this really can be attributed to one user, it's just implementing part of how Loved itself works, with the help of a third-party component. I think the client itself is what's being authorised and not me. my osu! account is not particularly relevant to anything that this will be doing.

anyway --- with the conclusion being to not have special behavior for requesting the scope itself, the majority of this PR will not be used anymore so I'm closing it. my first PR in this series will instead be for the main endpoint required, where I hope the context will help decide how the authorisation should be performed

@cl8n cl8n closed this Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants