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

Port Generic Oauth Support #132

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MattyIceee
Copy link

Fixes #90
I just brought over the pr 2211 from the parent project to add generic oauth implementation. I am having trouble testing this as there seems to be an issue with the build.sh. Some help validating would be appreciated.

brought over the pr [2211](janeczku/calibre-web#2211) from the parent project to add generic oauth implementation
@crocodilestick
Copy link
Owner

Hey @MattyIceee, thanks for this! I've read about this whole debacle before on the CW GitHub. Could you please do your best to summarise the situation for me? As far as I can tell the original PR for this was very popular with users and the current head maintainer of CW refused to merge it based on personal biases (or did he have a point in the end?)

My questions would be:

  • Has this still not been implemented into CW, and if so, do you know why?
  • Upon adding this, what changes would be apparent to the average user? Is it something that is off by default or would it change the login in procedure for all users?
  • If added, would the changes be compatible with everyone's existing configuration files, or would it alter then that would then render them incompatible with stock CW?

Additionally regarding build.sh, did you edit the script with your own variables as instructed in the comments before running it?

@MattyIceee
Copy link
Author

@crocodilestick

  1. Gonna be honest, I don't know much about why this wasn't pulled in to the original project. It sounds like the maintainers just don't want to worry about a generic oauth implementation and that's completely their choice.
  2. I haven't gotten this running nor was I the original author of the change but reading the code it appears to just be a third oauth provider (they already support Github and Google). That new provider would be opt in. So unless you configure it, I don't believe it would even be visible on the login page.
  3. I can't answer this 100% unless someone tries it. It's just an additive change so I'd imagine it probably wouldn't break the config seeing how the config is just parsed by looking for indexes. There wouldn't be any missing indexes from on older version. Just new entries for the new fields required if you want to do custom oidc for example.

Also this pr's diff is a little misleading, and I could've been better with my commit history (apologies, I'm just an overworked software engineer looking to host my books with his sso setup🥲).

Because of how this project works (via overlaying its changes onto the original project) I first had to pull the most current versions of the affected files. Then I re-applied the changes I saw in 2211 to them. I haven't done a bunch of research into how this was implemented and quite frankly I don't have the time or desire to. If the libraries used provide us with the functionality they appear to this should resolve a good chunk of custom oidc setups in an "opt in" way. Is it the cleanest change? idk. Is it the safest library,?idk. Are either of those things true for a majority of the oidc implementations we're all already hosting? probably not. As always be sure to have multiple layers of security and hardening setup in your deployments even if using sso.

build.sh issue appears to be the docker build unable to find one of the internal directories its creating. I'll review this later but it's probably breaking either do to platform issues (I'm building on windows, I'll try on my Mac later to see if that fixes it) or something env specific, that I'd expect more people to be running into.

Thanks again for the project, its pretty neat!

@MattyIceee
Copy link
Author

I got the image up and building but I'm bumping into an unrelated error (as in it happens even when I back out my changes).
After signing in with the default admin creds it looks like there's an issue with get_cwa_settings saying the list is empty and blowing up. Gonna try to build my changes off 2.1.2 as I'm not sure if the changes on the head are still being worked on.

@crocodilestick
Copy link
Owner

Build an image not with the current repo files as those are very much in active development for the next version, but using the files from the latest release (you can download an archive of them from the repo's Releases page)

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.

[Feature Request] Generic OIDC/OAuth2 support
2 participants