exceptionfactory commented on a change in pull request #4988:
URL: https://github.com/apache/nifi/pull/4988#discussion_r621414106
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/jwt/NiFiBearerTokenResolver.java
##########
@@ -0,0 +1,64 @@
+/*
+ * Copyright 2012-2016 the original author or authors.
Review comment:
Can this date range be removed? Otherwise it should be updated.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/jwt/JwtAuthenticationFilter.java
##########
@@ -33,43 +32,25 @@
private static final Logger logger =
LoggerFactory.getLogger(JwtAuthenticationFilter.class);
// The Authorization header contains authentication credentials
- public static final String AUTHORIZATION = "Authorization";
- private static final Pattern tokenPattern = Pattern.compile("^Bearer
(\\S*\\.\\S*\\.\\S*)$");
+ public static final String JWT_COOKIE_NAME = "__Host-jwt-auth-cookie";
+ private static NiFiBearerTokenResolver bearerTokenResolver = new
NiFiBearerTokenResolver();
@Override
public Authentication attemptAuthentication(final HttpServletRequest
request) {
- // only support jwt login when running securely
+ // Only support JWT login when running securely
if (!request.isSecure()) {
return null;
}
- // TODO: Refactor request header extraction logic to shared utility as
it is duplicated in AccessResource
+ // Check for JWT in cookie and header
+ final String headerToken = bearerTokenResolver.resolve(request);
- // get the principal out of the user token
- final String authorizationHeader = request.getHeader(AUTHORIZATION);
-
- // if there is no authorization header, we don't know the user
- if (authorizationHeader == null ||
!validJwtFormat(authorizationHeader)) {
- return null;
- } else {
- // Extract the Base64 encoded token from the Authorization header
- final String token = getTokenFromHeader(authorizationHeader);
- return new JwtAuthenticationRequestToken(token,
request.getRemoteAddr());
- }
- }
-
- private boolean validJwtFormat(String authenticationHeader) {
- Matcher matcher = tokenPattern.matcher(authenticationHeader);
- return matcher.matches();
- }
-
- public static String getTokenFromHeader(String authenticationHeader) {
- Matcher matcher = tokenPattern.matcher(authenticationHeader);
- if(matcher.matches()) {
- return matcher.group(1);
+ if (StringUtils.isNotBlank(headerToken)) {
+ return new JwtAuthenticationRequestToken(headerToken,
request.getRemoteAddr());
+ } else if (WebUtils.getCookie(request, JWT_COOKIE_NAME) != null) {
+ return new
JwtAuthenticationRequestToken(WebUtils.getCookie(request,
JWT_COOKIE_NAME).getValue(), request.getRemoteAddr());
Review comment:
Instead of checking the Cookie value in this conditional, recommend
moving the logic to the `NiFiBearerTokenResolver` for better encapsulation.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/jwt/NiFiBearerTokenResolver.java
##########
@@ -0,0 +1,64 @@
+/*
+ * Copyright 2012-2016 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.security.jwt;
+
+import org.apache.nifi.web.security.InvalidAuthenticationException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.http.HttpServletRequest;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+public class NiFiBearerTokenResolver implements BearerTokenResolver {
+ private static final Logger logger =
LoggerFactory.getLogger(NiFiBearerTokenResolver.class);
+ private static final Pattern BEARER_HEADER_PATTERN =
Pattern.compile("^Bearer (\\S*\\.\\S*\\.\\S*)$");
+ private static final Pattern JWT_PATTERN =
Pattern.compile("^(\\S*\\.\\S*\\.\\S*)$");
+ public static final String AUTHORIZATION = "Authorization";
+
+ @Override
+ public String resolve(HttpServletRequest request) {
+ final String authorizationHeader = request.getHeader(AUTHORIZATION);
+
+ // if there is no authorization header, we don't know the user
+ if (authorizationHeader == null ||
!validAuthorizationHeaderFormat(authorizationHeader)) {
+ logger.debug("Authorization header was not present or not in a
valid format.");
+ return null;
+ } else {
+ // Extract the Base64 encoded token from the Authorization header
+ return getTokenFromHeader(authorizationHeader);
+ }
+ }
+
+ public boolean validAuthorizationHeaderFormat(String authorizationHeader) {
+ Matcher matcher = BEARER_HEADER_PATTERN.matcher(authorizationHeader);
+ return matcher.matches();
+ }
+
+ public boolean validJwtFormat(String jwt) {
+ Matcher matcher = JWT_PATTERN.matcher(jwt);
+ return matcher.matches();
+ }
+
+ public String getTokenFromHeader(String authenticationHeader) {
+ Matcher matcher = BEARER_HEADER_PATTERN.matcher(authenticationHeader);
+ if(matcher.matches()) {
Review comment:
Recommend checking formatting to add a space after `if`:
```suggestion
if (matcher.matches()) {
```
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/nf-common.js
##########
@@ -505,7 +505,7 @@
var interval = nfCommon.MILLIS_PER_MINUTE;
var checkExpiration = function () {
- var expiration = nfStorage.getItemExpiration('jwt');
+ var expiration = nfStorage.getItemExpiration('loggedIn');
Review comment:
Should this value be changed?
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/test/groovy/org/apache/nifi/web/security/jwt/JwtAuthenticationFilterTest.groovy
##########
@@ -83,7 +83,7 @@ class JwtAuthenticationFilterTest extends GroovyTestCase {
String authenticationHeader = "Bearer " + jwtString
// Act
- boolean isValidHeader = new
JwtAuthenticationFilter().validJwtFormat(authenticationHeader)
+ boolean isValidHeader = new
NiFiBearerTokenResolver().validAuthorizationHeaderFormat(authenticationHeader)
Review comment:
The references to `NiFiBearerTokenResolver` should be moved out of this
test class. Recommend that a new test class focus testing on the interface
method and not the other methods.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-api/src/main/java/org/apache/nifi/web/NiFiCsrfTokenRepository.java
##########
@@ -0,0 +1,88 @@
+/*
+ * Copyright 2012-2016 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web;
+
+
+import org.springframework.security.web.csrf.CookieCsrfTokenRepository;
+import org.springframework.security.web.csrf.CsrfToken;
+import org.springframework.security.web.csrf.CsrfTokenRepository;
+import org.springframework.security.web.csrf.DefaultCsrfToken;
+
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+/**
+ * A {@link CsrfTokenRepository} implementation for NiFi that matches the NiFi
Cookie JWT against the
+ * Authorization header JWT to protect against CSRF. If the request is an
idempotent method type, then only the Cookie
+ * is required to be present - this allows authenticating access to static
resources using a Cookie. If the request is a non-idempotent
+ * method, NiFi requires the Authorization header (eg. for POST requests).
+ */
+public final class NiFiCsrfTokenRepository implements CsrfTokenRepository {
+
+ private CookieCsrfTokenRepository cookieRepository;
+
+ public NiFiCsrfTokenRepository() {
+ cookieRepository = new CookieCsrfTokenRepository();
+ }
+
+ @Override
+ public CsrfToken generateToken(HttpServletRequest request) {
+ return new DefaultCsrfToken("empty", "empty", "empty");
Review comment:
Instead of returning this value, perhaps it would be better to throw
`UnsupportedOperationException("Token generated not supported")`. Based on the
logic in the Spring Security `CsrfFilter`, this method should never be called,
correct? Throwing an exception would clarify the intention of this
implementation.
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-ui/src/main/webapp/js/nf/login/nf-login.js
##########
@@ -155,20 +152,14 @@
});
};
- var showLogoutLink = function () {
- nfCommon.showLogoutLink();
- };
-
var nfLogin = {
/**
* Initializes the login page.
*/
init: function () {
nfStorage.init();
- if (nfStorage.getItem('jwt') !== null) {
- showLogoutLink();
- }
Review comment:
Should the changes in this file be reverted to preserve the current
logout presentation behavior?
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/jwt/JwtAuthenticationFilter.java
##########
@@ -33,43 +32,25 @@
private static final Logger logger =
LoggerFactory.getLogger(JwtAuthenticationFilter.class);
// The Authorization header contains authentication credentials
- public static final String AUTHORIZATION = "Authorization";
- private static final Pattern tokenPattern = Pattern.compile("^Bearer
(\\S*\\.\\S*\\.\\S*)$");
+ public static final String JWT_COOKIE_NAME = "__Host-jwt-auth-cookie";
Review comment:
Recommend removing `cookie` from the name and changing it to something
more generic.
```suggestion
public static final String JWT_COOKIE_NAME =
"__Host-Authorization-Bearer";
```
##########
File path:
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/jwt/NiFiBearerTokenResolver.java
##########
@@ -0,0 +1,64 @@
+/*
+ * Copyright 2012-2016 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.nifi.web.security.jwt;
+
+import org.apache.nifi.web.security.InvalidAuthenticationException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.http.HttpServletRequest;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+public class NiFiBearerTokenResolver implements BearerTokenResolver {
+ private static final Logger logger =
LoggerFactory.getLogger(NiFiBearerTokenResolver.class);
+ private static final Pattern BEARER_HEADER_PATTERN =
Pattern.compile("^Bearer (\\S*\\.\\S*\\.\\S*)$");
+ private static final Pattern JWT_PATTERN =
Pattern.compile("^(\\S*\\.\\S*\\.\\S*)$");
+ public static final String AUTHORIZATION = "Authorization";
+
+ @Override
+ public String resolve(HttpServletRequest request) {
+ final String authorizationHeader = request.getHeader(AUTHORIZATION);
+
+ // if there is no authorization header, we don't know the user
+ if (authorizationHeader == null ||
!validAuthorizationHeaderFormat(authorizationHeader)) {
+ logger.debug("Authorization header was not present or not in a
valid format.");
+ return null;
+ } else {
+ // Extract the Base64 encoded token from the Authorization header
+ return getTokenFromHeader(authorizationHeader);
+ }
+ }
+
+ public boolean validAuthorizationHeaderFormat(String authorizationHeader) {
+ Matcher matcher = BEARER_HEADER_PATTERN.matcher(authorizationHeader);
+ return matcher.matches();
+ }
+
+ public boolean validJwtFormat(String jwt) {
+ Matcher matcher = JWT_PATTERN.matcher(jwt);
+ return matcher.matches();
+ }
+
+ public String getTokenFromHeader(String authenticationHeader) {
Review comment:
Can these methods be marked `private`?
--
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:
[email protected]
