[GitHub] [nifi] thenatog commented on a change in pull request #4593: NIFI-7584 Added OIDC logout mechanism.
thenatog commented on a change in pull request #4593:
URL: https://github.com/apache/nifi/pull/4593#discussion_r509539937
##
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/AccessResource.java
##
@@ -851,6 +1086,47 @@ private void forwardToMessagePage(final
HttpServletRequest httpServletRequest, f
uiContext.getRequestDispatcher("/WEB-INF/pages/message-page.jsp").forward(httpServletRequest,
httpServletResponse);
}
+private String determineLogoutMethod(String oidcDiscoveryUrl) {
+Matcher accessTokenMatcher =
REVOKE_ACCESS_TOKEN_LOGOUT_FORMAT.matcher(oidcDiscoveryUrl);
+Matcher idTokenMatcher =
ID_TOKEN_LOGOUT_FORMAT.matcher(oidcDiscoveryUrl);
+
+if (accessTokenMatcher.find()) {
+return REVOKE_ACCESS_TOKEN_LOGOUT;
+} else if (idTokenMatcher.find()) {
+return ID_TOKEN_LOGOUT;
+} else {
+return STANDARD_LOGOUT;
+}
+}
+
+private URI oidcRequestAuthorizationCode(@Context HttpServletResponse
httpServletResponse, String callback) {
+
+final String oidcRequestIdentifier = UUID.randomUUID().toString();
+
+// generate a cookie to associate this login sequence
+final Cookie cookie = new Cookie(OIDC_REQUEST_IDENTIFIER,
oidcRequestIdentifier);
Review comment:
Yes, from what I can tell the cookie is being used to maintain state for
the IDP login callback. Not exactly sure why this is a cookie. It's possible
the IDP strips headers for the callback redirect. Not sure if @mtien-apache saw
the JWT being stripped by the provider for callbacks? We could check with
mcgilman why it was set up to use a cookie.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi] thenatog commented on a change in pull request #4593: NIFI-7584 Added OIDC logout mechanism.
thenatog commented on a change in pull request #4593:
URL: https://github.com/apache/nifi/pull/4593#discussion_r509401269
##
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/AccessResource.java
##
@@ -851,6 +1086,47 @@ private void forwardToMessagePage(final
HttpServletRequest httpServletRequest, f
uiContext.getRequestDispatcher("/WEB-INF/pages/message-page.jsp").forward(httpServletRequest,
httpServletResponse);
}
+private String determineLogoutMethod(String oidcDiscoveryUrl) {
+Matcher accessTokenMatcher =
REVOKE_ACCESS_TOKEN_LOGOUT_FORMAT.matcher(oidcDiscoveryUrl);
+Matcher idTokenMatcher =
ID_TOKEN_LOGOUT_FORMAT.matcher(oidcDiscoveryUrl);
+
+if (accessTokenMatcher.find()) {
+return REVOKE_ACCESS_TOKEN_LOGOUT;
+} else if (idTokenMatcher.find()) {
+return ID_TOKEN_LOGOUT;
+} else {
+return STANDARD_LOGOUT;
+}
+}
+
+private URI oidcRequestAuthorizationCode(@Context HttpServletResponse
httpServletResponse, String callback) {
+
+final String oidcRequestIdentifier = UUID.randomUUID().toString();
+
+// generate a cookie to associate this login sequence
+final Cookie cookie = new Cookie(OIDC_REQUEST_IDENTIFIER,
oidcRequestIdentifier);
Review comment:
This cookie generation is done in two places (lines 1104 -1115 and
194-205). You could extract this to a private cookie generation method,
something along the lines of:
httpServletResponse.addCookie(generateCookie(oidcRequestIdentifier));
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi] thenatog commented on a change in pull request #4593: NIFI-7584 Added OIDC logout mechanism.
thenatog commented on a change in pull request #4593:
URL: https://github.com/apache/nifi/pull/4593#discussion_r504226782
##
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/AccessResource.java
##
@@ -329,24 +359,221 @@ public Response oidcExchange(@Context HttpServletRequest
httpServletRequest, @Co
)
public void oidcLogout(@Context HttpServletRequest httpServletRequest,
@Context HttpServletResponse httpServletResponse) throws Exception {
if (!httpServletRequest.isSecure()) {
-throw new IllegalStateException("User authentication/authorization
is only supported when running over HTTPS.");
+throw new IllegalStateException(AUTHENTICATION_NOT_ENABLED_MSG);
}
if (!oidcService.isOidcEnabled()) {
-throw new IllegalStateException("OpenId Connect is not
configured.");
+throw new
IllegalStateException(OPEN_ID_CONNECT_SUPPORT_IS_NOT_CONFIGURED_MSG);
}
-URI endSessionEndpoint = oidcService.getEndSessionEndpoint();
-String postLogoutRedirectUri = generateResourceUri("..", "nifi",
"logout-complete");
+// Get the oidc discovery url
+String oidcDiscoveryUrl = properties.getOidcDiscoveryUrl();
+
+// Determine the logout method
+String logoutMethod = determineLogoutMethod(oidcDiscoveryUrl);
+
+switch (logoutMethod) {
+case REVOKE_ACCESS_TOKEN_LOGOUT:
Review comment:
Just to confirm, if the logoutMethod found is
REVOKE_ACCESS_TOKEN_LOGOUT, we fall through to ID_TOKEN_LOGOUT, and if it's
STANDARD_LOGOUT we do the default case? My reading of this is that the
REVOKE_ACCESS_TOKEN_LOGOUT and ID_TOKEN_LOGOUT are the same (run the same code).
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi] thenatog commented on a change in pull request #4593: NIFI-7584 Added OIDC logout mechanism.
thenatog commented on a change in pull request #4593:
URL: https://github.com/apache/nifi/pull/4593#discussion_r504221263
##
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/AccessResource.java
##
@@ -329,24 +359,221 @@ public Response oidcExchange(@Context HttpServletRequest
httpServletRequest, @Co
)
public void oidcLogout(@Context HttpServletRequest httpServletRequest,
@Context HttpServletResponse httpServletResponse) throws Exception {
if (!httpServletRequest.isSecure()) {
-throw new IllegalStateException("User authentication/authorization
is only supported when running over HTTPS.");
+throw new IllegalStateException(AUTHENTICATION_NOT_ENABLED_MSG);
}
if (!oidcService.isOidcEnabled()) {
-throw new IllegalStateException("OpenId Connect is not
configured.");
+throw new
IllegalStateException(OPEN_ID_CONNECT_SUPPORT_IS_NOT_CONFIGURED_MSG);
Review comment:
Unlike other uses of the isOIdcEnabled() method, this does not redirect
to an error message page. Should it? Is there a reason it's different to the
other uses?
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [nifi] thenatog commented on a change in pull request #4593: NIFI-7584 Added OIDC logout mechanism.
thenatog commented on a change in pull request #4593:
URL: https://github.com/apache/nifi/pull/4593#discussion_r504218997
##
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/api/AccessResource.java
##
@@ -329,24 +359,221 @@ public Response oidcExchange(@Context HttpServletRequest
httpServletRequest, @Co
)
public void oidcLogout(@Context HttpServletRequest httpServletRequest,
@Context HttpServletResponse httpServletResponse) throws Exception {
if (!httpServletRequest.isSecure()) {
-throw new IllegalStateException("User authentication/authorization
is only supported when running over HTTPS.");
+throw new IllegalStateException(AUTHENTICATION_NOT_ENABLED_MSG);
}
if (!oidcService.isOidcEnabled()) {
-throw new IllegalStateException("OpenId Connect is not
configured.");
+throw new
IllegalStateException(OPEN_ID_CONNECT_SUPPORT_IS_NOT_CONFIGURED_MSG);
}
-URI endSessionEndpoint = oidcService.getEndSessionEndpoint();
-String postLogoutRedirectUri = generateResourceUri("..", "nifi",
"logout-complete");
+// Get the oidc discovery url
+String oidcDiscoveryUrl = properties.getOidcDiscoveryUrl();
+
+// Determine the logout method
+String logoutMethod = determineLogoutMethod(oidcDiscoveryUrl);
+
+switch (logoutMethod) {
+case REVOKE_ACCESS_TOKEN_LOGOUT:
+case ID_TOKEN_LOGOUT:
+// Make a request to the IdP
+URI authorizationURI =
oidcRequestAuthorizationCode(httpServletResponse, getOidcLogoutCallback());
+httpServletResponse.sendRedirect(authorizationURI.toString());
+break;
+case STANDARD_LOGOUT:
+default:
+// Get the OIDC end session endpoint
+URI endSessionEndpoint = oidcService.getEndSessionEndpoint();
+String postLogoutRedirectUri = generateResourceUri( "..",
"nifi", "logout-complete");
+
+if (endSessionEndpoint == null) {
+httpServletResponse.sendRedirect(postLogoutRedirectUri);
+} else {
+URI logoutUri = UriBuilder.fromUri(endSessionEndpoint)
+.queryParam("post_logout_redirect_uri",
postLogoutRedirectUri)
+.build();
+httpServletResponse.sendRedirect(logoutUri.toString());
+}
+break;
+}
+}
-if (endSessionEndpoint == null) {
-// handle the case, where the OpenID Provider does not have an end
session endpoint
-httpServletResponse.sendRedirect(postLogoutRedirectUri);
+@GET
+@Consumes(MediaType.WILDCARD)
+@Produces(MediaType.WILDCARD)
+@Path("oidc/logoutCallback")
+@ApiOperation(
+value = "Redirect/callback URI for processing the result of the
OpenId Connect logout sequence.",
+notes = NON_GUARANTEED_ENDPOINT
+)
+public void oidcLogoutCallback(@Context HttpServletRequest
httpServletRequest, @Context HttpServletResponse httpServletResponse) throws
Exception {
+// only consider user specific access over https
+if (!httpServletRequest.isSecure()) {
+forwardToMessagePage(httpServletRequest, httpServletResponse,
AUTHENTICATION_NOT_ENABLED_MSG);
+return;
+}
+
+// ensure oidc is enabled
+if (!oidcService.isOidcEnabled()) {
+forwardToMessagePage(httpServletRequest, httpServletResponse,
OPEN_ID_CONNECT_SUPPORT_IS_NOT_CONFIGURED_MSG);
+return;
+}
+
+final String oidcRequestIdentifier =
getCookieValue(httpServletRequest.getCookies(), OIDC_REQUEST_IDENTIFIER);
+if (oidcRequestIdentifier == null) {
+forwardToMessagePage(httpServletRequest, httpServletResponse, "The
login request identifier was not found in the request. Unable to continue.");
+return;
+}
+
+final com.nimbusds.openid.connect.sdk.AuthenticationResponse
oidcResponse;
+try {
+oidcResponse = AuthenticationResponseParser.parse(getRequestUri());
+} catch (final ParseException e) {
+logger.error("Unable to parse the redirect URI from the OpenId
Connect Provider. Unable to continue logout process.");
+
+// remove the oidc request cookie
+removeOidcRequestCookie(httpServletResponse);
+
+// forward to the error page
+forwardToMessagePage(httpServletRequest, httpServletResponse,
"Unable to parse the redirect URI from the OpenId Connect Provider. Unable to
continue logout process.");
+return;
+}
+
+if (oidcResponse.indicatesSuccess()) {
+final AuthenticationSuccessResponse successfulOidcResponse =
(Authentication
[GitHub] [nifi] thenatog commented on a change in pull request #4593: NIFI-7584 Added OIDC logout mechanism.
thenatog commented on a change in pull request #4593: URL: https://github.com/apache/nifi/pull/4593#discussion_r503595193 ## File path: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/pom.xml ## @@ -422,5 +422,23 @@ 1.3 test + +org.apache.httpcomponents +httpclient + + +org.apache.httpcomponents +httpclient + + +org.apache.httpcomponents +httpclient + + +com.squareup.okhttp3 Review comment: Is this dependency required? I don't see it used. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
