Skip to content

Refactoring in security (TLS) - #25056

Merged
dmatej merged 14 commits into
eclipse-ee4j:masterfrom
dmatej:tls
Jul 22, 2024
Merged

Refactoring in security (TLS)#25056
dmatej merged 14 commits into
eclipse-ee4j:masterfrom
dmatej:tls

Conversation

@dmatej

@dmatej dmatej commented Jul 19, 2024

Copy link
Copy Markdown
Contributor
  • Simplified
  • Fixed many NPEs and swallowed exceptions
  • Fixed HK2 related issues (missing @Contract, inaccessible classes in a context)
  • Narrowed dependencies (a bit)
  • I believe it is not worse than before :-) , but the progress is very slow and painful. Usual mistakes:
    • Typo in reflection. GlassFish vs. Glassfish took 3 hours.
    • Missing @Contract was the cause of all that original boilerplate code
    • Swallowed exceptions -> misleading logs reporting "something broken" when it was already broken three steps before.
    • Added dependency just for one import = bad architecture, something should be on another place.
  • All tests passed on OmniFish's Jenkins including TCK

Review

  • Better per commit. All changes are related - usually I broke something then noticed the error is not logged or is shadowed by another error, so I had to fix that first. Finally I reached some "safe point", committed, and then I split that commit to several smaller commits to at least partially meaningful blocks of the work.

@dmatej
dmatej force-pushed the tls branch 4 times, most recently from 252d005 to 0756c8a Compare July 20, 2024 10:10
dmatej added 13 commits July 20, 2024 23:05
- It was referred just from two other constants which had to have the
  dependency on deployment-common
- JWS will be removed soon OR replaced by the Open Web Start

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- the sleep can be interrupted
- reproduced randomly on local machine
- shortened sleep time to 100 ms

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- Added missing logs (when some class was not available, it did not even log it)
- Removed e.printstacktrace when we already have logging or throw
- Added missing exception cause

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- When I broke some classes critical for startup, this was throwing NPE too

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- import order, wildchar imports removed
- final fields
- redundant modifiers, fixed order
- improved braces
- improved generics
- habitat renamed to locator
- using foreach were possible
- throwing exceptions with causes

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- Now it is injectable
- Simplified initialization of related classes

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- SSLImplementation is an interface and HK2 Contract, GlassfishSSLImpl
  implements it
- SecurityRoleMapperFactoryGen moved to core-ee where is the only usage of it
- SSLConfigurator - lookup removed; in all cases I have seen now works the
    locator variant, but I left the class loader variant here for now.
- JSSEImplementation was used just in tests
    -> moved there as JSSEImplementation4Tests
- SecureAdminConfigUpgrade - the comment was not true any more
- GlassfishServerSocketFactory now always uses locator

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
- Changes based on directly used packages in the module
- I will do that with maven plugin later again.

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
@dmatej
dmatej marked this pull request as ready for review July 20, 2024 23:42
@dmatej
dmatej requested review from a team, arjantijms, avpinchuk and pzygielo July 20, 2024 23:42
@dmatej dmatej self-assigned this Jul 20, 2024
@dmatej dmatej added this to the 7.0.16 milestone Jul 20, 2024

private final InetAddress mAddress;
private final ServiceLocator habitat;
private final ServiceLocator locator;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Absolutely tiny remark, and no need to change it, but I think "serviceLocator" as variable name would be more clear (various other locations use that too)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We use both, I prefer shorter version as it is clear in the context what it is.

Signed-off-by: David Matějček <david.matejcek@omnifish.ee>
@dmatej
dmatej requested a review from pzygielo July 22, 2024 11:29
@dmatej
dmatej merged commit 1dc7069 into eclipse-ee4j:master Jul 22, 2024
@dmatej
dmatej deleted the tls branch July 22, 2024 17:20
bvfalcon pushed a commit to bvfalcon/eclipse-glassfish that referenced this pull request Apr 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants