RocketChat / Rocket.Chat

The communications platform that puts data protection first.
https://rocket.chat/
Other
40.63k stars 10.64k forks source link

Restore Username Field template tags to Custom OAuth Config for non-nested attributes #13480

Open Skosche3 opened 5 years ago

Skosche3 commented 5 years ago

Description:

As a RC_Admin, I would like custom oauth config to accept combination of various attributes similar to LDAP implementation so that non-nested attributes can be combined to provide username (ie #{givenName}.#{sn})

https://github.com/RocketChat/Rocket.Chat/issues/9051 removed required functionality that was added with https://github.com/RocketChat/Rocket.Chat/issues/3917.

Suggest logic be added to handle both use cases for getUserName Here is the code that was removed for our needed use case. https://github.com/RocketChat/Rocket.Chat/blob/c22467482123418604fe29ca9fef80e5fe0e3752/packages/rocketchat-custom-oauth/server/custom_oauth_server.js#L241

Steps to reproduce:

  1. Administration->OAuth
  2. Create custom oauth
  3. Set Username field: #{lastname}.#{firstname}.#{uid}

Expected behavior:

getUserName able to parse identity response data with fields like { "uid": "12345", "lastname": "Jones", "firstname": "Bob" } and create Jones.Bob.12345

Actual behavior:

username is undefined with current implementation and broke deployments counting on this functionality upon upgrading to newer version.

Server Setup Information:

Additional context

N/A

Relevant logs:

N/A

Skosche3 commented 5 years ago

@geekgonecrazy You may be interested in why this code was necessary. Hopefully we can create documentation for OAuth configuration so required use cases make sense for others to use.

@pierreozoux You may want to track this issue to ensure solution preserves your use case for nested attributes.

pierreozoux commented 5 years ago

Here is the PR: https://github.com/RocketChat/Rocket.Chat/pull/9066/files

From what I recall, this didn't work, I mean it was impossible to search for fields in the json, and now it works, I mean you can search for fields in the json.

Can you share your json?

Skosche3 commented 5 years ago

Set Username field: #{lastname}.#{firstname}.#{uid} JSON EX: { "uid": "12345", "lastname": "Jones", "firstname": "Bob" } username returns Jones.Bob.12345

The identity response JSON has additional fields that are not relevant to the point. Nothing is nested.

pierreozoux commented 5 years ago

Ok, I think I understand your problem. Now, the code that lookup one field, it doesn't create it with multiple part.

Do you understand what I mean?

I don't think it makes sense to create the username field with multiple parts. If you look at implementations in the wild, nobody does that (that I'm aware).

pierreozoux commented 5 years ago

Actually to make this PR I looked carefully.at implementations from discourse and gitlab, and I came to the conclusion that this is the way to go (and to be honest, I didn't get what the previous code was doing, and now I understand!)

pierreozoux commented 5 years ago

I'm sorry for you :/ but why don't you have a username field in your json?

Skosche3 commented 5 years ago

Let me see if I can provide a better example soon, I only pulled out a couple fields as an example of what we are trying to do. Unfortunately we don't control the service that is returning the identity response and have to work with what they provide us.

Our deployment upgrade was held up by this change and discovering the issue. We would like to avoid a custom copy of old function in place by getting something that works for all parties in core RC product and document clearly so community understands the purpose.

Originally, https://github.com/RocketChat/Rocket.Chat/issues/3917 was created to have the OAuth configuration be able to use a template like LDAP. I have not looked into what LDAP currently looks like.

Skosche3 commented 5 years ago

BTW, we agree with you and have reached out to the identity provider system and made similar recommendations....alas we work with what they give us. The username field is not useful and structure is not nested so we combine fields to make unique useful usernames.

pierreozoux commented 5 years ago

My best recommendation is to make a PR that allows people to configure both.

Skosche3 commented 5 years ago

We have a workaround in place now to restore code but hope to help get something that works for everyone submitted here.