You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@shiro.apache.org by "vasumakwana (via GitHub)" <gi...@apache.org> on 2023/04/03 00:18:12 UTC
[GitHub] [shiro] vasumakwana opened a new pull request, #824: Refactor
vasumakwana opened a new pull request, #824:
URL: https://github.com/apache/shiro/pull/824
### 1. Refactoring name: Extract method.
Location: shiro\web\src\test\java\org\apache\shiro\web\servlet\SimpleCookieTest.java\Line no. 169
Explanation: The method was extracted to make the code easy to understand and the method indicates its purpose clearly.
### 2. Refactoring name: Rename method.
Location: shiro\web\src\test\java\org\apache\shiro\web\filter\mgt\SimpleNamedFilterListTest.java\Line no. 52
Explanation: The original method name was not descriptive enough and did not follow the standard naming convention for test methods. The new name clearly indicates what the test is checking for.
Location: shiro\web\src\test\java\org\apache\shiro\web\RestoreSystemProperties.java\Line no. 48
Explanation: The new method name is more descriptive and follows the standard naming convention for method names.
### 3. Refactoring name: Decompose conditional.
Location: shiro\samples\spring-hibernate\src\main\java\org\apache\shiro\samples\sprhib\web\SignupValidator.java\Line no. 45
Explanation: To decompose the complicated parts of the conditional into separate methods, I have create two methods:
1. isEmailInvalid() - This method will take in the email address as a parameter and return a boolean value indicating whether the email is invalid or not based on the provided regular expression.
2. hasInvalidEmail() - This method will take in the command object as a parameter and call the getEmail() method to retrieve the email address. It will then use the isEmailInvalid() method to check if the email is invalid or not and return a boolean value accordingly.
### 4. Refactoring name: Extract Class
Location: shiro\web\src\main\java\org\apache\shiro\web\filter\authc\HttpMethodsExtractor.java and shiro\web\src\main\java\org\apache\shiro\web\filter\authc\HttpAuthenticationFilter.java\Line no. 205
Explanation: I have extracted the functionality for extracting HTTP methods from options into a new class called HttpMethodsExtractor. This class has a single public method extractHttpMethodsFromOptions that takes an array of options and returns a set of HTTP methods. The private method isHttpMethod is used internally by the extractHttpMethodsFromOptions method to check if an option is a valid HTTP method. In the HttpAuthenticationFilter class, I have created an instance of the HttpMethodsExtractor class and used it to extract HTTP methods from options in the
httpMethodsFromOptions method. This makes the code more modular and easier to maintain, as the functionality for extracting HTTP methods is now encapsulated in a separate class.
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] vasumakwana commented on a diff in pull request #824: Refactor
Posted by "vasumakwana (via GitHub)" <gi...@apache.org>.
vasumakwana commented on code in PR #824:
URL: https://github.com/apache/shiro/pull/824#discussion_r1160322009
##########
web/src/main/java/org/apache/shiro/web/filter/authc/FormAuthenticationFilter.java:
##########
@@ -1,227 +0,0 @@
-/*
Review Comment:
Can you help me why the license check is failing in the third pipeline?
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] vasumakwana commented on a diff in pull request #824: Refactor
Posted by "vasumakwana (via GitHub)" <gi...@apache.org>.
vasumakwana commented on code in PR #824:
URL: https://github.com/apache/shiro/pull/824#discussion_r1160162737
##########
web/src/main/java/org/apache/shiro/web/filter/authc/FormAuthenticationFilter.java:
##########
@@ -1,227 +0,0 @@
-/*
Review Comment:
Sorry, I am actually new to the open-source contribution, I am really excited to contribute to open-source projects starting with Apache Shiro.
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] vasumakwana commented on a diff in pull request #824: Refactor
Posted by "vasumakwana (via GitHub)" <gi...@apache.org>.
vasumakwana commented on code in PR #824:
URL: https://github.com/apache/shiro/pull/824#discussion_r1155493227
##########
web/src/main/java/org/apache/shiro/web/filter/authc/HttpMethodsExtractor.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.shiro.web.filter.authc;
+
+import java.util.HashSet;
+import java.util.Locale;
+import java.util.Set;
+
+public class HttpMethodsExtractor {
+
+ public static final String PERMISSIVE = "permissive";
+
+ /**
+ * HTTP Authorization header, equal to <code>Authorization</code>
+ */
+ protected static final String AUTHORIZATION_HEADER = "Authorization";
Review Comment:
I accidentally put that in there, even though I wasn't using those variables. So, I deleted those.
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lprimak commented on a diff in pull request #824: Refactor
Posted by "lprimak (via GitHub)" <gi...@apache.org>.
lprimak commented on code in PR #824:
URL: https://github.com/apache/shiro/pull/824#discussion_r1160195994
##########
web/src/main/java/org/apache/shiro/web/filter/authc/FormAuthenticationFilter.java:
##########
@@ -1,227 +0,0 @@
-/*
Review Comment:
Thanks, that's great!
Please don't make individual PRs for changes. The first PR was *almost* good to go until you decided to put in some "controversial" changes last minute :)
Just group into coherent units
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] vasumakwana commented on pull request #824: Refactor
Posted by "vasumakwana (via GitHub)" <gi...@apache.org>.
vasumakwana commented on PR #824:
URL: https://github.com/apache/shiro/pull/824#issuecomment-1495082692
Changed all the comments and code indentation from 80-character columns to 130-character columns.
Thank you!
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lprimak commented on a diff in pull request #824: Refactor
Posted by "lprimak (via GitHub)" <gi...@apache.org>.
lprimak commented on code in PR #824:
URL: https://github.com/apache/shiro/pull/824#discussion_r1160384599
##########
web/src/main/java/org/apache/shiro/web/filter/authc/FormAuthenticationFilter.java:
##########
@@ -1,227 +0,0 @@
-/*
Review Comment:
Let's take care of the comments first
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lprimak commented on a diff in pull request #824: Refactor
Posted by "lprimak (via GitHub)" <gi...@apache.org>.
lprimak commented on code in PR #824:
URL: https://github.com/apache/shiro/pull/824#discussion_r1160060823
##########
crypto/support/hashes/argon2/src/main/java/org/apache/shiro/crypto/support/hashes/argon2/Argon2Hash.java:
##########
@@ -209,17 +209,21 @@ public static Argon2Hash generate(
int outputLengthBits
) {
final int type;
+ Argon2Algorithm algorithm;
Review Comment:
Sorry, this refactoring brings no value and makes the code less readable
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lprimak commented on a diff in pull request #824: Refactor
Posted by "lprimak (via GitHub)" <gi...@apache.org>.
lprimak commented on code in PR #824:
URL: https://github.com/apache/shiro/pull/824#discussion_r1155490533
##########
web/src/main/java/org/apache/shiro/web/filter/authc/HttpMethodsExtractor.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.shiro.web.filter.authc;
+
+import java.util.HashSet;
+import java.util.Locale;
+import java.util.Set;
+
+public class HttpMethodsExtractor {
+
+ public static final String PERMISSIVE = "permissive";
+
+ /**
+ * HTTP Authorization header, equal to <code>Authorization</code>
+ */
+ protected static final String AUTHORIZATION_HEADER = "Authorization";
Review Comment:
no, just make the original declaration in `HttpAuthenticationFilter` package-private,
import / use it, don't duplicate it.
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] vasumakwana commented on a diff in pull request #824: Refactor
Posted by "vasumakwana (via GitHub)" <gi...@apache.org>.
vasumakwana commented on code in PR #824:
URL: https://github.com/apache/shiro/pull/824#discussion_r1157647760
##########
web/src/main/java/org/apache/shiro/web/filter/authc/FormAuthenticationFilter.java:
##########
@@ -30,6 +31,39 @@
import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
+/**
+
+ * The LoginUtils class provides utility methods for handling login functionality in a web application.
+ * It contains a method for detecting whether a login submission has been made via an HTTP POST request,
+ * which can be overridden by subclasses to provide custom behavior for detecting login submissions.
+ * <p>
+ * Subclasses can modify this behavior to detect login submissions via an HTTP PUT request or other HTTP methods.
+ * </p>
+ * <p>
+ * This class is designed to work together with the AccessControlFilter, which enforces access control policies in a web application.
+ * The AccessControlFilter can use the isLoginSubmission() method to determine whether a request is a login submission
+ * and should be handled differently from other requests.
+ * </p>
+ * <p>
+ * This class is not intended to be instantiated or subclassed, as all of its methods are static and protected.
+ * </p>
+ * @since 0.9
+ */
+class LoginUtils {
+ /**
Review Comment:
By moving the isLoginSubmission method to a separate class, we have made the code more modular and improved the cohesion of the classes. Additionally, any other classes that need to check if a request is a login submission can now use the LoginUtils class, rather than duplicating the logic in their own code. I think this could make the code more readable.
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] vasumakwana commented on pull request #824: Refactor
Posted by "vasumakwana (via GitHub)" <gi...@apache.org>.
vasumakwana commented on PR #824:
URL: https://github.com/apache/shiro/pull/824#issuecomment-1499853673
can you tell me why the license check is failing in the third pipeline? please help me, I will revert back the changes that you want.
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] vasumakwana commented on a diff in pull request #824: Refactor
Posted by "vasumakwana (via GitHub)" <gi...@apache.org>.
vasumakwana commented on code in PR #824:
URL: https://github.com/apache/shiro/pull/824#discussion_r1160143542
##########
crypto/support/hashes/argon2/src/main/java/org/apache/shiro/crypto/support/hashes/argon2/Argon2Hash.java:
##########
@@ -209,17 +209,21 @@ public static Argon2Hash generate(
int outputLengthBits
) {
final int type;
+ Argon2Algorithm algorithm;
Review Comment:
I am thinking of creating a new PR with small changes so it will be easier for you to review the changes and merge them into the main. Do you think I should do that?
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lprimak commented on a diff in pull request #824: Refactor
Posted by "lprimak (via GitHub)" <gi...@apache.org>.
lprimak commented on code in PR #824:
URL: https://github.com/apache/shiro/pull/824#discussion_r1160140878
##########
web/src/main/java/org/apache/shiro/web/filter/authc/FormAuthenticationFilter.java:
##########
@@ -1,227 +0,0 @@
-/*
Review Comment:
This file should be deleted
##########
crypto/support/hashes/argon2/src/main/java/org/apache/shiro/crypto/support/hashes/argon2/Argon2Hash.java:
##########
@@ -1,371 +0,0 @@
-/*
Review Comment:
This file should not be deleted
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] vasumakwana commented on pull request #824: Refactor
Posted by "vasumakwana (via GitHub)" <gi...@apache.org>.
vasumakwana commented on PR #824:
URL: https://github.com/apache/shiro/pull/824#issuecomment-1499236759
Can you help me why the build failed on Mac os version?
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] vasumakwana commented on a diff in pull request #824: Refactor
Posted by "vasumakwana (via GitHub)" <gi...@apache.org>.
vasumakwana commented on code in PR #824:
URL: https://github.com/apache/shiro/pull/824#discussion_r1160155116
##########
web/src/main/java/org/apache/shiro/web/filter/authc/FormAuthenticationFilter.java:
##########
@@ -1,227 +0,0 @@
-/*
Review Comment:
can I make individual PRs for each changes, so that it will be easy for you and me to track the changes?
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] vasumakwana commented on a diff in pull request #824: Refactor
Posted by "vasumakwana (via GitHub)" <gi...@apache.org>.
vasumakwana commented on code in PR #824:
URL: https://github.com/apache/shiro/pull/824#discussion_r1155488892
##########
web/src/main/java/org/apache/shiro/web/filter/authc/HttpMethodsExtractor.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.shiro.web.filter.authc;
+
+import java.util.HashSet;
+import java.util.Locale;
+import java.util.Set;
+
+public class HttpMethodsExtractor {
+
+ public static final String PERMISSIVE = "permissive";
+
+ /**
+ * HTTP Authorization header, equal to <code>Authorization</code>
+ */
+ protected static final String AUTHORIZATION_HEADER = "Authorization";
Review Comment:
private static final String AUTHORIZATION_HEADER = HttpAuthenticationFilter.AUTHORIZATION_HEADER;
did you mean like this?
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lprimak commented on pull request #824: Refactor
Posted by "lprimak (via GitHub)" <gi...@apache.org>.
lprimak commented on PR #824:
URL: https://github.com/apache/shiro/pull/824#issuecomment-1494763752
Why did you reformat all the comments? Please undo that one and squash the commits.
Coding style is for 130-character columns, not 80
Thank you!
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] vasumakwana commented on pull request #824: Refactor
Posted by "vasumakwana (via GitHub)" <gi...@apache.org>.
vasumakwana commented on PR #824:
URL: https://github.com/apache/shiro/pull/824#issuecomment-1495075649
Changed all the comments from 80-character columns to 130-character columns.
Thank you!
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lprimak commented on a diff in pull request #824: Refactor
Posted by "lprimak (via GitHub)" <gi...@apache.org>.
lprimak commented on code in PR #824:
URL: https://github.com/apache/shiro/pull/824#discussion_r1157659605
##########
web/src/main/java/org/apache/shiro/web/filter/authc/FormAuthenticationFilter.java:
##########
@@ -30,6 +31,39 @@
import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
+/**
+
+ * The LoginUtils class provides utility methods for handling login functionality in a web application.
+ * It contains a method for detecting whether a login submission has been made via an HTTP POST request,
+ * which can be overridden by subclasses to provide custom behavior for detecting login submissions.
+ * <p>
+ * Subclasses can modify this behavior to detect login submissions via an HTTP PUT request or other HTTP methods.
+ * </p>
+ * <p>
+ * This class is designed to work together with the AccessControlFilter, which enforces access control policies in a web application.
+ * The AccessControlFilter can use the isLoginSubmission() method to determine whether a request is a login submission
+ * and should be handled differently from other requests.
+ * </p>
+ * <p>
+ * This class is not intended to be instantiated or subclassed, as all of its methods are static and protected.
+ * </p>
+ * @since 0.9
+ */
+class LoginUtils {
+ /**
Review Comment:
Sorry I disagree. Classic case of over engineering
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] github-actions[bot] closed pull request #824: Refactor
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #824: Refactor
URL: https://github.com/apache/shiro/pull/824
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lprimak commented on a diff in pull request #824: Refactor
Posted by "lprimak (via GitHub)" <gi...@apache.org>.
lprimak commented on code in PR #824:
URL: https://github.com/apache/shiro/pull/824#discussion_r1160195994
##########
web/src/main/java/org/apache/shiro/web/filter/authc/FormAuthenticationFilter.java:
##########
@@ -1,227 +0,0 @@
-/*
Review Comment:
Thanks, that's great!
Please don't make individual PRs for changes. The first PR was *almost* good to go until you decided to put in some "controversial" changes last minute :)
##########
web/src/main/java/org/apache/shiro/web/filter/authc/FormAuthenticationFilter.java:
##########
@@ -1,227 +0,0 @@
-/*
Review Comment:
This file should not be deleted
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lprimak commented on pull request #824: Refactor
Posted by "lprimak (via GitHub)" <gi...@apache.org>.
lprimak commented on PR #824:
URL: https://github.com/apache/shiro/pull/824#issuecomment-1498305754
Got your CLA confirmation. Thank you
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lprimak commented on a diff in pull request #824: Refactor
Posted by "lprimak (via GitHub)" <gi...@apache.org>.
lprimak commented on code in PR #824:
URL: https://github.com/apache/shiro/pull/824#discussion_r1160381334
##########
crypto/support/hashes/argon2/src/main/java/org/apache/shiro/crypto/support/hashes/argon2/Argon2Hash.java:
##########
@@ -209,17 +209,21 @@ public static Argon2Hash generate(
int outputLengthBits
) {
final int type;
+ Argon2Algorithm algorithm;
Review Comment:
No. Just please reflect my comments in your current PR.
You are spamming Shiro with multiple PRs closing and opening constantly.
##########
web/src/main/java/org/apache/shiro/web/filter/authc/FormAuthenticationFilter.java:
##########
@@ -30,6 +31,39 @@
import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
+/**
+
+ * The LoginUtils class provides utility methods for handling login functionality in a web application.
+ * It contains a method for detecting whether a login submission has been made via an HTTP POST request,
+ * which can be overridden by subclasses to provide custom behavior for detecting login submissions.
+ * <p>
+ * Subclasses can modify this behavior to detect login submissions via an HTTP PUT request or other HTTP methods.
+ * </p>
+ * <p>
+ * This class is designed to work together with the AccessControlFilter, which enforces access control policies in a web application.
+ * The AccessControlFilter can use the isLoginSubmission() method to determine whether a request is a login submission
+ * and should be handled differently from other requests.
+ * </p>
+ * <p>
+ * This class is not intended to be instantiated or subclassed, as all of its methods are static and protected.
+ * </p>
+ * @since 0.9
+ */
+class LoginUtils {
+ /**
Review Comment:
Please revert this refactor
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lprimak commented on a diff in pull request #824: Refactor
Posted by "lprimak (via GitHub)" <gi...@apache.org>.
lprimak commented on code in PR #824:
URL: https://github.com/apache/shiro/pull/824#discussion_r1155475587
##########
web/src/main/java/org/apache/shiro/web/filter/authc/HttpMethodsExtractor.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.shiro.web.filter.authc;
+
+import java.util.HashSet;
+import java.util.Locale;
+import java.util.Set;
+
+public class HttpMethodsExtractor {
+
+ public static final String PERMISSIVE = "permissive";
Review Comment:
Remove this, can use `AuthenticatingFilter`.PERMISSIVE
##########
web/src/main/java/org/apache/shiro/web/filter/authc/HttpMethodsExtractor.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.shiro.web.filter.authc;
+
+import java.util.HashSet;
+import java.util.Locale;
+import java.util.Set;
+
+public class HttpMethodsExtractor {
+
+ public static final String PERMISSIVE = "permissive";
+
+ /**
+ * HTTP Authorization header, equal to <code>Authorization</code>
+ */
+ protected static final String AUTHORIZATION_HEADER = "Authorization";
+
+ /**
+ * HTTP Authentication header, equal to <code>WWW-Authenticate</code>
+ */
+ protected static final String AUTHENTICATE_HEADER = "WWW-Authenticate";
+
+ public Set<String> extractHttpMethodsFromOptions(String[] options) {
+ Set<String> methods = new HashSet<String>();
+
+ if (options != null) {
+ for (String option : options) {
+ if (isHttpMethod(option)) {
+ methods.add(option.toUpperCase(Locale.ENGLISH));
+ }
+ }
+ }
+ return methods;
+ }
+
+ private boolean isHttpMethod(String option) {
Review Comment:
Make static
##########
web/src/main/java/org/apache/shiro/web/filter/authc/HttpMethodsExtractor.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.shiro.web.filter.authc;
+
+import java.util.HashSet;
+import java.util.Locale;
+import java.util.Set;
+
+public class HttpMethodsExtractor {
+
+ public static final String PERMISSIVE = "permissive";
+
+ /**
+ * HTTP Authorization header, equal to <code>Authorization</code>
+ */
+ protected static final String AUTHORIZATION_HEADER = "Authorization";
Review Comment:
Remove this. Make `HttpAuthenticationFilter`.AUTHORIZATION_HEADER package-private and use that
##########
web/src/main/java/org/apache/shiro/web/filter/authc/HttpMethodsExtractor.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.shiro.web.filter.authc;
+
+import java.util.HashSet;
+import java.util.Locale;
+import java.util.Set;
+
+public class HttpMethodsExtractor {
+
+ public static final String PERMISSIVE = "permissive";
+
+ /**
+ * HTTP Authorization header, equal to <code>Authorization</code>
+ */
+ protected static final String AUTHORIZATION_HEADER = "Authorization";
+
+ /**
+ * HTTP Authentication header, equal to <code>WWW-Authenticate</code>
+ */
+ protected static final String AUTHENTICATE_HEADER = "WWW-Authenticate";
+
+ public Set<String> extractHttpMethodsFromOptions(String[] options) {
Review Comment:
Make static
##########
web/src/main/java/org/apache/shiro/web/filter/authc/HttpMethodsExtractor.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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.shiro.web.filter.authc;
+
+import java.util.HashSet;
+import java.util.Locale;
+import java.util.Set;
+
+public class HttpMethodsExtractor {
+
+ public static final String PERMISSIVE = "permissive";
+
+ /**
+ * HTTP Authorization header, equal to <code>Authorization</code>
+ */
+ protected static final String AUTHORIZATION_HEADER = "Authorization";
+
+ /**
+ * HTTP Authentication header, equal to <code>WWW-Authenticate</code>
+ */
+ protected static final String AUTHENTICATE_HEADER = "WWW-Authenticate";
Review Comment:
Remove. Same as above
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lprimak commented on a diff in pull request #824: Refactor
Posted by "lprimak (via GitHub)" <gi...@apache.org>.
lprimak commented on code in PR #824:
URL: https://github.com/apache/shiro/pull/824#discussion_r1156268199
##########
web/src/main/java/org/apache/shiro/web/filter/authc/HttpAuthenticationFilter.java:
##########
@@ -202,19 +202,10 @@ protected boolean isAccessAllowed(ServletRequest request, ServletResponse respon
}
}
+ private HttpMethodsExtractor httpMethodsExtractor = new HttpMethodsExtractor();
Review Comment:
No need for this since all the methods are static
##########
web/src/main/java/org/apache/shiro/web/filter/authc/HttpAuthenticationFilter.java:
##########
@@ -202,19 +202,10 @@ protected boolean isAccessAllowed(ServletRequest request, ServletResponse respon
}
}
+ private HttpMethodsExtractor httpMethodsExtractor = new HttpMethodsExtractor();
+
private Set<String> httpMethodsFromOptions(String[] options) {
- Set<String> methods = new HashSet<String>();
-
- if (options != null) {
- for (String option : options) {
- // to be backwards compatible with 1.3, we can ONLY check for known args
- // ideally we would just validate HTTP methods, but someone could already be using this for webdav
- if (!option.equalsIgnoreCase(PERMISSIVE)) {
- methods.add(option.toUpperCase(Locale.ENGLISH));
- }
- }
- }
- return methods;
+ return httpMethodsExtractor.extractHttpMethodsFromOptions(options);
Review Comment:
Call static methods directly as opposed via instance (which is not needed)
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] vasumakwana commented on a diff in pull request #824: Refactor
Posted by "vasumakwana (via GitHub)" <gi...@apache.org>.
vasumakwana commented on code in PR #824:
URL: https://github.com/apache/shiro/pull/824#discussion_r1160112114
##########
crypto/support/hashes/argon2/src/main/java/org/apache/shiro/crypto/support/hashes/argon2/Argon2Hash.java:
##########
@@ -209,17 +209,21 @@ public static Argon2Hash generate(
int outputLengthBits
) {
final int type;
+ Argon2Algorithm algorithm;
Review Comment:
Okay, I will revert the changes.
Sorry for the inconvenience caused.
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lprimak commented on a diff in pull request #824: Refactor
Posted by "lprimak (via GitHub)" <gi...@apache.org>.
lprimak commented on code in PR #824:
URL: https://github.com/apache/shiro/pull/824#discussion_r1156721944
##########
web/src/main/java/org/apache/shiro/web/filter/authc/FormAuthenticationFilter.java:
##########
@@ -30,6 +31,39 @@
import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
+/**
+
+ * The LoginUtils class provides utility methods for handling login functionality in a web application.
+ * It contains a method for detecting whether a login submission has been made via an HTTP POST request,
+ * which can be overridden by subclasses to provide custom behavior for detecting login submissions.
+ * <p>
+ * Subclasses can modify this behavior to detect login submissions via an HTTP PUT request or other HTTP methods.
+ * </p>
+ * <p>
+ * This class is designed to work together with the AccessControlFilter, which enforces access control policies in a web application.
+ * The AccessControlFilter can use the isLoginSubmission() method to determine whether a request is a login submission
+ * and should be handled differently from other requests.
+ * </p>
+ * <p>
+ * This class is not intended to be instantiated or subclassed, as all of its methods are static and protected.
+ * </p>
+ * @since 0.9
+ */
+class LoginUtils {
+ /**
Review Comment:
Why was this introduced? It's not really deserving of a complete class, especially non-static one
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] lprimak commented on a diff in pull request #824: Refactor
Posted by "lprimak (via GitHub)" <gi...@apache.org>.
lprimak commented on code in PR #824:
URL: https://github.com/apache/shiro/pull/824#discussion_r1156721944
##########
web/src/main/java/org/apache/shiro/web/filter/authc/FormAuthenticationFilter.java:
##########
@@ -30,6 +31,39 @@
import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletRequest;
+/**
+
+ * The LoginUtils class provides utility methods for handling login functionality in a web application.
+ * It contains a method for detecting whether a login submission has been made via an HTTP POST request,
+ * which can be overridden by subclasses to provide custom behavior for detecting login submissions.
+ * <p>
+ * Subclasses can modify this behavior to detect login submissions via an HTTP PUT request or other HTTP methods.
+ * </p>
+ * <p>
+ * This class is designed to work together with the AccessControlFilter, which enforces access control policies in a web application.
+ * The AccessControlFilter can use the isLoginSubmission() method to determine whether a request is a login submission
+ * and should be handled differently from other requests.
+ * </p>
+ * <p>
+ * This class is not intended to be instantiated or subclassed, as all of its methods are static and protected.
+ * </p>
+ * @since 0.9
+ */
+class LoginUtils {
+ /**
Review Comment:
Why was this introduced? It's not really deserving of a complete class
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] vasumakwana commented on pull request #824: Refactor
Posted by "vasumakwana (via GitHub)" <gi...@apache.org>.
vasumakwana commented on PR #824:
URL: https://github.com/apache/shiro/pull/824#issuecomment-1499789116
Can you help me why the license check is failing in the third pipeline?
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] vasumakwana commented on pull request #824: Refactor
Posted by "vasumakwana (via GitHub)" <gi...@apache.org>.
vasumakwana commented on PR #824:
URL: https://github.com/apache/shiro/pull/824#issuecomment-1493698227
Please review the changes I have made as per your request and I have also signed CLA as per your request.
--
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: commits-unsubscribe@shiro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org