-
Notifications
You must be signed in to change notification settings - Fork 5.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8215916: The failure reason of an optional JAAS LoginModule is not logged #9159
Conversation
Hi @jhuttana, welcome to this OpenJDK project and thanks for contributing! We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user jhuttana" as summary for the issue. If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing |
/covered |
Thank you! Please allow for a few business days to verify that your employer has signed the OCA. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated! |
… optional JAAS LoginModule
Webrevs
|
ite.printStackTrace | ||
(new java.io.PrintWriter(sw)); | ||
sw.flush(); | ||
le = new LoginException(sw.toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wangweij might have more to say, but I think you just want to dump this information using debug.println
if debug is enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the same suggestion as Sean. In JAAS, login could succeed even if one optional LoginModule failed, and in this case the reason for that failure is lost (even with your current fix). Logging it somewhere might help developer understand why it happened.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks you both for taking a look at this PR.
I will investigate further for suitable suggested changes.
Could you please suggest me how I can quickly check whether the changes I made are reflecting properly as expected ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several builtin LoginModule
implementations inside OpenJDK. For example, you can configure both NTLoginModule
and UnixLoginModule
as OPTIONAL in your JAAS login configuration file. No matter if you run on Windows or Linux, one will succeed and one will fail but overall the login will succeed. You can set -Djava.security.debug=logincontext
to see if there is information on the failed one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if you like, try writing this as a regression test. You can call System.setErr
at the beginning to redirect the log messages to your own ByteArrayOutputStream
, and then after restoring the original System.err
, you can inspect the output to see if the expected log message is there. This is not necessary since the code change is not significant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wangweij Thank you I will try these.
I verified the changes with sample example here: https://web.mit.edu/java_v1.5.0_22/distrib/share/docs/guide/security/jaas/tutorials/GeneralAcnOnly.html , before and after the patch. Results are below:
After Patch:
After the patch it is printing the stack trace. |
It will be nice if the test itself can confirm the correctness since this is not a manual test (and we definitely want to avoid manual test as much as possible). Also, tests in OpenJDK are meant to be launched by the |
Ok. So the steps what I followed to confirm the patch can be converted to test case? |
Yes, or you can write an individual test. |
Looks like already the test cases which serve our purpose to test our patch are already here: https://github.com/openjdk/jdk/tree/master/test/jdk/javax/security/auth/login/LoginContext I just tried executing the below set of test cases ( with or without the patch the result is same as SmartLoginModule.java , to me looks like not executed)
|
Your new test needs to be a regression test, which means before the fix it fails and after the fix it succeeds. Also, seeing the exception message in a jtreg output with your own eyes is not a test. The test itself should detect the exception and decide whether it's a success or a failure. In the case, I'd recommend you use the |
You are right.
Okay! To me at present this looks like a big task to do ;) May be because I haven't written any such tests so far :) |
@jhuttana Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information. |
@wangweij I was trying to add a tets case(I can say its still primary version of the test case. Still there are modifications needed) At present I am using the example test from here: https://web.mit.edu/java_v1.5.0_22/distrib/share/docs/guide/security/jaas/tutorials/GeneralAcnOnly.html#RunAc When I am trying the below command, its failing with some exit status which I am unable to find why! :
I will also investigate further but if you notice something evident could you please let me know whats wrong here? |
There is a .jtr file inside the JTwork directory containing the detail output. |
There are quite some JAAS tests inside |
That makes sense :) I will try that. |
Yes. If you decide to print out
Of course not. It would be useful if the test fails one day. For example, you can try to modify the exception message on line 150 of |
} | ||
System.out.printf("-- call stack is -- %n%s%n", s); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Print before the exception is thrown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
Please update the copyright line of the test to "Copyright (c) 2022, Red Hat, Inc." since this is a new test. No other comment. Thanks for the patience. |
Done :) |
I've submitted the change to our test servers. Will approve the change once the job is finished. |
Sure thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved.
Some tiny comments (sorry for more):
- No need to import
Configuration
class now. - No need to import
Paths
class now, or, you can change it toPath
and simplify its reference on line 52.
No need for more code review for these changes.
@jhuttana This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 82 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@wangweij) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
Done. Thank you! |
"""; | ||
|
||
System.out.println("config is : \n"+config); | ||
Files.writeString(java.nio.file.Path.of("cross-platform"), config.toString()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to write full package name here then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thank you!
/integrate |
/sponsor |
Going to push as commit 3c2289d.
Your commit was automatically rebased without conflicts. |
Could you please review the changes?
This patch is to address : https://bugs.openjdk.org/browse/JDK-8215916?jql=labels%20%3D%20starter-bug
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9159/head:pull/9159
$ git checkout pull/9159
Update a local copy of the PR:
$ git checkout pull/9159
$ git pull https://git.openjdk.org/jdk pull/9159/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 9159
View PR using the GUI difftool:
$ git pr show -t 9159
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9159.diff