feat: support generic oauth2 #2211
Conversation
decentral1se
force-pushed
the
generic-oauth-support
branch
from
4951c12
to
0e5548e
Compare
|
Oh nice @decentral1se, this is an incredibly useful feature. I understand you are still building it out so here some comments after using this feature:
|
|
+1 I would appreciate implementation of a generic Oauth. Would fit perfectly with my reverse proxy and Authelia |
|
Thanks for feedback! More most welcome. I am maintaining a temporary fork with this patch over in https://git.coopcloud.tech/coop-cloud-chaos-patchs/calibre-web and have published the |
|
Sorry guys. |
|
@OzzieIsaacs can't we just remove Google/Github and use this instead? So, instead of 3 login routines, you have 1. The code in this PR supports anything that speaks OpenID Connect. I really don't want to maintain a fork... if you can show me how your migrations work, I could probably even migrate existing clients into this new implementation, so users would not be affected on an upgrade. |
|
Sorry guys, I stick to my opinion. |
|
I really don't understand your mentality on this. Many people have asked for this and now we even have a PR for it. It feels like you're being stubborn just for the sake of being stubborn. People could work up wiki examples and contribute to make this easy for people to understand. I get that you do not want additional work supporting this. I get that very clearly. I wouldnt want additional work for you. I do think you need to be open however that you're not everyone. If someone has actually taken the time to write a PR that just makes the current implementation universal then why do you immediately have he say no? There are enough people who want this that your simply saying no without really looking at logically is really demeaning to your users. I feel like a child on this topic. "Daddy, look what I drew..can I please hang it on the wall?" "No, I like my walls the way they are" without even considering the wants or needs of the child. Can you please take another look and be open minded about it? If you put up a poll then you'd get peoples opinion on this. |
|
@OzzieIsaacs I strongly agree with @RXWatcher, it would be great if you would reconsider your decision. Your point that not many people use it is fair, but with only Google and GitHub the choice is fairly limited which also decreases the userbase, having this universal method allows all OAuth providers to work, even self hosted ones, so more people can and will use OAuth. The most work has been done with this commit, you mostly just have to merge it and done. That you don't want to deal with supporting users failing to implement generic OAuth is also understandable, but from my experience looking into other projects that implemented generic OAuth (even similar projects like Komga, a selfhosted Web Manga Library), such issues or support requests are almost nonexistent or very fast resolved by the community. This is probably because most if not all people selfhosting a service, especially when selfhosting an OAuth Provider, have enough experience to figure out potential OAuth configuration issues by themselves, as there is a lot of documentation from Google and GitHub, so I don't really think support would be much of an issue. For the effort needed to keep this up to date, I'm not 100% sure, but from what I know this implementation would not be needed to be updated as OAuth has been pretty consistent in the past, but even if updates would be needed sometime in the future, I'm sure this would be handled by members of the community by pull requests. In the end, I think merging this pull request will increase the user count having OAuth because of more flexibility and will allow me and many more to implement their own OAuth Flows (which will make us happy :) ), while only increasing the support amount for this feature by a minimal amount (if even any). It would be very great if you would reconsider your decision to implement this feature. |
|
And to re-iterate, as I people are still discussing the possibility of adding an additional provider, that is 100% not what I'm proposing here. I am proposing to remove 2 providers & extend functionality with a generic implementation. This was just a draft PR. I could re-work this PR to delete more code than it adds. It would arguably reduce maintenance burden. If you look at the existing code, its written in such a way so as to be extendable with a lot of providers. But you don't even want that. We could remove a lot of code so that it isn't doing this programmatic loading based on a list of providers & just use one. You could even avoid a tricky migration script and just document how to switch over provider login details. It'd be a few clicks for anyone using existing login providers. |
|
@decentral1se I think he's already moved on. He's not listening to a word you say on it. It's really sad that something that would reduce code and make it generic so multiple providers could be covered is completely ignored by the main dev because his mind is already made up when he saw the title. I doubt he even looked at what your PR did. |
|
Sorry to bump a closed issue, but it is really unfortunate that in order to work with University SSO for the Computer Science department where I studied, I have to maintain a fork of calibre-web because the two default providers have absolutely no |
|
Id even donate like 100 euros directly for this if he'd actually integrate it. |
|
If anyone wants to co-maintain the SSO fork & keep up with upstream, I'd gladly join forces. Hit Me Up. |
|
The current implementation is way to complex for users, I'm fearing a lot of questions regarding what to fill out here and there and so on. |
|
Looks like we're back in the game folks 🙃 To summarise what you wrote @OzzieIsaacs so as to confirm I understand clearly:
Is that correct? Also one question: can we re-use your existing database implementation of the oauth provider instead of the file based approach? I can't see the benefit of the file based approach & avoiding it would be less work. Then the form simply iterates through databse objects instead of files. And my response in general: you're asking for a lot of new work. You are also coming from a position of rejecting the work to accepting the work without a clear statement. So, before I take the risk of starting this, can you please confirm that you are going to follow through with accepting this work if it matches your expectations? I agree to work to your expectations but you must merge the PR then. I don't want to waste my time. |
Yes, it makes no sense to have a generic implementation and an additional implementation for 2 simple "instances of the generic one
The file based logic shall be some sort of config file for additional providers, to avoid having a lot of config options visible in the UI I expect less support requests if there is no "visible" extension possibility in the UI. This is still my main concern having a lot of people asking for support for oauth providers you never heared of.
The images don't have to be uploadable. Load them from a folder on the server, name could be given in the config file mentioned above. Or the name has to be the name of the oauth provider.
The data from the config file can be copied to the database and read during normal operation from there. Only one config file would be needed not one file for each provider.
My main concern is not the maintenance of the code, it's the support by people asked. By using a config file my hope is that people would describe working config options in the wiki and I'm feeling not forced to help people setup their oauth workflow if it's not accessible via the UI. Having an UI with the current options from this PR will in my opinion generate a lot of questions.
|
|
@OzzieIsaacs This feature would be hugely useful to me and I would be happy to work with @decentral1se to build documentation for the feature if that tips the scales at all. |
|
@decentral1se, I've pulled the latest upstream (07c67b0) and applied your patch (0e5548e) on top of it. At this point, the application works (as visible here: https://books.[link removed because of copyright violation]/login?next=%2Fme (note: not my instance, nor my fork, but uses Calibre-web with generic OAuth via Keycloak)). My environment uses Authentik rather than Keycloak, and this initial implementation has some hardcoded values specific to Keycloak (such as the relative userinfo path). So Ace-Archer and I are working to build out an implementation of this feature that works. If you're open to it, we would really appreciate a chance to chat with you about making this work, as we're diving in blind and your insight would likely alleviate some frustration in the early stages. |
|
Hey @Jafner I didn't get back around to this but it is on my TODO stack... I will be travelling for a while now but if you want to reach me, drop a mail via |
|
@Jafner and myself have been chatting via mail, we're kicking off a matrix chat to organise around this work and coordinate. Please join in if you want to chat! It's an open room for all calibre-web contributors, however you contribute, with code, design, discussion, etc. Link: |
|
You just made my month! Thank you for being open to this.
…On Sun, May 22, 2022, 04:20 Ozzie Isaacs ***@***.***> wrote:
The current implementation is way to complex for users, I'm fearing a lot
of questions regarding what to fill out here and there and so on.
What we could do:
In the code we still support github and google (with the generic
procedure). Additionally create a file where the users can add additional
oauth providers with all the needed config options (name, callback routine
and so on..), just leaving the variable ones open to the user (private and
public key). I think one in all line would be good. Maybe csv format?, or a
yaml format (at least something users can handle and easily read) In the UI
the name of the oauth provider is showing up and if you select it you can
fill in the additional config options (public and private key). Furthermore
please find a solution for the logo showing up in the old implementation on
the login screen. (Some place where the users can place the files and they
are getting read by calibre-web and displayed on the login screen.
Please create the pull request to the development branch (should be more
or less even with the main branch)
Please also generate a entry in the wiki to describe what to enter in the
file, where to find it. Users than could also extent the entry in the wiki
with their working configs.
With this complicated procedure I'm no longer feeling forced to support
the users with their config options. And the users with very very limited
knowledge hopefully never try to extend it and won't generate support work.
Last thing: I request some testing routines (mostly focused on wrong file
format, non writeable files and so on: (
https://github.com/OzzieIsaacs/calibre-web-test/tree/development/test)
—
Reply to this email directly, view it on GitHub
<#2211 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADLOXCOSEIXT3JNAAXTHFKLVLHU4NANCNFSM5JTUYP5A>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
Still would like to pick this up one day but budget became an issue in the related project and it is stalled for now. I'll close this off for now and maybe I'll come back to it! If anyone wants to pick up this work, please do. |
@Jafner any chance you have that code somewhere else, getting a 404 on that gitlab instance, and I’m interested in contributing. |
|
Migrated over to Gitea. |
|
Just wanted to add my words of support behind adding generic oauth/oidc (and the ability to use other foss apps like Authentik, Authelia and Keycloak). Hopefully it comes to fruition one day. Calibre web is a lovely piece of selfhosted software, truly. But I selfhost my apps to avoid using platforms like Google, and I'm not going to get my Nan to sign-up for a Github account 😄. Additionally, security with selfhosted software is paramount to me, and Calibre web is one of the few services I have to expose externally, which makes this issue more pressing. Briefly looking into similar software I see that Kavita also doesn't (yet) provide oauth/oidc. But if the importance of this issue is still in question, please have a look where it ranks on their feature requests page:
|
|
To reiterate, adding generic oauth2 support isn't a new feature. The auth library used for GitHub and Google auth also supports OIDC. Oauth2/OIDC could actually be consolidated to one config page that supports GitHub/Google/generic without changing or adding auth libraries |
brought over the pr [2211](janeczku/calibre-web#2211) from the parent project to add generic oauth implementation
Hi following #2199 I've been looking into the feasability of meeting a compromise between 1. not wanting to add additional login methods and 2. supporting a broader set of oauth2 providers.
My solution for this has been to implement a "generic" oauth2 provider which can support anything that supports oauth2. This would mean that you could potentially deprecate Github/Google methods and use this Generic method as the one oauth2 implementation. All sorts of oauth2 providers can then be supported without further coding work.
I don't think my implementation is very good but it is a start and it does indeed work. I'm testing it now. I don't know how to support migrations so this breaks when running with existing installations.
Curious if this can be merged in some fashion or if there is space for more discussion?