-
-
Notifications
You must be signed in to change notification settings - Fork 267
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
feat: add OAuth authentication #3114
Conversation
I thought this was going to be a way longer diff when I went to look at it. I'm going to have to take a look at the library and get a little more familiar, but thank you so much for starting on this! |
It looks like this doesn't run yet, so I'm going to mark it as a draft. Feel free to mark it as ready for review once you're ready :) |
It was just a missing |
Should be good for another CI run @mouse-reeve |
Any idea when it can be merged? Then we can move forward with the API implementation :) |
CI seems okay with it, and I'm still interested in working on this when it's merged. Anyone up for a review ? |
Ah I apologize for leaving you hanging on this! I would like to get feedback from other folks on @bookwyrm-social/code-review if yall feel like this is the right first step. |
I think it is currently not very clear what the scope of this PR is. Is it a first step in developing a client API? Can end users already use it after this is merged? If so, should we not integrate OAuth settings into the user preferences, or at least add a link to the management view of the OAuth application? Should there be a test that at least checks if OAuth is working? |
The goal is to add the Oauth library so we can build on that. After this I plan to:
But I'm waiting with the actual configuration because if people don't like this library addition it'll be a lot of reworking |
Ah yes, I understand. In that case, I think the library you chose seems like a good choice for adding OAuth authentication to BookWyrm. |
So, what can I do to make this mergable? |
Small prefix to my plans, because of the federated nature I guess I'll need to add something like https://docs.joinmastodon.org/methods/apps/ first so apps can register with the server. |
Thank you for your patience with this @SMillerDev -- I've been a little unsure on how to best move forward on this incrementally but I think this is the right first step. Much appreciated! |
Thanks @mouse-reeve ! Hoping to work on app registration soon |
#3361 is a first draft of that. |
This should allow API clients to authenticate using OAuth
Relates to #2292