8000 only add realm access to the current realm by edewit · Pull Request #21554 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

only add realm access to the current realm - #21554

Merged
ahus1 merged 6 commits into
keycloak:mainfrom
edewit:issue-20453
Aug 10, 2023
Merged

only add realm access to the current realm#21554
ahus1 merged 6 commits into
keycloak:mainfrom
edewit:issue-20453

Conversation

@edewit
@edewit edewit commented Jul 10, 2023
Copy link
Copy Markdown
Contributor

fixes: #21553

@edewit
edewit requested review from a team as code owners July 10, 2023 12:27
@ghost ghost added team/ui labels Jul 10, 2023
@cypress
cypress Bot commented Jul 10, 2023 •
edited
Loading
Copy link
Copy Markdown

1 flaky tests on run #8444 ↗︎

0 527 48 0 Flakiness 1

Details:

Merge 2d1d776 into 66262d0...
Project: Keycloak Admin UI Commit: ce9eda1e95 ℹ️
Status: Passed Duration: 16:43 💡
Started: Aug 9, 2023 1:46 PM Ended: Aug 9, 2023 2:03 PM
Flakiness  cypress/e2e/realm_settings_general_tab_test.spec.ts • 1 flaky test • chrome

View Output Video

Test Artifacts
Realm settings general tab tests > Test all general tab switches Output Screenshots

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

jonkoops
jonkoops previously approved these changes Jul 11, 2023
View reviewed changes
jonkoops
jonkoops previously approved these changes Jul 14, 2023
View reviewed changes
@stianst
stianst commented Jul 17, 2023
Copy link
Copy Markdown
Contributor

Couple failing tests here:

AdminConsolePermissionsCalculatedTest:

I reckon we can just remove the line checking for permissions for the master realm:

keycloak/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/AdminConsolePermissionsCalculatedTest.java

Line 76 in 67b20df

assertTrue("Permissions for " + Config.getAdminRealm() + " realm.", jsonNode.at("/realm_access/" + Config.getAdminRealm()).isArray());

AdminSignatureAlgorithmTest:

A bit weird this test as I don't get why it is doing the who-ami as it seems completely irrelevant. I reckon we can just delete the lines:

keycloak/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/admin/AdminSignatureAlgorithmTest.java

Lines 61 to 65 in 67b20df

String whoAmiUrl = suiteContext.getAuthServerInfo().getContextRoot().toString() + "/auth/admin/master/console/whoami";
JsonNode jsonNode = SimpleHttp.doGet(whoAmiUrl, client).auth(accessToken.getToken()).asJson();
assertNotNull(jsonNode.get("realm"));
assertNotNull(jsonNode.get("userId"));

stianst
stianst reviewed Jul 17, 2023 8000
View reviewed changes
@stianst stianst left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM just need to get the two broken tests to pass

@stianst stianst mentioned this pull request Jul 17, 2023
Closed
@edewit
edewit requested review from a team as code owners July 18, 2023 09:47
@edewit
edewit requested a review from a team July 18, 2023 09:47
@ghost ghost mentioned this pull request Jul 18, 2023
Closed
@ghost
8000 ghost commented Jul 18, 2023
Copy link
Copy Markdown

Unreported flaky test detected

If the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.model.session.SessionTimeoutsTest#testOfflineUserClientIdleTimeoutSmallerThanSessionNoRefresh

Keycloak CI - Store Model Tests

java.lang.AssertionError: expected null, but was:<org.keycloak.models.sessions.infinispan.AuthenticatedClientSessionAdapter@41b39514>
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.failNotNull(Assert.java:756)
	at org.junit.Assert.assertNull(Assert.java:738)
	at org.junit.Assert.assertNull(Assert.java:748)
...

Report flaky test

ghost
ghost reviewed Jul 18, 2023
View reviewed changes
@ghost ghost left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

This was referenced Jul 19, 2023
Closed
Closed
Closed
Closed
ghost
ghost reviewed Jul 19, 2023
View reviewed changes
@ghost ghost left a comment
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@ghost
ghost commented Jul 19, 2023
Copy link
Copy Markdown

Unreported flaky test detected

If the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.ui.account2.DeviceActivityTest#clientsTest

Keycloak CI - Account Console IT (firefox)

java.lang.AssertionError: Expected DeviceActivityPage but was Keycloak Account Management (https://localhost:8543/auth/realms/test/account/#/security/device-activity&state=5bc2a3d0-3be5-477f-9d13-4b45177457dc&session_state=1baacd42-5c69-42a5-a42b-dec4a6af4d1c&code=91459da4-bdc8-49a5-bed4-7a45131f4c3a.1baacd42-5c69-42a5-a42b-dec4a6af4d1c.5b78e799-1129-4226-9a14-58527b01771b)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.page.AbstractPage.assertCurrent(AbstractPage.java:110)
	at jdk.internal.reflect.GeneratedMethodAccessor101.invoke(Unknown Source)
...

Report flaky test

ahus1
ahus1 approved these changes Aug 10, 2023
View reviewed changes
@ahus1 ahus1 left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed the code changes and tested it manually.
Thank you for this change - this is a big step forward to support a lot of realms in the UI!

agagancarczyk
agagancarczyk approved these changes Aug 10, 2023
View reviewed changes
@agagancarczyk agagancarczyk left a comment
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@ahus1
ahus1 merged commit 874d206 into keycloak:main Aug 10, 2023
@ahus1 ahus1 mentioned this pull request Aug 15, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stianst stianst stianst left review comments
@ahus1 ahus1 ahus1 approved these changes
+3 more reviewers
@agagancarczyk agagancarczyk agagancarczyk approved these changes
@jonkoops jonkoops jonkoops left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

flaky-test team/cloud-native

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

WhoAmI endpoint

5 participants

@edewit @stianst @jonkoops @ahus1 @agagancarczyk
0