calibre-web
calibre-web copied to clipboard
janeczku
feat: support generic oauth2
Hi following https://github.com/janeczku/calibre-web/issues/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?
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:
-
"Default Settings for New Users " (set in
/admin/viewconfig) are not applied to users logging in for the first time via the generic oauth -
In case admins only allow login via generic oauth, a toggle where calibre-web's native login is hidden (or a mapping of the login button directly to
/link/generic) would be handy -
In theory one could inherit different roles from the OAuth provider, for example whether someone is an administrator or has particular permissions. That could be useful to support.
-
Generic oauth login currently allows only a singular 'generic' login, but one could easily imagine more would be configured.
+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 thecoopcloud/calibre-web docker image via https://git.coopcloud.tech/coop-cloud-chaos-patchs/docker-calibre-web. So, you can already test this if you want! A group is already testing https://books.lumbung.space now and things are doing fine I think. Configs for the deployment in https://git.coopcloud.tech/coop-cloud/calibre-web also. Curious if more people find this feature useful.
Sorry guys. I stick to my opinion that I will not include any more oauth provider or any other login routine. I don't use this and it's heavy for me to support and this type of feature create a lot of problems as errors are hard to reproduce and I don't want to end up programming administration routines instead of programming a ebook online presenting app.
@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. From the past I know that only a very small minority is using OAuth. Compared to this many times people show up and want OAuth login for this and that provider. From reverse proxy use cases I know that many people with small "admin" knowledge try to set up complex things, which causes frequently trouble. I don't want to spend time to support this, spending hours to set up certain OAuth provider and figure out how to set it up with user who tell my only a little amount of what they did and how their installation looks like
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 First of, sorry for this long text, but it would be great if you would take the time to read this, as I tried to get all my points in it, as I'm a strong advocate for this feature. Thanks I'm advance :)
I strongly agree with @RXWatcher, it would be great if you would reconsider your decision. You have Google and GitHub as an OAuth option, so having one more should not be that big of a deal, especially as it's such an advantage for users/admins.
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. Thanks a lot
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 customizability. I really hope you reconsider your stance on this @OzzieIsaacs - I'd even offer to maintain that specific bit of code.
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.
What we (you) 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)
Looks like we're back in the game folks :upside_down_face:
To summarise what you wrote @OzzieIsaacs so as to confirm I understand clearly:
- i should aim to deprecate/remove the existing oauth implementation
- and replace it with the generic implementation to support oauth login
- add an additional file based logic to support adding additional providers
- add support for image upload for login buttons
- submit these changes to the dev branch
- write a wiki page which clearly explains all the oauth form fields
- write tests (edit: added this one)
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.
i should aim to deprecate/remove the existing oauth implementation
Yes, it makes no sense to have a generic implementation and an additional implementation for 2 simple "instances of the generic one
add an additional file based logic to support adding additional providers
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.
add support for image upload for login buttons
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.
Also one question: can we re-use your existing database implementation of the oauth provider instead of the file based approach?
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.
You are also coming from a position of rejecting the work to accepting the work without a clear statement
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.
I agree to work to your expectations but you must merge the PR then. I don't want to waste my time. Yes I will merge it, otherwise I would just not have responded, as I already closed the PR.
@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, @ace-archer and I have begun work on a fork for this here: https://gitlab.jafner.net/Jafner/calibre-web
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 d34ae261d3 at sinenomine dot email with questions, can try to answer. Otherwise, I'll try to get back on this shortly and we could combine our powers.
@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:
https://matrix.to/#/#calibre-web-contrib:autonomic.zone
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 https://github.com/janeczku/calibre-web/pull/2211#issuecomment-1133842946, 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.
@decentral1se, @ace-archer and I have begun work on a fork for this here: https://gitlab.jafner.net/Jafner/calibre-web
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.
@Jafner any chance you have that code somewhere else, getting a 404 on that gitlab instance, and I’m interested in contributing.
