You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by GitBox <gi...@apache.org> on 2022/11/18 21:13:18 UTC
[GitHub] [nifi] thenatog commented on a diff in pull request #6637: NIFI-10177: implemented ID token logout and revoke access token logou…
thenatog commented on code in PR #6637:
URL: https://github.com/apache/nifi/pull/6637#discussion_r1026848869
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/ApplicationResource.java:
##########
@@ -230,4 +230,10 @@ protected RevisionInfo getRevisionInfo(final LongParameter version, final Client
return revisionInfo;
}
+ private String getNiFiRegistryUri() {
+ final String nifiRegistryApiUrl = generateResourceUri();
+ final String baseUrl = StringUtils.substringBeforeLast(nifiRegistryApiUrl, "/nifi-registry-api");
+
+ return baseUrl + "/nifi-registry/";
Review Comment:
I would check if there's a way to use UriBuilder here to build this path instead of using string manipulation/substring
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/security/authentication/jwt/JwtService.java:
##########
@@ -175,6 +175,20 @@ public String generateSignedToken(String identity, String preferredUsername, Str
}
+ public void deleteKey(final String userIdentity) {
Review Comment:
Why does this method duplicate the below 'logOut()' method?
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessResource.java:
##########
@@ -704,20 +707,131 @@ public void oidcLogout(@Context HttpServletRequest httpServletRequest, @Context
throw new IllegalStateException("OpenId Connect is not configured.");
}
- final String tokenHeader = httpServletRequest.getHeader(JwtService.AUTHORIZATION);
- jwtService.logOutUsingAuthHeader(tokenHeader);
+ // Checks if OIDC service supports logout using either by
+ // 1. revoke access token method, or
+ // 2. ID token logout method.
+ // If either of the above methods are supported,
+ // redirects request to OP to request authorization that can be exchanged for a token used for logout
Review Comment:
Just want to confirm this - my understanding is that if we have an id_token, we can make a simple logout request to end_session_endpoint. If we only received an access_token initially, we need to HTTP request a new access_token and send this to revocation_endpoint. Is this how we are logging out/what this comment is saying?
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessResource.java:
##########
@@ -832,4 +950,133 @@ private boolean isBasicLoginSupported(HttpServletRequest request) {
private boolean isOIDCLoginSupported(HttpServletRequest request) {
return request.isSecure() && oidcService != null && oidcService.isOidcEnabled();
}
+
+ private String determineLogoutMethod() {
+ if (oidcService.getEndSessionEndpoint() != null) {
+ return ID_TOKEN_LOGOUT;
+ } else if (oidcService.getRevocationEndpoint() != null) {
+ return REVOKE_ACCESS_TOKEN_LOGOUT;
+ } else {
+ return STANDARD_LOGOUT;
+ }
+ }
+
+ /**
+ * Generates the request Authorization URI for the OpenID Connect Provider. Returns an authorization
+ * URI using the provided callback URI.
+ *
+ * @param httpServletResponse the servlet response
+ * @param callback the OIDC callback URI
+ * @return the authorization URI
+ */
+ private URI oidcRequestAuthorizationCode(@Context final HttpServletResponse httpServletResponse, final 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);
+ cookie.setPath("/");
+ cookie.setHttpOnly(true);
+ cookie.setMaxAge(60);
+ cookie.setSecure(true);
+ httpServletResponse.addCookie(cookie);
+
+ // get the state for this request
+ final State state = oidcService.createState(oidcRequestIdentifier);
+
+ // build the authorization uri
+ final URI authorizationUri = UriBuilder.fromUri(oidcService.getAuthorizationEndpoint())
+ .queryParam("client_id", oidcService.getClientId())
+ .queryParam("response_type", "code")
+ .queryParam("scope", oidcService.getScope().toString())
+ .queryParam("state", state.getValue())
+ .queryParam("redirect_uri", callback)
+ .build();
+ return authorizationUri;
+ }
+
+ private String getOidcRequestIdentifier(final HttpServletRequest httpServletRequest) {
+ return getCookieValue(httpServletRequest.getCookies(), OIDC_REQUEST_IDENTIFIER);
+ }
+
+ private com.nimbusds.openid.connect.sdk.AuthenticationResponse parseAuthenticationResponse(final URI requestUri,
Review Comment:
Is there a reason these nimbusds classes are being used vs our implementation?
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessResource.java:
##########
@@ -832,4 +950,133 @@ private boolean isBasicLoginSupported(HttpServletRequest request) {
private boolean isOIDCLoginSupported(HttpServletRequest request) {
return request.isSecure() && oidcService != null && oidcService.isOidcEnabled();
}
+
+ private String determineLogoutMethod() {
+ if (oidcService.getEndSessionEndpoint() != null) {
+ return ID_TOKEN_LOGOUT;
+ } else if (oidcService.getRevocationEndpoint() != null) {
+ return REVOKE_ACCESS_TOKEN_LOGOUT;
+ } else {
+ return STANDARD_LOGOUT;
+ }
+ }
+
+ /**
+ * Generates the request Authorization URI for the OpenID Connect Provider. Returns an authorization
+ * URI using the provided callback URI.
+ *
+ * @param httpServletResponse the servlet response
+ * @param callback the OIDC callback URI
+ * @return the authorization URI
+ */
+ private URI oidcRequestAuthorizationCode(@Context final HttpServletResponse httpServletResponse, final 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);
+ cookie.setPath("/");
+ cookie.setHttpOnly(true);
+ cookie.setMaxAge(60);
+ cookie.setSecure(true);
+ httpServletResponse.addCookie(cookie);
+
+ // get the state for this request
+ final State state = oidcService.createState(oidcRequestIdentifier);
+
+ // build the authorization uri
+ final URI authorizationUri = UriBuilder.fromUri(oidcService.getAuthorizationEndpoint())
+ .queryParam("client_id", oidcService.getClientId())
+ .queryParam("response_type", "code")
+ .queryParam("scope", oidcService.getScope().toString())
+ .queryParam("state", state.getValue())
+ .queryParam("redirect_uri", callback)
+ .build();
+ return authorizationUri;
+ }
+
+ private String getOidcRequestIdentifier(final HttpServletRequest httpServletRequest) {
+ return getCookieValue(httpServletRequest.getCookies(), OIDC_REQUEST_IDENTIFIER);
+ }
+
+ private com.nimbusds.openid.connect.sdk.AuthenticationResponse parseAuthenticationResponse(final URI requestUri,
+ final HttpServletResponse httpServletResponse,
+ final boolean isLogin) {
+ final com.nimbusds.openid.connect.sdk.AuthenticationResponse oidcResponse;
+ try {
+ oidcResponse = AuthenticationResponseParser.parse(requestUri);
+ } catch (final ParseException e) {
+ final String loginOrLogoutString = isLogin ? "login" : "logout";
+ logger.error(String.format("Unable to parse the redirect URI from the OpenId Connect Provider. Unable to continue %s process.", loginOrLogoutString));
+
+ // remove the oidc request cookie
+ removeOidcRequestCookie(httpServletResponse);
+
+ throw new IllegalStateException(String.format("Unable to parse the redirect URI from the OpenId Connect Provider. Unable to continue %s process.", loginOrLogoutString));
+ }
+ return oidcResponse;
+ }
+
+ private void validateOIDCState(final String oidcRequestIdentifier,
+ final AuthenticationSuccessResponse successfulOidcResponse,
+ final HttpServletResponse httpServletResponse,
+ final boolean isLogin) {
+ // confirm state
+ final State state = successfulOidcResponse.getState();
+ if (state == null || !oidcService.isStateValid(oidcRequestIdentifier, state)) {
+ final String loginOrLogoutMessage = isLogin ? "login" : "logout";
+ logger.error(String.format("The state value returned by the OpenId Connect Provider does not match the stored state. Unable to continue %s process.", loginOrLogoutMessage));
+
+ // remove the oidc request cookie
+ removeOidcRequestCookie(httpServletResponse);
+
+ throw new IllegalStateException(String.format("Purposed state does not match the stored state. Unable to continue %s process.", loginOrLogoutMessage));
+ }
+ }
+
+ /**
+ * Sends a POST request to the revoke endpoint to log out of the ID Provider.
+ *
+ * @param httpServletResponse the servlet response
+ * @param accessToken the OpenID Connect Provider access token
+ * @param revokeEndpoint the name of the cookie
+ * @throws IOException exceptional case for communication error with the OpenId Connect Provider
+ */
+ private void revokeEndpointRequest(@Context HttpServletResponse httpServletResponse, String accessToken, URI revokeEndpoint) throws IOException, NoSuchAlgorithmException {
+ final CloseableHttpClient httpClient = getHttpClient();
+ HttpPost httpPost = new HttpPost(revokeEndpoint);
+
+ List<NameValuePair> params = new ArrayList<>();
+ // Append a query param with the access token
+ params.add(new BasicNameValuePair("token", accessToken));
+ httpPost.setEntity(new UrlEncodedFormEntity(params));
+
+ try (CloseableHttpResponse response = httpClient.execute(httpPost)) {
+ if (response.getStatusLine().getStatusCode() == HTTPResponse.SC_OK) {
+ // redirect to NiFi Registry page after logout completes
+ logger.debug("You are logged out of the OpenId Connect Provider.");
+ final String postLogoutRedirectUri = getNiFiRegistryUri();
+ httpServletResponse.sendRedirect(postLogoutRedirectUri);
+ } else {
+ logger.error("There was an error logging out of the OpenId Connect Provider. " +
+ "Response status: " + response.getStatusLine().getStatusCode());
+ }
+ } finally {
+ httpClient.close();
+ }
+ }
+
+ private CloseableHttpClient getHttpClient() throws NoSuchAlgorithmException {
+ final int msTimeout = 30_000;
Review Comment:
This timeout value could be set as a final member variable and it might be better with a shorter value. In fact it looks like there's a nifi.registry.security.user.oidc.read.timeout and nifi.registry.security.user.oidc.connect.timeout properties that potentially could be used.
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessResource.java:
##########
@@ -832,4 +950,133 @@ private boolean isBasicLoginSupported(HttpServletRequest request) {
private boolean isOIDCLoginSupported(HttpServletRequest request) {
return request.isSecure() && oidcService != null && oidcService.isOidcEnabled();
}
+
+ private String determineLogoutMethod() {
+ if (oidcService.getEndSessionEndpoint() != null) {
+ return ID_TOKEN_LOGOUT;
+ } else if (oidcService.getRevocationEndpoint() != null) {
+ return REVOKE_ACCESS_TOKEN_LOGOUT;
+ } else {
+ return STANDARD_LOGOUT;
+ }
+ }
+
+ /**
+ * Generates the request Authorization URI for the OpenID Connect Provider. Returns an authorization
+ * URI using the provided callback URI.
+ *
+ * @param httpServletResponse the servlet response
+ * @param callback the OIDC callback URI
+ * @return the authorization URI
+ */
+ private URI oidcRequestAuthorizationCode(@Context final HttpServletResponse httpServletResponse, final 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);
+ cookie.setPath("/");
+ cookie.setHttpOnly(true);
+ cookie.setMaxAge(60);
+ cookie.setSecure(true);
+ httpServletResponse.addCookie(cookie);
+
+ // get the state for this request
+ final State state = oidcService.createState(oidcRequestIdentifier);
+
+ // build the authorization uri
+ final URI authorizationUri = UriBuilder.fromUri(oidcService.getAuthorizationEndpoint())
+ .queryParam("client_id", oidcService.getClientId())
+ .queryParam("response_type", "code")
+ .queryParam("scope", oidcService.getScope().toString())
+ .queryParam("state", state.getValue())
+ .queryParam("redirect_uri", callback)
+ .build();
+ return authorizationUri;
+ }
+
+ private String getOidcRequestIdentifier(final HttpServletRequest httpServletRequest) {
+ return getCookieValue(httpServletRequest.getCookies(), OIDC_REQUEST_IDENTIFIER);
+ }
+
+ private com.nimbusds.openid.connect.sdk.AuthenticationResponse parseAuthenticationResponse(final URI requestUri,
+ final HttpServletResponse httpServletResponse,
+ final boolean isLogin) {
+ final com.nimbusds.openid.connect.sdk.AuthenticationResponse oidcResponse;
+ try {
+ oidcResponse = AuthenticationResponseParser.parse(requestUri);
+ } catch (final ParseException e) {
+ final String loginOrLogoutString = isLogin ? "login" : "logout";
+ logger.error(String.format("Unable to parse the redirect URI from the OpenId Connect Provider. Unable to continue %s process.", loginOrLogoutString));
+
+ // remove the oidc request cookie
+ removeOidcRequestCookie(httpServletResponse);
+
+ throw new IllegalStateException(String.format("Unable to parse the redirect URI from the OpenId Connect Provider. Unable to continue %s process.", loginOrLogoutString));
+ }
+ return oidcResponse;
+ }
+
+ private void validateOIDCState(final String oidcRequestIdentifier,
+ final AuthenticationSuccessResponse successfulOidcResponse,
+ final HttpServletResponse httpServletResponse,
+ final boolean isLogin) {
+ // confirm state
+ final State state = successfulOidcResponse.getState();
+ if (state == null || !oidcService.isStateValid(oidcRequestIdentifier, state)) {
+ final String loginOrLogoutMessage = isLogin ? "login" : "logout";
+ logger.error(String.format("The state value returned by the OpenId Connect Provider does not match the stored state. Unable to continue %s process.", loginOrLogoutMessage));
+
+ // remove the oidc request cookie
+ removeOidcRequestCookie(httpServletResponse);
+
+ throw new IllegalStateException(String.format("Purposed state does not match the stored state. Unable to continue %s process.", loginOrLogoutMessage));
Review Comment:
Should this say 'Proposed state' ?
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessResource.java:
##########
@@ -119,6 +137,7 @@ public AccessResource(
this.oidcService = oidcService;
this.kerberosSpnegoIdentityProvider = kerberosSpnegoIdentityProvider;
this.identityProvider = identityProvider;
+ this.bearerTokenResolver = new DefaultBearerTokenResolver();
Review Comment:
This token resolver is using Spring directly whereas NiFi code also contains a StandardBearerTokenResolver implementation. We might want to confirm if we should use that instead.
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessResource.java:
##########
@@ -589,44 +611,25 @@ public void oidcCallback(@Context HttpServletRequest httpServletRequest, @Contex
throw new IllegalStateException("OpenId Connect is not configured.");
}
- final String oidcRequestIdentifier = getCookieValue(httpServletRequest.getCookies(), OIDC_REQUEST_IDENTIFIER);
+ final String oidcRequestIdentifier = getOidcRequestIdentifier(httpServletRequest);
if (oidcRequestIdentifier == null) {
throw new IllegalStateException("The login request identifier was not found in the request. Unable to continue.");
}
- 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 login process.");
-
- // remove the oidc request cookie
- removeOidcRequestCookie(httpServletResponse);
- // forward to the error page
- throw new IllegalStateException("Unable to parse the redirect URI from the OpenId Connect Provider. Unable to continue login process.");
- }
+ final com.nimbusds.openid.connect.sdk.AuthenticationResponse oidcResponse =
Review Comment:
Again here, any benefit to using these nimbusds implementations of AuthenticationResponse?
##########
nifi-registry/nifi-registry-core/nifi-registry-web-api/src/main/java/org/apache/nifi/registry/web/api/AccessResource.java:
##########
@@ -687,7 +690,7 @@ public Response oidcExchange(@Context HttpServletRequest httpServletRequest, @Co
return generateOkResponse(jwt).build();
}
- @DELETE
+ @GET
Review Comment:
Is there a reason to change this to a GET request? Historically I assume this has been a DELETE request for some time now, therefore this would potentially be changing the API contract.
--
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.
To unsubscribe, e-mail: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org