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

Login: Allow custom name and icon for social providers #63297

Merged
merged 1 commit into from
Mar 3, 2023

Conversation

jkroepke
Copy link
Contributor

What is this feature?

This PR allows the modification of social login providers

Details image

Why do we need this feature?

Users may known to Login with company IDP or Login with Azure AD instead Login with Microsoft

Who is this feature for?

End Users

Which issue(s) does this PR fix?:

Fixes #

Special notes for your reviewer:

Okta social provider already supports this. The field name and icon is already availible for all social providers, but the frontend component does not care about it.

@jkroepke jkroepke requested review from torkelo and a team as code owners February 11, 2023 21:14
@jkroepke jkroepke requested review from joshhunt and JoaoSilvaGrafana and removed request for a team February 11, 2023 21:14
@CLAassistant
Copy link

CLAassistant commented Feb 11, 2023

CLA assistant check
All committers have signed the CLA.

@grafanabot grafanabot added area/frontend pr/external This PR is from external contributor labels Feb 11, 2023
@vtorosyan vtorosyan requested review from a team, linoman and kalleep and removed request for a team February 21, 2023 17:23
@vtorosyan vtorosyan requested a review from Jguer February 21, 2023 17:24
Copy link
Contributor

@kalleep kalleep left a comment

Choose a reason for hiding this comment

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

Thanks for the pr. Could you also update the sample.ini file with these values?

conf/defaults.ini Outdated Show resolved Hide resolved
@kalleep kalleep added this to the 9.5.0 milestone Feb 22, 2023
@kalleep kalleep added the no-backport Skip backport of PR label Feb 22, 2023
@jkroepke jkroepke requested a review from a team February 22, 2023 11:35
@gelicia gelicia requested a review from kalleep February 22, 2023 13:09
@kalleep
Copy link
Contributor

kalleep commented Feb 23, 2023

Sadly we cannot accept the pr, some providers has guidelines on how these buttons should look and feel.
Some references:
https://learn.microsoft.com/en-us/azure/active-directory/develop/howto-add-branding-in-azure-ad-apps
https://developers.google.com/identity/branding-guidelines

@kalleep kalleep closed this Feb 23, 2023
@jkroepke
Copy link
Contributor Author

@kalleep I'm bit sad to hear this.

The restrictions you mentioned are for marketplace related instances, but not for private instances. After merging the PR, the Sign button are still compliant for the mention guidelines. From grafana point of view, the Administrator of Grafana is now responsible for been complaint.

Please, re-think about this. We have a Single Tenant Application on Azure AD and 2 Tenants to connect.

I'm able to change the label of the generic oauth provider while I'm not able to change it for the azuread provider.

Right now, we on our login screen

  • Sign in with Tenant A
  • Sign in with Microsoft

Which confuses or end users.

I'm asking my self, why other solutions are less strict here and allows end-users to configure the label of the button, for example Gitlab, ArgoCD.

Please @kalleep, re-think about this, if Grafana should enforce this on application level OR if its okay the move guideline compliance to grafana end-users like me.

@danielkenlee
Copy link
Contributor

danielkenlee commented Mar 2, 2023

Hi @jkroepke, we discussed internally and have decided to allow merging the PR. You made some reasonable arguments implementing this change. Grafana also doesn't want to be in the position of enforcing the guidelines from the identity providers and rightly the responsibility should sit with end users / admins. We strive to provide consistent and powerful tooling to our users and recognise the value of this configuration option.

@danielkenlee danielkenlee reopened this Mar 2, 2023
@kalleep
Copy link
Contributor

kalleep commented Mar 2, 2023

@jkroepke before we can approve the pr, could you please also update the sample.ini file with the new options?

@jkroepke
Copy link
Contributor Author

jkroepke commented Mar 2, 2023

@kalleep done 👍

Copy link
Contributor

@linoman linoman left a comment

Choose a reason for hiding this comment

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

Great job @jkroepke

@kalleep kalleep merged commit c323a7c into grafana:main Mar 3, 2023
@jkroepke jkroepke deleted the social_custom_icon_name branch March 3, 2023 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants