Skip to content

Conversation

@yersan
Copy link
Contributor

@yersan yersan commented Oct 4, 2022

…by using a different protection domain when the API is used with the security manager enabled

Complete the fix for #621

#621 was fixed by adding a new privilege block for the point where the iterator.next was being called, however, looking more closely at the different methods under FactoryFinder#find, there are other points in the path that can also fire SM checks, for example Thread.currentThread().getContextClassLoader(); or Class.forName(OSGI_SERVICE_LOADER_CLASS_NAME);

This PR moves the whole doPrivilegde one level above to the StreamProvider class which is indeed the one that explicitly loads the expected interface provided by the implementations.

The PR rollbacks the changes added in #622, the privileged block for getting the system property, and adds the check one level above.

@jbescos
Copy link
Member

jbescos commented Oct 5, 2022

Could you also add a new line here please?:
https://github.com/yersan/javamail/blob/1d500391b8e5358b7b7330d385f30cea03eccf08/doc/release/CHANGES.txt#L22

It should contain the next:
E 623 Add the ability to the API consumers to load the API implementations

The PR looks good to me.

@yersan
Copy link
Contributor Author

yersan commented Oct 5, 2022

Hi @jbescos , I've just added the line to CHANGES.txt, thanks for the review.

@jbescos
Copy link
Member

jbescos commented Oct 5, 2022

Thank you @yersan , there is only other comment about the copyright year in the code review. Could you check it so I can approve this, please?.

@yersan yersan force-pushed the secmgr branch 2 times, most recently from e40f094 to 72bc810 Compare October 5, 2022 09:39
@yersan
Copy link
Contributor Author

yersan commented Oct 5, 2022

@jbescos ahh, right, for the StreamProvider.java. Sorry, I forgot to change that one. In any case, I didn't see such a comment in the code review though, I'm probably not looking at the correct place. In any case, It should be fine now, thanks.

Copy link
Member

@jbescos jbescos left a comment

Choose a reason for hiding this comment

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

Thank you

public static StreamProvider provider() {
return FactoryFinder.find(StreamProvider.class);
}
public static StreamProvider provider() {
Copy link
Member

Choose a reason for hiding this comment

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

Could you update the copyright year of this file?. The first line should be:
Copyright (c) 2021, 2022 Oracle and/or its affiliates. All rights reserved.

Copy link
Contributor

@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.

use spaces instead of TABs, please

…by using a different protection domain when the API is used with security manager enabled
@yersan
Copy link
Contributor Author

yersan commented Oct 5, 2022

Hi @lukasj , done, notice the file in question has a mix of spaces vs tabs, I wasn't sure about the policy here. In general, the project has a mix of them in several places, a bulk update on all of them could be useful for the future, of course in a different issue

@lukasj
Copy link
Contributor

lukasj commented Oct 5, 2022

I agree usage of spaces/TABs is currently inconsistent in this project and we're trying to move to spaces to be consistent with most projects as we modify the code. If you want to bulk update it, feel free to prepare the PR for that. Thanks.

@lukasj lukasj self-requested a review October 5, 2022 14:05
Copy link
Contributor

@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.

LGTM

@lukasj lukasj merged commit 82dae3a into jakartaee:master Oct 5, 2022
@lukasj lukasj linked an issue Oct 5, 2022 that may be closed by this pull request
@yersan yersan deleted the secmgr branch October 5, 2022 14:21
@yersan
Copy link
Contributor Author

yersan commented Oct 5, 2022

Thanks. @lukasj, do you know by any chance when will be the next release of these two Mail and Activation?

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.

j.m.u.FactoryFinder.factoryFromServiceLoader needs PrivilegedAction

3 participants