[Bug]: Identified "username" from Azure AD (OpenID) doesn't match schema

larsrinn commented 1 year ago

Contact Details

lars.rinn@node.energy

What happened?

When some users are trying to login using Azure AD, they get a server error. I think found the underlying issue:

Here, the schema for the username field is defined to match a regex: https://github.com/danny-avila/LibreChat/blob/909b00c7521277e49e4cf7319cd237c27250bd28/api/models/User.js#L28
Here, the username is defined to be the supplied username, the given name or empty: https://github.com/danny-avila/LibreChat/blob/909b00c7521277e49e4cf7319cd237c27250bd28/api/strategies/openidStrategy.js#L78
For us, we don't get a username claim back, so it takes the given_name. If this name doesn't match the regex, the server fails. (This would also happen for the fallback option of an empty string if given_name wasn't there either)

This leads to the question: What is the username used for at all? Couldn't it be removed alltogether? If I see it correctly, it's not shown in the frontend

Steps to Reproduce

Setup LibreChat to authenticate via Azure AD as explained in the documentation
Create a user with a given name not matching the regex
Try to login

What browsers are you seeing the problem on?

No response

Relevant log output

No response

Screenshots

No response

Code of Conduct

[X] I agree to follow this project's Code of Conduct

danny-avila commented 1 year ago

I think the way JWT is currently handled, it's required, but maybe we can remove this strictness in the matching requirement. Going to look into it!

danny-avila commented 1 year ago

@Berry-13 is working on a fix for this!

danny-avila commented 1 year ago

@larsrinn While I couldn't reproduce the exact scenario you have, I merged a change that should fix your issue.

larsrinn commented 1 year ago

Great. Thank you. I had to delete the users collection because it still had the constraint at the database level. Now it seems to work and it was no issues, because we didn't have any users yet. But is there a way around this? Sorry, I don't have experience with Mongo and mongoose. Coming more from the Python and Postgres world

danny-avila commented 1 year ago

Great. Thank you. I had to delete the users collection because it still had the constraint at the database level. Now it seems to work and it was no issues, because we didn't have any users yet. But is there a way around this? Sorry, I don't have experience with Mongo and mongoose. Coming more from the Python and Postgres world

This might help https://stackoverflow.com/questions/44318188/add-new-validator-to-existing-collection

In general from what I gather, you have to follow what the stackoverflow link says (run a script or use mongo shell), or migrate the data to a new collection. Either way is not so simple and starting the collection over as you did would be easier so past validation doesn't exist.

larsrinn commented 1 year ago

Either way is not so simple and starting the collection over as you did would be easier so past validation doesn't exist.

Now this was easily possible because there were no actual users. But if there had been, I would have deleted all their accounts and content, which doesn't sound great.

I did a quick search in the internet whether there is something like schema migrations in mongoose but was unsuccessful. Are you aware of this? It would be ideal, if there was a script which could be executed at every deployment, which updates the database levels constraints to match the ones defined in the schema currently.

danny-avila / LibreChat