You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@shiro.apache.org by GitBox <gi...@apache.org> on 2021/03/21 21:28:32 UTC
[GitHub] [shiro] pitjazz opened a new pull request #288: Bugfix, fixed Scanner issue
pitjazz opened a new pull request #288:
URL: https://github.com/apache/shiro/pull/288
Following this checklist to help us incorporate your contribution quickly and easily:
- [ ] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/SHIRO) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
- [ ] Each commit in the pull request should have a meaningful subject line and body.
- [ ] Format the pull request title like `[SHIRO-XXX] - Fixes bug in SessionManager`,
where you replace `SHIRO-XXX` with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the commit message.
- [ ] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [ ] Run `mvn clean install apache-rat:check` to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
- [ ] If you have a group of commits related to the same change, please squash your commits into one and force push your branch using `git rebase -i`.
Trivial changes like typos do not require a JIRA issue (javadoc, comments...).
In this case, just format the pull request title like `(DOC) - Add javadoc in SessionManager`.
If this is your first contribution, you have to read the [Contribution Guidelines](https://github.com/apache/shiro/blob/master/CONTRIBUTING.md)
If your pull request is about ~20 lines of code you don't need to sign an [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf)
if you are unsure please ask on the developers list.
To make clear that you license your contribution under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [ ] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
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:
users@infra.apache.org
[GitHub] [shiro] bdemers commented on pull request #288: Bugfix, fixed Scanner issue
Posted by GitBox <gi...@apache.org>.
bdemers commented on pull request #288:
URL: https://github.com/apache/shiro/pull/288#issuecomment-817947036
Thanks @pitjazz
--
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:
users@infra.apache.org
[GitHub] [shiro] pitjazz commented on a change in pull request #288: Bugfix, fixed Scanner issue
Posted by GitBox <gi...@apache.org>.
pitjazz commented on a change in pull request #288:
URL: https://github.com/apache/shiro/pull/288#discussion_r602789211
##########
File path: core/src/main/java/org/apache/shiro/realm/text/TextConfigurationRealm.java
##########
@@ -211,9 +215,15 @@ protected void processUserDefinitions(Map<String, String> userDefs) {
protected static Set<String> toLines(String s) {
LinkedHashSet<String> set = new LinkedHashSet<String>();
- Scanner scanner = new Scanner(s);
- while (scanner.hasNextLine()) {
- set.add(scanner.nextLine());
+ try (Scanner scanner = new Scanner(s)) {
+ while (scanner.hasNextLine()) {
+ set.add(scanner.nextLine());
+ }
+ } catch (Exception e) {
+ if (log.isWarnEnabled()) {
+ String msg = "Unable to fetch next line, Scanner stream corrupted.";
+ log.warn(msg, e);
+ }
Review comment:
I removed the catch block, updated the pull request. What I want to do is just close the scanner and catch is not needed for that.
I also removed the unit test. There is no need to test how scanner reads the nextLine, because reading of users and roles are already tested in TextConfigurationRealmTest. Perhaps closing the scanner could be tested, but IMHO it's not relevant.
--
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:
users@infra.apache.org
[GitHub] [shiro] bmarwell commented on a change in pull request #288: Bugfix, fixed Scanner issue
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #288:
URL: https://github.com/apache/shiro/pull/288#discussion_r599715248
##########
File path: core/src/main/java/org/apache/shiro/realm/text/TextConfigurationRealm.java
##########
@@ -211,11 +212,14 @@ protected void processUserDefinitions(Map<String, String> userDefs) {
protected static Set<String> toLines(String s) {
LinkedHashSet<String> set = new LinkedHashSet<String>();
- Scanner scanner = new Scanner(s);
- while (scanner.hasNextLine()) {
- set.add(scanner.nextLine());
- }
- return set;
+ try (Scanner scanner = new Scanner(s)) {
+ while (scanner.hasNextLine()) {
+ set.add(scanner.nextLine());
+ }
+ } catch (NoSuchElementException e) {
+ e.printStackTrace();
+ }
+ return set;
Review comment:
> how was this the exception triggered?
> Link to Apache's SonarCloud: the bug.
yields: `Use try-with-resources or close this "Scanner" in a "finally" clause.`
That is, you would not want to catch the exception. Just the try would be fine in this case, as this is only about closing the scanner. But as we are reading a String, this is a noop anyway:
https://github.com/openjdk/jdk/blob/d7268fa3a68c2356548651a27b307d7f7158e700/src/java.base/share/classes/java/util/Scanner.java#L1171-L1175
So in this case, while Sonar is right in the sense of "good habit" (as the source could change): My -1 on this as we are not "fixing" anything or making anything better. Closing the scanner is fine for me, but there was no underlying bug and the API would also never be able to change to something `Closeable`.
--
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:
users@infra.apache.org
[GitHub] [shiro] bdemers commented on pull request #288: Bugfix, fixed Scanner issue
Posted by GitBox <gi...@apache.org>.
bdemers commented on pull request #288:
URL: https://github.com/apache/shiro/pull/288#issuecomment-805049078
@pitjazz actually, reopened the PR with a suggestion, as I _think_ that is what you are trying to resolve
--
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:
users@infra.apache.org
[GitHub] [shiro] bdemers merged pull request #288: Bugfix, fixed Scanner issue
Posted by GitBox <gi...@apache.org>.
bdemers merged pull request #288:
URL: https://github.com/apache/shiro/pull/288
--
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:
users@infra.apache.org
[GitHub] [shiro] pitjazz commented on pull request #288: Bugfix, fixed Scanner issue
Posted by GitBox <gi...@apache.org>.
pitjazz commented on pull request #288:
URL: https://github.com/apache/shiro/pull/288#issuecomment-809257832
> Code looks good now!
>
> Can you fix the commit message (some of it is no longer valid)
Commit message fixed.
--
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:
users@infra.apache.org
[GitHub] [shiro] bdemers commented on a change in pull request #288: Bugfix, fixed Scanner issue
Posted by GitBox <gi...@apache.org>.
bdemers commented on a change in pull request #288:
URL: https://github.com/apache/shiro/pull/288#discussion_r599736092
##########
File path: core/src/main/java/org/apache/shiro/realm/text/TextConfigurationRealm.java
##########
@@ -211,9 +215,15 @@ protected void processUserDefinitions(Map<String, String> userDefs) {
protected static Set<String> toLines(String s) {
LinkedHashSet<String> set = new LinkedHashSet<String>();
- Scanner scanner = new Scanner(s);
- while (scanner.hasNextLine()) {
- set.add(scanner.nextLine());
+ try (Scanner scanner = new Scanner(s)) {
+ while (scanner.hasNextLine()) {
+ set.add(scanner.nextLine());
+ }
+ } catch (Exception e) {
+ if (log.isWarnEnabled()) {
+ String msg = "Unable to fetch next line, Scanner stream corrupted.";
+ log.warn(msg, e);
+ }
Review comment:
```suggestion
}
```
I'm guessing just removing the catch block should remove the sonar error.
--
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:
users@infra.apache.org
[GitHub] [shiro] bdemers commented on a change in pull request #288: Bugfix, fixed Scanner issue
Posted by GitBox <gi...@apache.org>.
bdemers commented on a change in pull request #288:
URL: https://github.com/apache/shiro/pull/288#discussion_r599732138
##########
File path: core/src/main/java/org/apache/shiro/realm/text/TextConfigurationRealm.java
##########
@@ -211,9 +215,15 @@ protected void processUserDefinitions(Map<String, String> userDefs) {
protected static Set<String> toLines(String s) {
LinkedHashSet<String> set = new LinkedHashSet<String>();
- Scanner scanner = new Scanner(s);
- while (scanner.hasNextLine()) {
- set.add(scanner.nextLine());
+ try (Scanner scanner = new Scanner(s)) {
+ while (scanner.hasNextLine()) {
+ set.add(scanner.nextLine());
+ }
+ } catch (Exception e) {
+ if (log.isWarnEnabled()) {
+ String msg = "Unable to fetch next line, Scanner stream corrupted.";
+ log.warn(msg, e);
+ }
Review comment:
Agree, we want the original exception thrown here, failure to parse the config (especially config related to security) is a valid reason to throw an exception and resulting in a failure of the application to start.
A couple of other nits, as you learn more about Java:
you don't need to check `log.isWarnEnabled()` unless you are concatenating strings, or doing another expensive operation in that block, the log back `warn` method will do this check for you.
But like we mentioned, we don't want to swallow this exception
@pitjazz let's close this PR for now, if there is a listing issue you want to fix, we can continue the thread here and provide other suggestions if 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shiro] bdemers commented on a change in pull request #288: Bugfix, fixed Scanner issue
Posted by GitBox <gi...@apache.org>.
bdemers commented on a change in pull request #288:
URL: https://github.com/apache/shiro/pull/288#discussion_r599024538
##########
File path: core/src/main/java/org/apache/shiro/realm/text/TextConfigurationRealm.java
##########
@@ -211,11 +212,14 @@ protected void processUserDefinitions(Map<String, String> userDefs) {
protected static Set<String> toLines(String s) {
LinkedHashSet<String> set = new LinkedHashSet<String>();
- Scanner scanner = new Scanner(s);
- while (scanner.hasNextLine()) {
- set.add(scanner.nextLine());
- }
- return set;
+ try (Scanner scanner = new Scanner(s)) {
+ while (scanner.hasNextLine()) {
+ set.add(scanner.nextLine());
+ }
+ } catch (NoSuchElementException e) {
+ e.printStackTrace();
+ }
+ return set;
Review comment:
The rest of the file is formatted with spaces, this blog looks like tabs? can you fix that?
Instead of `e.printStackTrace()` use a logger.
More importantly though, how was this the exception triggered? I'm guessing if `hasNextLine()` returns true, and then `.nextLine()` throws a `NoSuchElementException`, that's not an error you would want to recover from, as the stream would have been corrupted? And in this case potentially worse because we are operating against a String in memory, which means things have gone severely wrong?
I could be miss understanding something here though, I only took a quick look.
--
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:
users@infra.apache.org
[GitHub] [shiro] bmarwell commented on a change in pull request #288: Bugfix, fixed Scanner issue
Posted by GitBox <gi...@apache.org>.
bmarwell commented on a change in pull request #288:
URL: https://github.com/apache/shiro/pull/288#discussion_r599716165
##########
File path: core/src/main/java/org/apache/shiro/realm/text/TextConfigurationRealm.java
##########
@@ -211,9 +215,15 @@ protected void processUserDefinitions(Map<String, String> userDefs) {
protected static Set<String> toLines(String s) {
LinkedHashSet<String> set = new LinkedHashSet<String>();
- Scanner scanner = new Scanner(s);
- while (scanner.hasNextLine()) {
- set.add(scanner.nextLine());
+ try (Scanner scanner = new Scanner(s)) {
+ while (scanner.hasNextLine()) {
+ set.add(scanner.nextLine());
+ }
+ } catch (Exception e) {
+ if (log.isWarnEnabled()) {
+ String msg = "Unable to fetch next line, Scanner stream corrupted.";
+ log.warn(msg, e);
+ }
Review comment:
Do not catch a generic exception. In this case, do not catch anything at all. We would want the Exception to promote here, otherwise the behaviour would change.
##########
File path: core/src/test/java/org/apache/shiro/realm/text/TextConfigurationRealmTest.java
##########
@@ -247,6 +249,21 @@ public void run() {
assertTrue("account doesn't exist when it should", realm.accountExists("user1"));
testThread.test();
}
+
+ /*
+ * Test that scanner reads next line
+ */
+ @Test
+ public void testScannerHasNextLine() {
+ realm = new TestRealm() {
+ public void test(Thread runnable) throws InterruptedException {
+ Scanner scanner = new Scanner("Scanner\nTest\n");
+ Assert.assertNotNull(scanner);
+ assertEquals("Scanner", scanner.nextLine());
+ assertEquals("Test", scanner.nextLine());
Review comment:
You are testing `java.util.Scanner`, not `TextConfigurationRealm` you modified.
--
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:
users@infra.apache.org
[GitHub] [shiro] pitjazz commented on a change in pull request #288: Bugfix, fixed Scanner issue
Posted by GitBox <gi...@apache.org>.
pitjazz commented on a change in pull request #288:
URL: https://github.com/apache/shiro/pull/288#discussion_r599032388
##########
File path: core/src/main/java/org/apache/shiro/realm/text/TextConfigurationRealm.java
##########
@@ -211,11 +212,14 @@ protected void processUserDefinitions(Map<String, String> userDefs) {
protected static Set<String> toLines(String s) {
LinkedHashSet<String> set = new LinkedHashSet<String>();
- Scanner scanner = new Scanner(s);
- while (scanner.hasNextLine()) {
- set.add(scanner.nextLine());
- }
- return set;
+ try (Scanner scanner = new Scanner(s)) {
+ while (scanner.hasNextLine()) {
+ set.add(scanner.nextLine());
+ }
+ } catch (NoSuchElementException e) {
+ e.printStackTrace();
+ }
+ return set;
Review comment:
Link to Apache's SonarCloud: [the bug](https://sonarcloud.io/project/issues?id=apache_shiro&languages=java&open=AXTvO9kn3S_U1xqKcObL&resolved=false&severities=BLOCKER&types=BUG).
--
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:
users@infra.apache.org
[GitHub] [shiro] bdemers closed pull request #288: Bugfix, fixed Scanner issue
Posted by GitBox <gi...@apache.org>.
bdemers closed pull request #288:
URL: https://github.com/apache/shiro/pull/288
--
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:
users@infra.apache.org