galovics commented on code in PR #4825: URL: https://github.com/apache/fineract/pull/4825#discussion_r2196829655
########## fineract-core/src/main/java/org/apache/fineract/infrastructure/core/filters/GeolocationHeaderFilter.java: ########## @@ -0,0 +1,97 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 + * + * http://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.fineract.infrastructure.core.filters; + +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import java.io.IOException; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.apache.commons.lang3.StringUtils; +import org.apache.fineract.infrastructure.core.config.FineractProperties; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.stereotype.Component; +import org.springframework.web.filter.OncePerRequestFilter; + +@Component +@ConditionalOnProperty(prefix = "fineract.geolocation", name = "enabled", havingValue = "false", matchIfMissing = false) +@RequiredArgsConstructor +@Slf4j +public class GeolocationHeaderFilter extends OncePerRequestFilter { + + private final FineractProperties fineractProperties; + + private static final String[] IP_HEADER_CANDIDATES = { "X-Forwarded-For", "Proxy-Client-IP", "WL-Proxy-Client-IP", + "HTTP_X_FORWARDED_FOR", "HTTP_X_FORWARDED", "HTTP_X_CLUSTER_CLIENT_IP", "HTTP_CLIENT_IP", "HTTP_FORWARDED_FOR", + "HTTP_FORWARDED", "HTTP_VIA", "REMOTE_ADDR" }; + + public static String getClientIpAddress(HttpServletRequest request) { Review Comment: Shouldn't be static. ########## fineract-core/src/main/java/org/apache/fineract/infrastructure/core/filters/GeolocationHeaderFilter.java: ########## @@ -0,0 +1,97 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 + * + * http://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.fineract.infrastructure.core.filters; + +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import java.io.IOException; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.apache.commons.lang3.StringUtils; +import org.apache.fineract.infrastructure.core.config.FineractProperties; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.stereotype.Component; +import org.springframework.web.filter.OncePerRequestFilter; + +@Component +@ConditionalOnProperty(prefix = "fineract.geolocation", name = "enabled", havingValue = "false", matchIfMissing = false) +@RequiredArgsConstructor +@Slf4j +public class GeolocationHeaderFilter extends OncePerRequestFilter { + + private final FineractProperties fineractProperties; + + private static final String[] IP_HEADER_CANDIDATES = { "X-Forwarded-For", "Proxy-Client-IP", "WL-Proxy-Client-IP", + "HTTP_X_FORWARDED_FOR", "HTTP_X_FORWARDED", "HTTP_X_CLUSTER_CLIENT_IP", "HTTP_CLIENT_IP", "HTTP_FORWARDED_FOR", + "HTTP_FORWARDED", "HTTP_VIA", "REMOTE_ADDR" }; + + public static String getClientIpAddress(HttpServletRequest request) { + for (String header : IP_HEADER_CANDIDATES) { + String ip = request.getHeader(header); + if (ip != null && ip.length() != 0 && !"unknown".equalsIgnoreCase(ip)) { + log.debug("CALLER IP : {}", ip); Review Comment: During development we run fineract in debug mode. This will spam the logs for sure. I'd change this to trace level. ########## fineract-provider/src/main/resources/application.properties: ########## @@ -62,6 +62,7 @@ fineract.correlation.enabled=${FINERACT_LOGGING_HTTP_CORRELATION_ID_ENABLED:fals fineract.correlation.header-name=${FINERACT_LOGGING_HTTP_CORRELATION_ID_HEADER_NAME:X-Correlation-ID} fineract.job.stuck-retry-threshold=${FINERACT_JOB_STUCK_RETRY_THRESHOLD:5} +fineract.geolocation.enabled=${FINERACT_GEOLOCATION_ENABLED:false} Review Comment: This is not geolocation. I'm not even sure why we are calling it "geolocation" something. It's simply client ip tracking, nothing else. ########## fineract-core/src/main/java/org/apache/fineract/infrastructure/core/filters/GeolocationHeaderFilter.java: ########## @@ -0,0 +1,97 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 + * + * http://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.fineract.infrastructure.core.filters; + +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import java.io.IOException; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.apache.commons.lang3.StringUtils; +import org.apache.fineract.infrastructure.core.config.FineractProperties; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.stereotype.Component; +import org.springframework.web.filter.OncePerRequestFilter; + +@Component +@ConditionalOnProperty(prefix = "fineract.geolocation", name = "enabled", havingValue = "false", matchIfMissing = false) +@RequiredArgsConstructor +@Slf4j +public class GeolocationHeaderFilter extends OncePerRequestFilter { + + private final FineractProperties fineractProperties; + + private static final String[] IP_HEADER_CANDIDATES = { "X-Forwarded-For", "Proxy-Client-IP", "WL-Proxy-Client-IP", + "HTTP_X_FORWARDED_FOR", "HTTP_X_FORWARDED", "HTTP_X_CLUSTER_CLIENT_IP", "HTTP_CLIENT_IP", "HTTP_FORWARDED_FOR", + "HTTP_FORWARDED", "HTTP_VIA", "REMOTE_ADDR" }; + + public static String getClientIpAddress(HttpServletRequest request) { + for (String header : IP_HEADER_CANDIDATES) { + String ip = request.getHeader(header); + if (ip != null && ip.length() != 0 && !"unknown".equalsIgnoreCase(ip)) { + log.debug("CALLER IP : {}", ip); + return ip; + } + } + log.debug("getRemoteAddr method : {}", request.getRemoteAddr()); + return request.getRemoteAddr(); + } + + @Override + protected void doFilterInternal(final HttpServletRequest request, final HttpServletResponse response, final FilterChain filterChain) + throws IOException, ServletException { + FineractProperties.FineractGeolocationProperties geolocationProperties = fineractProperties.getGeolocation(); + if (geolocationProperties.isEnabled()) { + handleClientIp(request, response, filterChain, geolocationProperties); + } else { + filterChain.doFilter(request, response); + } + + } + + private void handleClientIp(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain, + FineractProperties.FineractGeolocationProperties geolocationProperties) throws IOException, ServletException { + + String clientIpAddress = getClientIpAddress(request); + if (StringUtils.isNotBlank(clientIpAddress)) { + log.debug("Found Client IP in header : {}", clientIpAddress); Review Comment: Trace level. ########## fineract-provider/src/test/resources/application-test.properties: ########## @@ -20,6 +20,8 @@ fineract.node-id=1 fineract.security.basicauth.enabled=true +fineract.geolocation.enabled=false Review Comment: Why is this needed? by default the application.properties falls back to a false value. ########## fineract-core/src/main/java/org/apache/fineract/infrastructure/core/filters/GeolocationHeaderFilter.java: ########## @@ -0,0 +1,97 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 + * + * http://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.fineract.infrastructure.core.filters; + +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import java.io.IOException; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.apache.commons.lang3.StringUtils; +import org.apache.fineract.infrastructure.core.config.FineractProperties; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.stereotype.Component; +import org.springframework.web.filter.OncePerRequestFilter; + +@Component +@ConditionalOnProperty(prefix = "fineract.geolocation", name = "enabled", havingValue = "false", matchIfMissing = false) +@RequiredArgsConstructor +@Slf4j +public class GeolocationHeaderFilter extends OncePerRequestFilter { + + private final FineractProperties fineractProperties; + + private static final String[] IP_HEADER_CANDIDATES = { "X-Forwarded-For", "Proxy-Client-IP", "WL-Proxy-Client-IP", + "HTTP_X_FORWARDED_FOR", "HTTP_X_FORWARDED", "HTTP_X_CLUSTER_CLIENT_IP", "HTTP_CLIENT_IP", "HTTP_FORWARDED_FOR", + "HTTP_FORWARDED", "HTTP_VIA", "REMOTE_ADDR" }; + + public static String getClientIpAddress(HttpServletRequest request) { + for (String header : IP_HEADER_CANDIDATES) { + String ip = request.getHeader(header); + if (ip != null && ip.length() != 0 && !"unknown".equalsIgnoreCase(ip)) { Review Comment: "unknown"? I'd rather not hardcode this unless we have a good reason to. ########## fineract-core/src/main/java/org/apache/fineract/infrastructure/core/filters/GeolocationHeaderFilter.java: ########## @@ -0,0 +1,97 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 + * + * http://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.fineract.infrastructure.core.filters; + +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import java.io.IOException; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.apache.commons.lang3.StringUtils; +import org.apache.fineract.infrastructure.core.config.FineractProperties; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.stereotype.Component; +import org.springframework.web.filter.OncePerRequestFilter; + +@Component +@ConditionalOnProperty(prefix = "fineract.geolocation", name = "enabled", havingValue = "false", matchIfMissing = false) Review Comment: I'd rather not use the string version of this but use the typed version. We have many "XYZCondition" classes. ########## fineract-core/src/main/java/org/apache/fineract/infrastructure/core/filters/GeolocationHeaderFilter.java: ########## @@ -0,0 +1,97 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 + * + * http://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.fineract.infrastructure.core.filters; + +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import java.io.IOException; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.apache.commons.lang3.StringUtils; +import org.apache.fineract.infrastructure.core.config.FineractProperties; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.stereotype.Component; +import org.springframework.web.filter.OncePerRequestFilter; + +@Component +@ConditionalOnProperty(prefix = "fineract.geolocation", name = "enabled", havingValue = "false", matchIfMissing = false) +@RequiredArgsConstructor +@Slf4j +public class GeolocationHeaderFilter extends OncePerRequestFilter { + + private final FineractProperties fineractProperties; + + private static final String[] IP_HEADER_CANDIDATES = { "X-Forwarded-For", "Proxy-Client-IP", "WL-Proxy-Client-IP", + "HTTP_X_FORWARDED_FOR", "HTTP_X_FORWARDED", "HTTP_X_CLUSTER_CLIENT_IP", "HTTP_CLIENT_IP", "HTTP_FORWARDED_FOR", + "HTTP_FORWARDED", "HTTP_VIA", "REMOTE_ADDR" }; + + public static String getClientIpAddress(HttpServletRequest request) { + for (String header : IP_HEADER_CANDIDATES) { + String ip = request.getHeader(header); + if (ip != null && ip.length() != 0 && !"unknown".equalsIgnoreCase(ip)) { + log.debug("CALLER IP : {}", ip); + return ip; + } + } + log.debug("getRemoteAddr method : {}", request.getRemoteAddr()); + return request.getRemoteAddr(); + } + + @Override + protected void doFilterInternal(final HttpServletRequest request, final HttpServletResponse response, final FilterChain filterChain) + throws IOException, ServletException { + FineractProperties.FineractGeolocationProperties geolocationProperties = fineractProperties.getGeolocation(); Review Comment: I don't really get this check. Isn't the conditional on the bean handles this? ########## fineract-core/src/main/java/org/apache/fineract/infrastructure/core/filters/GeolocationHeaderFilter.java: ########## @@ -0,0 +1,97 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 + * + * http://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.fineract.infrastructure.core.filters; + +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import java.io.IOException; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.apache.commons.lang3.StringUtils; +import org.apache.fineract.infrastructure.core.config.FineractProperties; +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.stereotype.Component; +import org.springframework.web.filter.OncePerRequestFilter; + +@Component +@ConditionalOnProperty(prefix = "fineract.geolocation", name = "enabled", havingValue = "false", matchIfMissing = false) +@RequiredArgsConstructor +@Slf4j +public class GeolocationHeaderFilter extends OncePerRequestFilter { + + private final FineractProperties fineractProperties; + + private static final String[] IP_HEADER_CANDIDATES = { "X-Forwarded-For", "Proxy-Client-IP", "WL-Proxy-Client-IP", + "HTTP_X_FORWARDED_FOR", "HTTP_X_FORWARDED", "HTTP_X_CLUSTER_CLIENT_IP", "HTTP_CLIENT_IP", "HTTP_FORWARDED_FOR", + "HTTP_FORWARDED", "HTTP_VIA", "REMOTE_ADDR" }; + + public static String getClientIpAddress(HttpServletRequest request) { + for (String header : IP_HEADER_CANDIDATES) { + String ip = request.getHeader(header); + if (ip != null && ip.length() != 0 && !"unknown".equalsIgnoreCase(ip)) { + log.debug("CALLER IP : {}", ip); + return ip; + } + } + log.debug("getRemoteAddr method : {}", request.getRemoteAddr()); Review Comment: Same, trace level. ########## fineract-core/src/main/java/org/apache/fineract/infrastructure/core/filters/ClientIpHolder.java: ########## @@ -0,0 +1,39 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 + * + * http://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.fineract.infrastructure.core.filters; + +public final class ClientIpHolder { Review Comment: I suggest not doing it this way. One more threadlocal is just an extra we don't need. Why can't we get the IP simply where we are saving the command? ########## fineract-core/src/main/java/org/apache/fineract/infrastructure/core/filters/GeolocationHeaderFilter.java: ########## @@ -0,0 +1,93 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you 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 + * + * http://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.fineract.infrastructure.core.filters; + +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import java.io.IOException; +import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; +import org.apache.commons.lang3.StringUtils; +import org.apache.fineract.infrastructure.core.config.FineractProperties; +import org.springframework.web.filter.OncePerRequestFilter; + +@RequiredArgsConstructor +@Slf4j +public class GeolocationHeaderFilter extends OncePerRequestFilter { + + private final FineractProperties fineractProperties; + + private static final String[] IP_HEADER_CANDIDATES = { "X-Forwarded-For", "Proxy-Client-IP", "WL-Proxy-Client-IP", Review Comment: big no. This must be explained one by one why a specific header is used. Please put the comments in the code so everybody understands the purpose. ########## fineract-core/src/main/java/org/apache/fineract/commands/domain/CommandSource.java: ########## @@ -162,6 +166,7 @@ public static CommandSource fullEntryFrom(final CommandWrapper wrapper, final Js .transactionId(command.getTransactionId()) // .creditBureauId(command.getCreditBureauId()) // .organisationCreditBureauId(command.getOrganisationCreditBureauId()) // + .clientIp(ClientIpHolder.getClientIp()) // Review Comment: Woah, this is really not good. ########## fineract-provider/src/test/resources/application-test.properties: ########## @@ -20,6 +20,8 @@ fineract.node-id=1 fineract.security.basicauth.enabled=true +fineract.geolocation.enabled=false +fineract.security.two-factor.enabled=false Review Comment: why the two-factor.enabled change? -- 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]
