logout with oidc #3049
logout with oidc #3049
Conversation
|
@jjoyce0510 : can you review? |
| ? config.getString(AUTH_BASE_URL_CONFIG_PATH) | ||
| : DEFAULT_BASE_URL_PATH; | ||
|
|
||
| _isOidcEnabled = config.hasPath("auth.oidc.enabled") && config.getBoolean("auth.oidc.enabled"); |
There was a problem hiding this comment.
Can we reuse some static constants for these string values?
There was a problem hiding this comment.
I dont see how they can be reused at the moment.
There was a problem hiding this comment.
They should be inside the OidcConfigs.java class as public static strings
| @@ -65,8 +65,10 @@ export const ManageAccount = ({ urn: _urn, pictureLink: _pictureLink, name }: Pr | |||
| </MenuItem> | |||
| ); | |||
| })} | |||
| <MenuItem id="user-profile-menu-logout" danger key="logout" onClick={handleLogout} tabIndex={0}> | |||
| Log out | |||
| <MenuItem danger key="logout" tabIndex={0}> | |||
There was a problem hiding this comment.
Have you validated that this still works for both OIDC and non-oidc (jaas) authentication modes?
There was a problem hiding this comment.
Yes, we validated in our env that it works for both OIDC (keycloak) and non-oidc authentication modes
There was a problem hiding this comment.
okay - thank you for confirming!
| */ | ||
| public Result executeLogout() throws ExecutionException, InterruptedException { | ||
| if (_isOidcEnabled) { | ||
| return logout().toCompletableFuture().get(); |
There was a problem hiding this comment.
Will this also delete the PLAY_SESSION cookie that DataHub uses for authentication?
There was a problem hiding this comment.
Nope, it will not delete the PLAY_SESSION cookie. It is still handled by MangeAccount (react).
datahub-frontend/conf/routes
Outdated
| @@ -45,6 +45,7 @@ GET /authenticate re | |||
| POST /logIn react.controllers.AuthenticationController.logIn() | |||
| GET /callback/oidc @org.pac4j.play.CallbackController.callback() | |||
| POST /callback/oidc @org.pac4j.play.CallbackController.callback() | |||
| GET /centralLogout controllers.CentralLogoutController.executeLogout() | |||
There was a problem hiding this comment.
What if we just call this "/logOut" for simplicity + consistency? The controller can figure out how to do that based on what auth mechanisms are enabled
There was a problem hiding this comment.
Yes, I will make the change shortly
…eAccount to call the new endpoint
| @@ -45,6 +45,7 @@ GET /authenticate re | |||
| POST /logIn react.controllers.AuthenticationController.logIn() | |||
| GET /callback/oidc @org.pac4j.play.CallbackController.callback() | |||
| POST /callback/oidc @org.pac4j.play.CallbackController.callback() | |||
| GET /logOut controllers.CentralLogoutController.executeLogout() | |||
There was a problem hiding this comment.
renamed to logOut as suggested
Co-authored-by: junjie <Jjlchiam4e>
Checklist
#2735
calling identity provider to kill the logout session