-
Notifications
You must be signed in to change notification settings - Fork 110
Add the ability to the API consumers to load the API implementations … #623
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
Conversation
|
Could you also add a new line here please?: It should contain the next: The PR looks good to me. |
|
Hi @jbescos , I've just added the line to CHANGES.txt, thanks for the review. |
|
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?. |
e40f094 to
72bc810
Compare
|
@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. |
jbescos
left a comment
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.
Thank you
| public static StreamProvider provider() { | ||
| return FactoryFinder.find(StreamProvider.class); | ||
| } | ||
| public static StreamProvider provider() { |
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.
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.
lukasj
left a comment
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.
use spaces instead of TABs, please
…by using a different protection domain when the API is used with security manager enabled
|
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 |
|
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
left a comment
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.
LGTM
|
Thanks. @lukasj, do you know by any chance when will be the next release of these two Mail and Activation? |
…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.nextwas 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 exampleThread.currentThread().getContextClassLoader();orClass.forName(OSGI_SERVICE_LOADER_CLASS_NAME);This PR moves the whole doPrivilegde one level above to the
StreamProviderclass 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.