On Fri, 1 Jul 2022 17:31:06 GMT, Weijun Wang <[email protected]> wrote:
> Add null-checks in all `LoginModule` implementations. It's possible that an
> application calls `logout` after a login failure, where most internal
> variables for principals and credentials are null and removing a null from
> the `Subject`'s principals and credentials sets will trigger a
> `NullPointerException`.
Changes requested by mullan (Reviewer).
src/java.base/share/classes/javax/security/auth/spi/LoginModule.java line 231:
> 229: * before removing it from the Principals or Credentials set
> 230: * of a {@code Subject}, otherwise a {@code
> NullPointerException} will
> 231: * be thrown. This is especially important when this method is
> called
I might also add: "... will be thrown as these sets prohibit null elements."
Even better, you could add a hyperlink "these sets prohibit null elements" to
the `Subject` constructor which states this.
Also, I recommend updating the `Subject` constructors and clarifying that
removing null elements also causes an NPE. Basically update the following
sentence (change in italics):
These Sets also prohibit null elements, and attempts to add, *query, or remove*
a null element will result in a NullPointerException.
src/java.base/share/classes/javax/security/auth/spi/LoginModule.java line 231:
> 229: * before removing it from the Principals or Credentials set
> 230: * of a {@code Subject}, otherwise a {@code
> NullPointerException} will
> 231: * be thrown. This is especially important when this method is
> called
s/when/if/
test/jdk/javax/security/auth/login/modules/SafeLogout.java line 37:
> 35: * @bug 8282730
> 36: * @key randomness
> 37: * @summary LdapLoginModule throw NPE from logout method after login
> failure
I think the summary can be more descriptive here and doesn't have to match the
bug description. How about "Check that all LoginModule implementations don't
throw NPE from logout method after login failure"
test/jdk/javax/security/auth/login/modules/SafeLogout.java line 51:
> 49:
> 50: static void test(int pos) throws Exception {
> 51: // Create random JAAS login config.
I'm probably missing something obvious, but can you explain why this test uses
a random login config? I would add some comments explaining that more.
-------------
PR: https://git.openjdk.org/jdk/pull/9348
