Re: RFR: 8215916: The failure reason of an optional JAAS LoginModule is not logged [v5]
On Mon, 15 Aug 2022 15:58:50 GMT, Weijun Wang wrote: > System.setErr Looks like call stack info of the exception is not produced for OPTIONAL login module when debug is on. Here : https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/javax/security/auth/login/LoginContext.java#L877 we don't dump call stack for exception thrown when manually the credentials are supplied. So we don't get what we are expecting in out output stream. But when we use Unix/NT Login module which uses gid and uid to verify the login then already the test case dumps the stack trace: https://github.com/openjdk/jdk/pull/9159/files#diff-455563a58230c5d5d6e24f73ab094d4d7572dc2c2771e3db70feb53c7d9b3a18R78 - PR: https://git.openjdk.org/jdk/pull/9159
Re: RFR: 8215916: The failure reason of an optional JAAS LoginModule is not logged [v5]
On Mon, 8 Aug 2022 17:34:59 GMT, Jayashree Huttanagoudar
wrote:
>> Could you please review the changes?
>> This patch is to address :
>> https://bugs.openjdk.org/browse/JDK-8215916?jql=labels%20%3D%20starter-bug
>
> Jayashree Huttanagoudar has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Added test case using {Unix,NT}LoginModule
I have modified the test case to include `System.setErr` and
`ByteArrayOutputStream`
-
PR: https://git.openjdk.org/jdk/pull/9159
Re: RFR: 8215916: The failure reason of an optional JAAS LoginModule is not logged [v6]
> Could you please review the changes? > This patch is to address : > https://bugs.openjdk.org/browse/JDK-8215916?jql=labels%20%3D%20starter-bug Jayashree Huttanagoudar has updated the pull request incrementally with one additional commit since the last revision: Address review comment for test case - Changes: - all: https://git.openjdk.org/jdk/pull/9159/files - new: https://git.openjdk.org/jdk/pull/9159/files/a7997f05..704cfefd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=9159&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=9159&range=04-05 Stats: 16 lines in 1 file changed: 10 ins; 5 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/9159.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9159/head:pull/9159 PR: https://git.openjdk.org/jdk/pull/9159
Re: RFR: 8215916: The failure reason of an optional JAAS LoginModule is not logged [v6]
On Tue, 16 Aug 2022 08:44:03 GMT, Jayashree Huttanagoudar wrote: >> Could you please review the changes? >> This patch is to address : >> https://bugs.openjdk.org/browse/JDK-8215916?jql=labels%20%3D%20starter-bug > > Jayashree Huttanagoudar has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comment for test case Great, the test looks fine. Now, let's come back to the bug fix itself about failure reason of an optional LoginModule. In the "cross-platform" test case, only "[LoginContext]: login OPTIONAL failure" is shown in the debug output. It will nice to include the stack trace of the exception there. (the same exception we observed in the "windows" case). - PR: https://git.openjdk.org/jdk/pull/9159
Re: RFR: 8215916: The failure reason of an optional JAAS LoginModule is not logged [v7]
> Could you please review the changes? > This patch is to address : > https://bugs.openjdk.org/browse/JDK-8215916?jql=labels%20%3D%20starter-bug Jayashree Huttanagoudar has updated the pull request incrementally with one additional commit since the last revision: Address review comment for cross-platform in the test case - Changes: - all: https://git.openjdk.org/jdk/pull/9159/files - new: https://git.openjdk.org/jdk/pull/9159/files/704cfefd..a4bf014f Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=9159&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=9159&range=05-06 Stats: 4 lines in 1 file changed: 0 ins; 1 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/9159.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9159/head:pull/9159 PR: https://git.openjdk.org/jdk/pull/9159
Re: RFR: 8215916: The failure reason of an optional JAAS LoginModule is not logged [v7]
On Tue, 16 Aug 2022 13:49:34 GMT, Jayashree Huttanagoudar wrote: >> Could you please review the changes? >> This patch is to address : >> https://bugs.openjdk.org/browse/JDK-8215916?jql=labels%20%3D%20starter-bug > > Jayashree Huttanagoudar has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comment for cross-platform in the test case Why the new change? The bug is about an optional LoginModule failure not logged, and it only happens when there are multiple login modules and the overall login succeeds. - PR: https://git.openjdk.org/jdk/pull/9159
Re: RFR: 8215916: The failure reason of an optional JAAS LoginModule is not logged [v7]
On Tue, 16 Aug 2022 14:17:43 GMT, Weijun Wang wrote: > Why the new change? The bug is about an optional LoginModule failure not > logged, and it only happens when there are multiple login modules and the > overall login succeeds. Hmm then I misunderstood your previous comment : >In the "cross-platform" test case, only "[LoginContext]: login OPTIONAL >failure" is shown in the debug output. It will nice to include the stack trace >of the exception there. (the same exception we observed in the "windows" case). To address above comment I have split the cross-platform piece of code in the test case. If the current change was not expected I think I didn't get it :( Could you please elaborate ? - PR: https://git.openjdk.org/jdk/pull/9159
Re: RFR: 8290775: Some doc errors in DerOutputStream.java [v5]
On Sat, 23 Jul 2022 05:37:16 GMT, jquanC wrote: >> Looks good to me. Thanks! > >> Looks good to me. Thanks! > > Cheers and appreciation! And I have learned more from your guidance. @jquanC Please type`/integrate` if you want to get the fix included in OpenJDK. - PR: https://git.openjdk.org/jdk/pull/9585
Re: RFR: 8215916: The failure reason of an optional JAAS LoginModule is not logged [v7]
On Tue, 16 Aug 2022 13:49:34 GMT, Jayashree Huttanagoudar wrote: >> Could you please review the changes? >> This patch is to address : >> https://bugs.openjdk.org/browse/JDK-8215916?jql=labels%20%3D%20starter-bug > > Jayashree Huttanagoudar has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comment for cross-platform in the test case The original "cross-platform" case is exactly what this bug is about. The overall login succeeds but the reason of the optional login module failure is not logged in the debug output. You need to update the source code of `LoginContext.java` to print out the reason and then confirm it in the test by examining the content of `new String(byes)`. Until now, you still only have a test change in your commits. This is not a test-only bug. - PR: https://git.openjdk.org/jdk/pull/9159
Re: RFR: 8215916: The failure reason of an optional JAAS LoginModule is not logged [v7]
On Tue, 16 Aug 2022 14:32:46 GMT, Weijun Wang wrote: > The original "cross-platform" case is exactly what this bug is about. The > overall login succeeds but the reason of the optional login module failure is > not logged in the debug output. You need to update the source code of > `LoginContext.java` to print out the reason and then confirm it in the test > by examining the content of `new String(byes)`. > > Until now, you still only have a test change in your commits. This is not a > test-only bug. Ok. But the change what I had for LoginContext.java in this commit was not appropriate : https://github.com/openjdk/jdk/pull/9159/commits/13a51a6d2e026225ec8ac9f1516c7f709c72ce08 right? - PR: https://git.openjdk.org/jdk/pull/9159
Re: RFR: 8290775: Some doc errors in DerOutputStream.java [v5]
On Sat, 23 Jul 2022 05:37:16 GMT, jquanC wrote: >> Looks good to me. Thanks! > >> Looks good to me. Thanks! > > Cheers and appreciation! And I have learned more from your guidance. > @jquanC Please type`/integrate` if you want to get the fix included in > OpenJDK. Thanks for your reminder! I just marked it, please check if it is correct. - PR: https://git.openjdk.org/jdk/pull/9585
Integrated: 8290775: Some doc errors in DerOutputStream.java
On Thu, 21 Jul 2022 08:53:31 GMT, jquanC wrote:
> There are some doc errors in sun.security.util.DerOutputStream, like the
> followings,
>
>
> /**
> * Private helper routine for writing DER encoded string values.
> * @param s the string to write
> * @param stringTag one of the DER string tags that indicate which
> * encoding should be used to write the string out.
> * @param enc the name of the encoder that should be used corresponding
> * to the above tag.
> */
> private void writeString(String s, byte stringTag, Charset charset) throws
> IOException
>
> The parameter is charset, but not enc.
>
>
> /**
> * Marshals a DER integer on the output stream.
> *
> * @param i the integer in bytes, equivalent to BigInteger::toByteArray.
> */
> public void putInteger(byte[] buf) throws IOException {
>
> The parameter is buf, but not i.
This pull request has now been integrated.
Changeset: 3e122419
Author:jquanC
Committer: Weijun Wang
URL:
https://git.openjdk.org/jdk/commit/3e122419b2979235f57c0dd549ca63647ea73753
Stats: 6 lines in 1 file changed: 0 ins; 0 del; 6 mod
8290775: Some doc errors in DerOutputStream.java
Reviewed-by: xuelei
-
PR: https://git.openjdk.org/jdk/pull/9585
Re: RFR: 8215916: The failure reason of an optional JAAS LoginModule is not logged [v7]
On Tue, 16 Aug 2022 13:49:34 GMT, Jayashree Huttanagoudar wrote: >> Could you please review the changes? >> This patch is to address : >> https://bugs.openjdk.org/browse/JDK-8215916?jql=labels%20%3D%20starter-bug > > Jayashree Huttanagoudar has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comment for cross-platform in the test case > Ok. But the change what I had for LoginContext.java in this commit was not > appropriate : > [13a51a6](https://github.com/openjdk/jdk/commit/13a51a6d2e026225ec8ac9f1516c7f709c72ce08) > right? No. If you only put the reason inside an exception but the overall login succeeds, this exception is dismissed and will not show up anywhere. You just need to add an extra `le.printStackTrace()` call next to the existing "[LoginContext]: login OPTIONAL failure" output. Then no matter if the overall login succeeds or not you can always find it in the debug output. - PR: https://git.openjdk.org/jdk/pull/9159
Re: RFR: 8215916: The failure reason of an optional JAAS LoginModule is not logged [v7]
On Tue, 16 Aug 2022 14:50:11 GMT, Weijun Wang wrote: > > Ok. But the change what I had for LoginContext.java in this commit was not > > appropriate : > > [13a51a6](https://github.com/openjdk/jdk/commit/13a51a6d2e026225ec8ac9f1516c7f709c72ce08) > > right? > > No. If you only put the reason inside an exception but the overall login > succeeds, this exception is dismissed and will not show up anywhere. > > You just need to add an extra `le.printStackTrace()` call next to the > existing "[LoginContext]: login OPTIONAL failure" output. Then no matter if > the overall login succeeds or not you can always find it in the debug output. That means in the source code here: https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/javax/security/auth/login/LoginContext.java#L881 ? - PR: https://git.openjdk.org/jdk/pull/9159
Re: RFR: 8215916: The failure reason of an optional JAAS LoginModule is not logged [v7]
On Tue, 16 Aug 2022 14:57:06 GMT, Jayashree Huttanagoudar wrote: > That means in the source code here: > https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/javax/security/auth/login/LoginContext.java#L881 > ? Yes. - PR: https://git.openjdk.org/jdk/pull/9159
Re: RFR: 8215916: The failure reason of an optional JAAS LoginModule is not logged [v8]
> Could you please review the changes? > This patch is to address : > https://bugs.openjdk.org/browse/JDK-8215916?jql=labels%20%3D%20starter-bug Jayashree Huttanagoudar has updated the pull request incrementally with one additional commit since the last revision: Address review comment to revert back the previous changes in the test case - Changes: - all: https://git.openjdk.org/jdk/pull/9159/files - new: https://git.openjdk.org/jdk/pull/9159/files/a4bf014f..0bcf670e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=9159&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=9159&range=06-07 Stats: 4 lines in 1 file changed: 1 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/9159.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9159/head:pull/9159 PR: https://git.openjdk.org/jdk/pull/9159
