Skip to content
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

Remove SecurityManager references from logging package #16 #117

Merged
merged 11 commits into from
Jan 23, 2024

Conversation

jmehrens
Copy link
Contributor

Signed-off-by: jmehrens [email protected]

@jmehrens
Copy link
Contributor Author

jmehrens commented Jan 14, 2024

Summary of changes:

  1. Removed the security manager test to verify methods throw when under the security manager and logging control is not granted. The only way to bring this test back is to create a test to run with custom log manager that overrides checkAccess. I don't think this is worth doing.
  2. Removed 2 tests related to verifying a memory optimization around super error manager. This test doesn't really have anything to do with correctness so the loss of coverage is not a big deal.
  3. Some of the existing SecurityManager test around the context class loader were updated to use a custom thread instead of a SecurityManager.
  4. New method and tests were added to optionally invoke the AccessController if is still present. This only happens if permissions fail when attempting an operation without privileges.
  5. Removed AccessController and SecurityManager from the demos
  6. Removed ThreadDeath checks during unwrap of exceptions. ThreadDeath is now deprecated.
  7. Replaced some of the argument checks with Objects.requireNonNull. I'll fix remaining ones in follow on tickets.

@jmehrens jmehrens requested a review from jbescos January 14, 2024 07:35
@jmehrens jmehrens self-assigned this Jan 14, 2024
@jmehrens jmehrens marked this pull request as draft January 14, 2024 14:44
@jmehrens jmehrens marked this pull request as ready for review January 14, 2024 21:40
@jmehrens
Copy link
Contributor Author

jmehrens commented Jan 15, 2024

Summary of build using /usr/lib/jvm/jdk-21-oracle-x64 to show that changes work:

Reactor Summary for Angus Mail Project 2.0.3-SNAPSHOT:

Angus Mail Project ................................. SUCCESS [ 45.369 s]
Angus Mail Core .................................... SUCCESS [ 27.830 s]
Angus Mail providers ............................... SUCCESS [ 0.114 s]
Angus Mail imap provider ........................... SUCCESS [ 7.682 s]
Angus Mail smtp provider ........................... SUCCESS [ 2.039 s]
Angus Mail pop3 provider ........................... SUCCESS [ 1.724 s]
Angus Mail logging handler ......................... SUCCESS [ 3.471 s]
Angus Mail Provider ................................ SUCCESS [04:15 min]
Angus Mail default provider ........................ SUCCESS [ 14.756 s]
Angus Mail dsn support ............................. SUCCESS [ 2.307 s]
Angus Mail Gmail IMAP provider ..................... SUCCESS [ 2.015 s]
Angus Mail mbox provider ........................... SUCCESS [ 33.610 s]
Jakarta Mail API demos ............................. SUCCESS [ 0.097 s]
Jakarta Mail API demo .............................. SUCCESS [ 3.050 s]
Jakarta Mail API demo client ....................... SUCCESS [ 1.169 s]
Jakarta Mail API demo servlet ...................... SUCCESS [ 2.496 s]
Jakarta Mail API demo taglib ....................... SUCCESS [ 2.627 s]
Jakarta Mail API demo webapp ....................... SUCCESS [ 1.548 s]
Jakarta Mail API logging demo ...................... SUCCESS [ 4.679 s]
Jakarta Mail API demo Outlook message support ...... SUCCESS [ 0.550 s]
Angus Mail documentation ........................... SUCCESS [ 19.608 s]

BUILD SUCCESS

Total time: 07:13 min

Copy link
Member

@lukasj lukasj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • MailHandler is missing cp year update
  • I prefer Objects.requireNonNull over if (x == null) throw new NPE (in LogManagerProperties) but that is a matter of taste

@jmehrens jmehrens marked this pull request as draft January 15, 2024 16:13
@jmehrens jmehrens marked this pull request as ready for review January 15, 2024 21:46
@jmehrens jmehrens requested a review from lukasj January 15, 2024 21:46
@jmehrens
Copy link
Contributor Author

@lukasj

  • MailHandler is missing cp year update

    • I prefer Objects.requireNonNull over if (x == null) throw new NPE (in LogManagerProperties) but that is a matter of taste

Fixed copywrite year. I updated the code to use requireNonNull. There is one case that is omitted because the argument is a Throwable which interestingly enough will trigger an IDE warning that return type of Throwable was not read. That method will be dealt with in a follow on ticket.

@jmehrens jmehrens marked this pull request as draft January 19, 2024 16:17
@jmehrens jmehrens marked this pull request as ready for review January 19, 2024 19:44
@lukasj
Copy link
Member

lukasj commented Jan 23, 2024

thanks!

@lukasj lukasj merged commit d223d1f into eclipse-ee4j:master Jan 23, 2024
4 checks passed
@jmehrens jmehrens linked an issue Mar 6, 2024 that may be closed by this pull request
@jmehrens jmehrens linked an issue Sep 5, 2024 that may be closed by this pull request
@jmehrens jmehrens mentioned this pull request Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove System.setSecurityManager from tests build test fail on jdk21
2 participants