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.

Sign up for GitHub

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

Jump to bottom

add support for openid connect #206

Merged
merged 2 commits into from
Jul 29, 2023
Merged

add support for openid connect #206

merged 2 commits into from
Jul 29, 2023

Conversation

jabdoa2
Copy link
Contributor

@jabdoa2 jabdoa2 commented Jul 18, 2023 •
edited
Loading

This allow using Danswer in typical (non-google) enterprise environments.

  • Access Tokens can be very large. A token without claims is already 1100 bytes for me (larger than allowed in danswer by default). With roles I got a 12kB token. For that reason I changed the field to TEXT in the database.
  • Danswer used to swallow most errors when OIDC would fail. Nodejs forwards a request to the backend and swallows all errors. Even within the backend we catched all ValueErrors and only returned the last exception with the request. Added full stack trace logging to allow debugging issues with userinfo and other endpoints.
  • Allow changing name of the login provider on the login button.
  • Changed variables and URLs to generic OAUTH_XX (without google in the name) but kept compatibility with the existing google integration
  • Tested against Keycloak with OpenID Connect

Next steps:

  • Claim to role mappings
  • Auto login/SSO (Login button is just an extra click)

@vercel
Copy link

vercel bot commented Jul 18, 2023

@jabdoa2 is attempting to deploy a commit to the Danswer Team on Vercel.

A member of the Team first needs to authorize it.

@jabdoa2 jabdoa2 force-pushed the support_openid_connect branch from 874a107 to 7e42b0d Compare July 18, 2023 22:46
@Weves Weves self-assigned this Jul 28, 2023
@Weves
Copy link
Contributor

Weves commented Jul 29, 2023

Hey @jabdoa2, thanks a bunch for this change (and sorry about the slow turnaround). Overall looks great and I'm sure this will be useful for a bunch of people.

There seems to be a bit of a rebase issue. I was able to pull this branch in locally and rebase against main without too much hassle, so if you could do that as well and update this PR that would be great. Once that's done, happy to merge it in.

A couple quick things that I'm happy to take care of in a follow up (or if you want to address them at the same time as rebasing, that works for me as well):

  • We should update the env.prod.template to include the OAUTH_TYPE, since that is now mandatory if you want to use OAuth.
  • There are some mypy issues in main.py as a result of oauth_client potentially being None

@jabdoa2
Copy link
Contributor Author

jabdoa2 commented Jul 29, 2023

I can rebase it. Or you can do it. Whatever is easier. I think I merged in the wrong direction.

I agree on the compose template. Also for the Kubernetes templates. We probably need some documentation as well. So far I am toying around locally only. At some point I will move my stuff into Kubernetes and will probably make a ton of adjustments to the helm chart anyway.

jabdoa2 added 2 commits July 29, 2023 13:32
@jabdoa2 @Weves
add support for openid connect
3129d2d 
This allow using Danswer in typical (non-google) enterprise environments.

* Access Tokens can be very large. A token without claims is already 1100 bytes for me (larger than allowed in danswer by default). With roles I got a 12kB token. For that reason I changed the field to TEXT in the database.
* Danswer used to swallow most errors when OIDC would fail. Nodejs forwards a request to the backend and swallows all errors. Even within the backend we catched all ValueErrors and only returned the last exception with the request. Added full stack trace logging to allow debugging issues with userinfo and other endpoints.
* Allow changing name of the login provider on the login button.
* Changed variables and URLs to generic OAUTH_XX (without google in the name) but kept compatibility with the existing google integration
* Tested again Keycloak with OpenID Connect

Next steps:
* Claim to role mappings
* Auto login/SSO (Login button is just an extra click)
@jabdoa2 @Weves
add docker compose and fix merge conflict
5d78712
@Weves Weves force-pushed the support_openid_connect branch from 370fab6 to 5d78712 Compare July 29, 2023 20:55
@Weves
Copy link
Contributor

Weves commented Jul 29, 2023

Rebased!

Weves
Weves approved these changes Jul 29, 2023
View reviewed changes
Copy link
Contributor

@Weves Weves left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Based on another read-through, I think the # special case for google in main.py is actually not needed (unless I'm missing something), but will handle that + other comments in a follow up PR.

@vercel
Copy link

vercel bot commented Jul 29, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
internal-search ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 29, 2023 9:03pm

@Weves
Copy link
Contributor

Weves commented Jul 29, 2023

Thanks again for this change 🙇

@Weves Weves merged commit 6378011 into onyx-dot-app:main Jul 29, 2023
@jabdoa2
Copy link
Contributor Author

jabdoa2 commented Jul 29, 2023

Based on another read-through, I think the # special case for google in main.py is actually not needed (unless I'm missing something), but will handle that + other comments in a follow up PR.

That would be only needed to keep compatibility for existing installations. Otherwise the callback url would change. Not sure which stability guarantees danswer currently provides. There are certain projects which would require this kind of backwards compatibility for PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Weves Weves Weves approved these changes

Assignees

@Weves Weves

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jabdoa2 @Weves