Add config to control whether to perform "OpenID Connect RP-Initiated Logout" when using an external OIDC provider #2590

peterhaochen47 · 2023-11-06T22:06:18Z

Notes for reviewer:

refactors of the impacted classes/code path will be in a different PR, so this PR would be easier to review.
reasons for the config name performRpInitiatedLogout:
- The exact language of the spec
- Follow the pattern of the other boolean configs of this endpoint: starting with a verb
- alternatives considered: SSOLogout, performSSOLogoutOnIdpAfterUAALogout, triggerExternalOIDCProviderLogout etc.

cf-gitbot · 2023-11-06T22:06:24Z

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/186419332

The labels on this github issue will be updated when the story is started.

- Add test coverage on the existing behavior described in #2589 where UAA attempts to log the user out of the external OIDC provider after a successful UAA logout (this is called the RP initiated logout) [#184752215]

- New config "performRpInitiatedLogout" (default to true to preserve existing behavior) added to /identity-providers API and uaa.yml. It is a flag controlling whether to log out of the external provider after a successful UAA logout per [OIDC RP-Initiated Logout](https://openid.net/specs/openid-connect-rpinitiated-1_0.html)" - doc changes [more context: #2589] [#184752215]

bruce-ricard

I'm quite confident that the change works as expected.

Just added a few minor readability comments inline.

bruce-ricard · 2023-11-07T20:42:06Z

uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/OIDCLoginIT.java

+    @Test
+    public void successfulUaaLogoutTriggersExternalOIDCProviderLogout() {
+        validateSuccessfulOIDCLogin(zoneUrl, testAccounts.getUserName(), testAccounts.getPassword());
+
+        String externalOIDCProviderLoginPage = baseUrl;
+        webDriver.get(externalOIDCProviderLoginPage);
+        Assert.assertThat("URL validation failed", webDriver.getCurrentUrl(), endsWith("/login"));
+    }
+
+    @Test
+    public void successfulUaaLogoutDoesNotTriggerExternalOIDCProviderLogout_whenConfiguredNotTo() {
+        identityProvider.getConfig().setPerformRpInitiatedLogout(false);
+        updateProvider();
+
+        validateSuccessfulOIDCLogin(zoneUrl, testAccounts.getUserName(), testAccounts.getPassword());
+
+        String externalOIDCProviderLoginPage = baseUrl;
+        webDriver.get(externalOIDCProviderLoginPage);
+        Assert.assertThat(webDriver.getPageSource(), containsString("Where to?"));
+    }
+


I don't understand which part of the tests check whether the OIDC logout is triggered or not.

It appears that you are testing that through the currentUrl and the PageSource of the webDriver?

If the integration tests aren't more precisely checking that we are indeed logged in/our of the OIDC provider, I don't really see the added value compared to the tests in ZoneAwareWhitelistLogoutHandlerTests.java

webDriver.get(externalOIDCProviderLoginPage); Assert.assertThat("URL validation failed", webDriver.getCurrentUrl(), endsWith("/login"));

^ This is going to the external OIDC provider login page (which is also a UAA), and verifies that you are redirected to the UAA /login endpoint. (which only happens if the user is unauthenticated/logged-out).

Assert.assertThat(webDriver.getPageSource(), containsString("Where to?"));

This "Where to?" string only shows up on the external OIDC provider login page (which is also a UAA), if you are still logged in as an authenticated user.

I understand that this way of checking is sort of hard to read, but it's consistent with how we perform the same type of check elsewhere. The "page object" refactor sort of addresses this readability issue by including this check inside of page objects, but I don't wanna include the page objects refactor in this PR, but I make it easy to perform that refactor in the future.

bruce-ricard · 2023-11-07T20:43:07Z

uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/OIDCLoginIT.java

+
+        String externalOIDCProviderLoginPage = baseUrl;
+        webDriver.get(externalOIDCProviderLoginPage);
+        Assert.assertThat("URL validation failed", webDriver.getCurrentUrl(), endsWith("/login"));


Why did you decide to add "URL validation failed" as the response to a failed assertion here? I don't feel like that's going to help debug.

I'm just following the newest existing pattern brought by the "page objects" refactor. I assumed there's some benefit to this pattern? At least shouldn't harm.

Hmm I don't like the word "validation" in this context.

shouldn't harm

I feel like the word "validation" shouts "I'm not test code". In test code we typically "assert", "test", "check". Validation is IMO when typically when real production code needs to validate something before doing something else with it.
It "harmed" my understanding when I read the code at first. I was thinking that it was a bad copy and paste from some functional code. I didn't think we could validate in tests.

@swalchemist what do you think?

I think I should improve that message. We should only add these assertion messages if there's no better way to improve our understanding of what the test is doing and/or what happens when it fails.

In this case, I think "We're at the right URL" works better as the message, e.g., "AssertThat ... we're at the right URL."

Ok, how about for the purpose of this PR, I'll forget about refactoring to using the shared "validateUrl" function in the future, and just formulate a reason param that works for the test here specifically (how about reason = "Not redirected to external OIDC provider login page." as that is what I'm actually testing here), or just taking the reason param out for now.

Commit pushed.

uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/OIDCLoginIT.java

...c/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthLogoutHandlerTest.java

.../java/org/cloudfoundry/identity/uaa/authentication/ZoneAwareWhitelistLogoutHandlerTests.java

...r/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/OauthIDPWrapperFactoryBean.java

.../main/java/org/cloudfoundry/identity/uaa/authentication/ZoneAwareWhitelistLogoutHandler.java

.../org/cloudfoundry/identity/uaa/provider/AbstractExternalOAuthIdentityProviderDefinition.java

- to improve readibility, in test setup, explicitly set "performRpInitiatedLogout" config to "true" (even though that is the current default) [#186127119]

.../org/cloudfoundry/identity/uaa/provider/AbstractExternalOAuthIdentityProviderDefinition.java

...r/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthLogoutHandler.java

bruce-ricard · 2023-11-08T00:16:46Z

uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/OIDCLoginIT.java

+
+        String externalOIDCProviderLoginPage = baseUrl;
+        webDriver.get(externalOIDCProviderLoginPage);
+        Assert.assertThat("URL validation failed", webDriver.getCurrentUrl(), endsWith("/login"));


Hmm I don't like the word "validation" in this context.

shouldn't harm

I feel like the word "validation" shouts "I'm not test code". In test code we typically "assert", "test", "check". Validation is IMO when typically when real production code needs to validate something before doing something else with it.
It "harmed" my understanding when I read the code at first. I was thinking that it was a bad copy and paste from some functional code. I didn't think we could validate in tests.

@swalchemist what do you think?

.../main/java/org/cloudfoundry/identity/uaa/authentication/ZoneAwareWhitelistLogoutHandler.java

- no need to use the Object Boolean when primitive boolean will do [#184752215]

[#184752215]

strehle · 2023-11-14T14:20:17Z

Review.
LGTM

2 remarks from sonar
https://sonarcloud.io/summary/new_code?id=cloudfoundry-identity-parent&pullRequest=2590

1 smell about raw type usage https://sonarcloud.io/project/issues?resolved=false&sinceLeakPeriod=true&types=CODE_SMELL&pullRequest=2590&id=cloudfoundry-identity-parent&open=AYunIauC0iXdSBD0Oshu
1 additional test might increase coverage on zone logout, e.g. https://sonarcloud.io/component_measures?id=cloudfoundry-identity-parent&pullRequest=2590&metric=new_coverage&view=list

strehle

see my comment

- based on Sonar scan suggestion [#184752215]

type parameters)." - based on Sonar scan suggestion [#184752215]

peterhaochen47 · 2023-11-15T20:40:11Z

Review. LGTM

2 remarks from sonar https://sonarcloud.io/summary/new_code?id=cloudfoundry-identity-parent&pullRequest=2590

1 smell about raw type usage https://sonarcloud.io/project/issues?resolved=false&sinceLeakPeriod=true&types=CODE_SMELL&pullRequest=2590&id=cloudfoundry-identity-parent&open=AYunIauC0iXdSBD0Oshu 1 additional test might increase coverage on zone logout, e.g. https://sonarcloud.io/component_measures?id=cloudfoundry-identity-parent&pullRequest=2590&metric=new_coverage&view=list

Thanks! I've addressed all the ones I can.

- a new oidc/oauth provider config "performRpInitiatedLogout" is added in cloudfoundry/uaa#2590 - this repo only requires an addition in the example config provided in the spec file (since uaa-release passes through the oauth/oidc provider config to uaa server verbatim, so no new translation logic required when adding a new config on this layer) - add the field to tests [#184752215]

peterhaochen47 added the DO NOT MERGE Internal Test or WIP, please DO NOT MERGE label Nov 6, 2023

cf-gitbot added the unscheduled label Nov 6, 2023

backfill test: RP initiated logout

3527e59

- Add test coverage on the existing behavior described in #2589 where UAA attempts to log the user out of the external OIDC provider after a successful UAA logout (this is called the RP initiated logout) [#184752215]

peterhaochen47 force-pushed the pr/develop/rp-initiated-logout-config branch from 677283f to 388ecbc Compare November 7, 2023 00:00

peterhaochen47 changed the title ~~[DRAFT] Add config for "OpenID Connect RP-Initiated Logout" when using an external OIDC provider~~ [DRAFT] Add config to control "OpenID Connect RP-Initiated Logout" when using an external OIDC provider Nov 7, 2023

peterhaochen47 changed the title ~~[DRAFT] Add config to control "OpenID Connect RP-Initiated Logout" when using an external OIDC provider~~ [DRAFT] Add config to control whether to perform "OpenID Connect RP-Initiated Logout" when using an external OIDC provider Nov 7, 2023

peterhaochen47 force-pushed the pr/develop/rp-initiated-logout-config branch from 388ecbc to 1f895f6 Compare November 7, 2023 00:02

peterhaochen47 force-pushed the pr/develop/rp-initiated-logout-config branch from 1f895f6 to c4b2118 Compare November 7, 2023 02:46

bruce-ricard reviewed Nov 7, 2023

View reviewed changes

refactor: makes test more readible

d02a9ff

- to improve readibility, in test setup, explicitly set "performRpInitiatedLogout" config to "true" (even though that is the current default) [#186127119]

bruce-ricard reviewed Nov 8, 2023

View reviewed changes

refactor: simplify type

00a3af2

- no need to use the Object Boolean when primitive boolean will do [#184752215]

peterhaochen47 marked this pull request as ready for review November 8, 2023 00:42

peterhaochen47 changed the title ~~[DRAFT] Add config to control whether to perform "OpenID Connect RP-Initiated Logout" when using an external OIDC provider~~ Add config to control whether to perform "OpenID Connect RP-Initiated Logout" when using an external OIDC provider Nov 8, 2023

peterhaochen47 added in_review The PR is currently in review and removed DO NOT MERGE Internal Test or WIP, please DO NOT MERGE labels Nov 8, 2023

peterhaochen47 requested a review from strehle November 8, 2023 00:42

peterhaochen47 removed in_review The PR is currently in review unscheduled labels Nov 9, 2023

refactor: improve test failure output

fe69e18

[#184752215]

peterhaochen47 requested a review from bruce-ricard November 9, 2023 19:15

strehle approved these changes Nov 14, 2023

View reviewed changes

bruce-ricard approved these changes Nov 14, 2023

View reviewed changes

peterhaochen47 added 2 commits November 15, 2023 09:47

improve test coverage

ace865b

- based on Sonar scan suggestion [#184752215]

refactor: address issue "Generic types should not be used raw (without

c52a0b9

type parameters)." - based on Sonar scan suggestion [#184752215]

peterhaochen47 merged commit 602e19a into develop Nov 15, 2023
18 checks passed

peterhaochen47 deleted the pr/develop/rp-initiated-logout-config branch November 15, 2023 23:50

peterhaochen47 mentioned this pull request Nov 17, 2023

Miscellaneous refactors #2608

Merged

peterhaochen47 mentioned this pull request Nov 17, 2023

Add spec file example for "performRpInitiatedLogout" cloudfoundry/uaa-release#716

Merged

cf-gitbot added delivered accepted Accepted the issue and removed delivered labels Jan 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add config to control whether to perform "OpenID Connect RP-Initiated Logout" when using an external OIDC provider #2590

Add config to control whether to perform "OpenID Connect RP-Initiated Logout" when using an external OIDC provider #2590

peterhaochen47 commented Nov 6, 2023 •

edited

Loading

cf-gitbot commented Nov 6, 2023

bruce-ricard left a comment

bruce-ricard Nov 7, 2023

peterhaochen47 Nov 7, 2023

bruce-ricard Nov 7, 2023

peterhaochen47 Nov 7, 2023

bruce-ricard Nov 8, 2023

swalchemist Nov 8, 2023

peterhaochen47 Nov 8, 2023

peterhaochen47 Nov 9, 2023

bruce-ricard Nov 8, 2023

strehle commented Nov 14, 2023

strehle left a comment

peterhaochen47 commented Nov 15, 2023

Add config to control whether to perform "OpenID Connect RP-Initiated Logout" when using an external OIDC provider #2590

Add config to control whether to perform "OpenID Connect RP-Initiated Logout" when using an external OIDC provider #2590

Conversation

peterhaochen47 commented Nov 6, 2023 • edited Loading

cf-gitbot commented Nov 6, 2023

bruce-ricard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strehle commented Nov 14, 2023

strehle left a comment

Choose a reason for hiding this comment

peterhaochen47 commented Nov 15, 2023

peterhaochen47 commented Nov 6, 2023 •

edited

Loading