-
Notifications
You must be signed in to change notification settings - Fork 75
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(groups): add setting display name to group backend #855
base: master
Are you sure you want to change the base?
feat(groups): add setting display name to group backend #855
Conversation
876bf14
to
c28b364
Compare
7478ee3
to
efa5f58
Compare
Signed-off-by: Iñaki Linaza Argüeso <[email protected]>
Signed-off-by: Iñaki Linaza Argüeso <[email protected]>
Signed-off-by: Iñaki Linaza Argüeso <[email protected]>
efa5f58
to
e941936
Compare
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
Hi @iLinaza thank you for your contribution. I wonder about the backgrounds of your change though. The group backend is presenting the groups as received from the IdP. Why should we allow manual changes of the group display name? |
Hi! Thank you for your reply. In my case what I need is to be able to set the display name of the groups. Solving it as an IdP mapping would be enough in my case and, as you say, is what would make sense. Actually, I may be wrong, the Group Display Name cannot be set as a mapping. Is this correct? I might try revising the PR to make the DisplayName set as from a mapping if that is the case. |
There is no mapping for group display names, no. That would also be strange as the response data is user focused. It would work if the group data structure we receive would contain both an identifier and a display name for each group, and this would need handling in our backend, as currently only a string list is a expected (for simplicity and compatibility this should be kept). This would my favored solution. |
If I understand correctly, do you mean changing the structure of the data received from the IdP and use a single mapping for both id and names? If this is your idea, I would need a little more detailed explanation to understand how this could be implemented. Or that, in addition to sending a list of group identifiers for the existing group id mapping (saml-attribute-mapping-group_mapping), a list of group names should also be sent, in the same order for a new display name mapping? The reason I want users to be able to see the group display name as well is because the group name is sometimes visible to them:
|
Right now we expect only a list of group names that we treat like IDs. Should still be supported. Then, to cater to your demands, we could also accept a json string, containing info like: [
{
"id": "group_161",
"name": "Department 161"
},
{
"id": "group_999",
"name": "Extraordinary Services"
}
] Not sure whether there is already a best practice out there on a format or such.
This is a possibility, but I am not very keen of having this information separated and combining it afterwards again. Unless there is a convincing case for this?
👍 |
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
SAML's GroupBackend does not allow you to set the display name of the groups.
The Nextcloud interface, ISetDisplayNameBackend, is implemented, which will enable setting the group name to work with SAML.
Also, group searching (on GroupBackend, getGroups method) also searches in display names.