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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
