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

initial ui for organizations #29643

Merged
merged 33 commits into from
May 29, 2024
Merged

initial ui for organizations #29643

merged 33 commits into from
May 29, 2024

Conversation

edewit
Copy link
Contributor

@edewit edewit commented May 17, 2024

It's missing cypress tests, but I'm putting this out here to collect some feedback

Closes #29759

@edewit edewit self-assigned this May 17, 2024
Copy link
Contributor

@ssilvert ssilvert left a comment

Choose a reason for hiding this comment

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

@edewit I tried this out and everything looks great except I can't add identity providers. The dropdown is not working.

Not sure if you expect it to work yet?

@pedroigor
Copy link
Contributor

pedroigor commented May 20, 2024

The identity provider dropdown works for me after typing something to run a search. But I think we are not yet filtering out the identity providers already linked to an organization, are we?

The comments I have are:

  • The domain select box for identity providers does not have a "Select one" option or something to indicate that there is no domain set. I also can not unset a domain. I understand there is a marker to indicate if a domain is set, so please ignore this comment if that is the expected behavior.
  • I noticed that endpoints are being called twice when fetching data from the server. And this is not only for orgs but any UI. I guess this is because I'm running pnpn run dev?
  • Domain when creating/updating the organization is not mandatory
  • The title of the dialog when linking a broker is "Invite Member"
  • When a broker is already linked to an organization and I try to add the same one to another organization, clicking on the save button on the Link identity provider dialog tries to update the broker instead of calling the endpoint to link the broker. If the broker is not yet associated with an org, it works fine.

@edewit Other than that, LGTM. Thanks for it!

@pedroigor
Copy link
Contributor

I also found some issues in the backend we need to fix when managing brokers:

  • Ignore empty strings when checking for the broker domain. The UI is sending an empty string and we need to ignore it.

@pedroigor
Copy link
Contributor

UI failure might be related to #29691

@xianli123
Copy link
Contributor

Hi @pedroigor @edewit @ssilvert I was trying to reply to the comments that Pedro left:

But I think we are not yet filtering out the identity providers already linked to an organization, are we?

I agree with filtering out the IDPs that are already linked to the current org. I'd like not to show the IDPs in the dropdown list that are already linked to the org.

The domain select box for identity providers does not have a "Select one" option or something to indicate that there is no domain set.

We can add a "None" option and make it selected by default as the screenshot shows below. We have the same UI in the current console.
Screenshot 2024-05-21 at 14 06 34

WDYT?

@xianli123
Copy link
Contributor

I quoted @pedroigor 's comments below, and have a question about the IDP table.

When a broker is already linked to an organization and I try to add the same one to another organization, clicking on the save button on the Link identity provider dialog tries to update the broker instead of calling the endpoint to link the broker. If the broker is not yet associated with an org, it works fine.

Do we need to indicate if the IDPs are linked to organizations in the IDPs table as shown below?
Screenshot 2024-05-21 at 14 18 11

@edewit
Copy link
Contributor Author

edewit commented May 21, 2024

The identity provider dropdown works for me after typing something to run a search. But I think we are not yet filtering out the identity providers already linked to an organization, are we?

No I'm not filtering out the IDPs that have already been linked, I'm now using the "normal" idp endpoint, I'll have to see if there is a search parameter that I can use to give me only unlinked once.

The comments I have are:

  • The domain select box for identity providers does not have a "Select one" option or something to indicate that there is no domain set. I also can not unset a domain. I understand there is a marker to indicate if a domain is set, so please ignore this comment if that is the expected behavior.

Right I'll use "None" like Kun suggested as that indicates more the intent of leaving it blank.

  • I noticed that endpoints are being called twice when fetching data from the server. And this is not only for orgs but any UI. I guess this is because I'm running pnpn run dev?

Yes this is something that react does in development mode to check if we don't break the "Rules of React". In production it will only call the endpoints once

  • Domain when creating/updating the organization is not mandatory

Right hence addind the None opion

  • The title of the dialog when linking a broker is "Invite Member"
  • When a broker is already linked to an organization and I try to add the same one to another organization, clicking on the save button on the Link identity provider dialog tries to update the broker instead of calling the endpoint to link the broker. If the broker is not yet associated with an org, it works fine.

@edewit Other than that, LGTM. Thanks for it!

@pedroigor
Copy link
Contributor

Do we need to indicate if the IDPs are linked to organizations in the IDPs table as shown below?

We can do that if you check for the kc.org config entry. If it is there, it is linked to an organization.

@edewit
Copy link
Contributor Author

edewit commented May 21, 2024

@edewit I tried this out and everything looks great except I can't add identity providers. The dropdown is not working.

Not sure if you expect it to work yet?

Changed it so now shows the initial 20

@pedroigor
Copy link
Contributor

pedroigor commented May 21, 2024

@edewit The latest changes, LGTM. There are some changes we need in the backend to allow unset a domain and we are preparing a follow-up for it.

@pedroigor
Copy link
Contributor

Should also fix #29759.

@@ -1672,7 +1673,9 @@ private static void updateOrganizationBroker(RealmModel realm, IdentityProviderR

String domain = representation.getConfig().get(OrganizationModel.ORGANIZATION_DOMAIN_ATTRIBUTE);

if (domain != null && org.getDomains().map(OrganizationDomainModel::getName).noneMatch(domain::equals)) {
if (StringUtil.isBlank(domain)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Changed this to make sure empty strings remove the domain from the broker config.

Copy link
Contributor

@vramik vramik left a comment

Choose a reason for hiding this comment

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

Thank you @edewit for the PR.

I've found few comments:

  • I've noticed you can put multiple domains with same value when creating/updating an org
    image
    ... only single domain is present in OrganizationRepresentation in the request. When the page is refreshed it shows correctly only one domain.

EDIT: not reproducible any more, probably some issue on my end
- If you update the domain from e.g. orga.com to orgb.com and press save it's not propagated to the backend and when page is refreshed there is again orga.com.

createOrganization=Create organization
domain=Domain
organizationDomainHelp=What is a domain
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose this help text is temporary, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I didn't have text to explain this field yet

Copy link
Contributor

Choose a reason for hiding this comment

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

@edewit Pedro has provided a help text for the Domain. FYI:
"Sets one or more domains to an organization so that users are mapped to an organization whenever their email domains match one of the domains herein set forth."

edewit and others added 24 commits May 29, 2024 07:22
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Closes keycloak#29759

Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Copy link
Contributor

@stianst stianst left a comment

Choose a reason for hiding this comment

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

Approving based on review from @pedroigor

@stianst stianst merged commit f088b00 into keycloak:main May 29, 2024
71 checks passed
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.

Do not validate broker domain when the domain is an empty string
8 participants