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

feat: Support for Customizable OpenID Profile Fields via Environment Variable #5449

Closed
wants to merge 10 commits into from

Conversation

rubentalstra
Copy link
Collaborator

@rubentalstra rubentalstra commented Jan 24, 2025

Summary

Closes #4362
#4354

Docs: LibreChat-AI/librechat.ai#205

This Pull Request introduces comprehensive OpenID Connect (OIDC) support. The key changes include:

  • Environment Configuration: Updated .env.example to include new environment variables required for OpenID integration.
  • User Schema Enhancements: Modified userSchema.js to accommodate additional fields for OpenID data, ensuring seamless user management.
  • Data Mappers Implementation: Added openidDataMapper.js with provider-specific data mappers for Microsoft, AWS Cognito and Keycloak to handle custom OpenID data effectively.
  • OpenID Strategy Configuration: Enhanced openidStrategy.js to utilize the new data mappers, enforce role-based access control, and manage user avatars based on OpenID data.
  • Dependency Updates: Updated package.json and package-lock.json to include necessary libraries for OpenID functionality.
  • Type Definitions: Updated TypeScript types in types.ts to support custom OpenID data mappings.

Change Type

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Testing

currently only the Microsoft provider tested and full working.

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
  • Local unit tests pass with my changes
  • A pull request for updating the documentation has been submitted.

@rubentalstra rubentalstra changed the title started with Support for Customizable OpenID Profile Fields via Envir… started with Support for Customizable OpenID Profile Fields via Environment Variable Jan 24, 2025
@rubentalstra rubentalstra changed the title started with Support for Customizable OpenID Profile Fields via Environment Variable 🌟 feat: started with Support for Customizable OpenID Profile Fields via Environment Variable Jan 24, 2025
@rubentalstra rubentalstra changed the title 🌟 feat: started with Support for Customizable OpenID Profile Fields via Environment Variable feat: started with Support for Customizable OpenID Profile Fields via Environment Variable Jan 25, 2025
@rubentalstra
Copy link
Collaborator Author

@danny-avila i read your comment on a different pull request regarding a generic method for determining custom data from different openID providers.

Is this going in the right direction? I would love to get some feedback to contribute to this amazing project.

Simplifies token decoding by removing redundant utility functions, integrating direct decoding with proper error handling. Enhances logging clarity and consistency, improves role assignment logic, and adds robust error handling during avatar updates. Streamlines user creation and update processes while maintaining feature parity.
Removed @octokit/rest, @okta/okta-auth-js, and google-auth-library. Integrated @keycloak/keycloak-admin-client and @aws-sdk/client-cognito-identity-provider to enhance authentication handling. This change aligns dependencies with the updated authentication strategy.
@rubentalstra rubentalstra changed the title feat: started with Support for Customizable OpenID Profile Fields via Environment Variable feat: Support for Customizable OpenID Profile Fields via Environment Variable Jan 26, 2025
@rubentalstra rubentalstra marked this pull request as ready for review January 26, 2025 18:58
Introduced a reusable MicrosoftGraphClient in the constructor and added a `setAccessToken` method for managing tokens. Simplified custom data mapping logic and improved validation and error handling for `customQuery` and token usage. Refactored `cleanOdataKeys` to `cleanData` for better clarity and efficiency.
The @keycloak/keycloak-admin-client package was removed as it is no longer required.
@danny-avila
Copy link
Owner

I'm hesitant to make drastic changes here since a lot of people depend on OpenID for Authentication. If you could introduce this with the least amount of changes, adhering to the original code as much as possible, that would be appreciated. The smallest of changes have caused headaches in this module before and I wish to avoid that.

Also I noticed you wrote "// Not Tested"

You shouldn't introduce code you aren't testing, rather focus on what does work and introduce it non-intrusively.

Lastly, any new methods and functions, I would recommend introducing in another module other than api/strategies/openidStrategy.js

@rubentalstra
Copy link
Collaborator Author

thank you for the feedback. I will for sure simplify it and make as less changes as possible. then I will only add the option first of microsoft graph api.

@rubentalstra
Copy link
Collaborator Author

closed because of new pull request: #5612

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.

2 participants