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

Get required OpenID Role #1488

Closed

Conversation

siriusfreak
Copy link
Contributor

@siriusfreak siriusfreak commented Jan 4, 2024

Summary

This change allows to perform role-based authorization for OpenID providers.

The new parameter OPENID_REQUIRED_ROLE specifies the allowed role name.

Change Type

New feature (non-breaking change which adds functionality)

Testing

Tested by the personal installation of Keycloak. The configuration process is described in the OpenID with Keycloak and Role Restriction section.

Test Configuration:

Checklist

  • My code adheres to this project's style guidelines
  • I have performed a self-review of my own code
  • I have commented in any complex areas of my code
  • I have made pertinent documentation changes
  • My changes do not introduce new warnings
  • I have written tests demonstrating that my changes are effective or that my feature works (No test for this module in the current codebase)
  • Local unit tests pass with my changes (No test for this module itself (No test for this module in the current codebase)
  • Any changes dependent on mine have been merged and published in downstream modules (Not applicable)

@danny-avila
Copy link
Owner

Hi @siriusfreak hope you're well! do you still plan on contributing this?

Improve process and change libs
@siriusfreak siriusfreak marked this pull request as ready for review January 25, 2024 15:08
@siriusfreak
Copy link
Contributor Author

@danny-avila pls check it

@childotg
Copy link

Dear @siriusfreak , for who is looking for a groups claims integration, do you think you can add that support too? It is very common to use also security groups in addition of application roles IdPs. What do you think about?

@siriusfreak
Copy link
Contributor Author

I am relay on KeyCloack. Could you show me your identity provider?

Basically, we need only check if there are required items in the list inside JWT token. So it could be more generalized like a way to check the token.

@childotg
Copy link

Sure it is Entra (former Azure AD).
It HAS app roles but it is very common (not just here with Azure) to have roles check against the groups claim.
So I was wondering if you want to extend this behavior with an addition or just extending the behavior of the two new env variables you introduced.
In case, I can dig into it in the next days
Thanks!

@siriusfreak
Copy link
Contributor Author

siriusfreak commented Jan 26, 2024

Hi. @childotg @danny-avila

I added more flexibility to the configuration. Now, it is possible to determine the exact source for the token inside ID provider response. I tested it with Entra and Keycloak.

@childotg You can use emitting groups as roles to retrieve roles in id_token. Entra return group UUIDs in roles parameter of id token

Example configs.

Keycloack:

OPENID_REQUIRED_ROLE=librechat
OPENID_REQUIRED_ROLE_TOKEN_KIND=access
OPENID_REQUIRED_ROLE_PARAMETER_PATH = "realm_access.roles"

Entra:

OPENID_REQUIRED_ROLE=librechat
OPENID_REQUIRED_ROLE_TOKEN_KIND=id
OPENID_REQUIRED_ROLE_PARAMETER_PATH = "roles"

image

@@ -30,6 +30,8 @@ services:
image: mongo
restart: always
user: "${UID}:${GID}"
ports:
Copy link
Owner

Choose a reason for hiding this comment

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

These ports cannot be exposed by default. if this is needed, it has to be done with the override compose file: https://docs.librechat.ai/install/configuration/docker_override.html

@@ -38,6 +40,8 @@ services:
image: getmeili/meilisearch:v1.5
restart: always
user: "${UID}:${GID}"
ports:
Copy link
Owner

Choose a reason for hiding this comment

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

same as above

@danny-avila
Copy link
Owner

thanks for the documentation. it would also be nice to have a guide on how to setup roles via azure as you showed in one the comments here but more comprehensive.

@siriusfreak
Copy link
Contributor Author

@danny-avila

  • docker-compose port exposing removed
  • docs updated
  • auth docs for auth providers moved to the separate files

@@ -57,7 +60,13 @@ async function setupOpenId() {
let user = await User.findOne({ openidId: userinfo.sub });

if (!user) {
user = await User.findOne({ email: userinfo.email });
if (!userinfo.email) {
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the recent update. This caught my attention, there is some nesting here that is confusing. In general I'm against nesting but it would make more sense to have this condition as a separate block above the user not being found at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked deeper and remove this print, because it most for more logs, not for important logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Owner

Choose a reason for hiding this comment

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

sorry for the delay! thank you will test this today!

@danny-avila danny-avila mentioned this pull request Apr 2, 2024
8 tasks
@danny-avila
Copy link
Owner

Merging this PR's commits along with some edits, mostly documentation, in #2279

@danny-avila danny-avila closed this Apr 2, 2024
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.

3 participants