Does the test need to run in othervm? It doesn't seem like it adds or
changes the order of providers.
Otherwise looks good.
--Sean
On 07/22/2016 06:19 PM, Valerie Peng wrote:
I added a new test covering the basics of getInstance() methods.
Webrev updated: http://cr.openjdk.java.net/~valeriep/8159488/webrev.02
If you have a particular scenario that you want me to cover, please let
me know.
Thanks,
Valerie
On 7/21/2016 12:55 PM, Valerie Peng wrote:
Existing regression tests for the java.xml.crypto do test the
getInstance() methods as I observed during my own testing.
Maybe not all of them are covered though. I will double check and add
test if necessary...
Thanks,
Valerie
On 7/21/2016 12:08 PM, Sean Mullan wrote:
On 07/20/2016 07:57 PM, Valerie Peng wrote:
Sean,
I have updated webrev to rid the dependency on sun.security.jca
packages. Also, I found one additional SecurityPermission is needed.
Please find the updated webrev at:
http://cr.openjdk.java.net/~valeriep/8159488/webrev.01/
So, we should close JDK-8161710 as a duplicate of JDK-8159488 then?
Yes. The fix looks good. Could you write a simple test which calls
the getInstance methods with good/bogus algs and providers just to
make sure they all work as expected? There don't seem to be any
regression tests for that.
Thanks,
Sean
Thanks,
Valerie
On 7/19/2016 11:57 AM, Sean Mullan wrote:
On 07/18/2016 05:38 PM, Valerie Peng wrote:
Hi Sean,
I found these two classes in java.xml.crypto module reading local
files:
src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/keys/storage/implementations/CertsInFilesystemDirectoryResolver.java
src/java.xml.crypto/share/classes/com/sun/org/apache/xml/internal/security/utils/JavaUtils.java
I think we should leave it out. The methods that need that permission
are in non-exported packages and are essentially unused.
Also, you may have noticed, but I just filed
https://bugs.openjdk.java.net/browse/JDK-8161710. Fixing this would
allow us to remove this permission:
permission java.lang.RuntimePermission
"accessClassInPackage.sun.security.jca.*";
I'm not sure if you have the time to tackle that as part of this
issue, but I think it would be acceptable to fix them together as part
of this issue if so.
--Sean
If you think the File reading permission is not needed for
java.xml.crypto module, I will remove the corresponding permission
entry.
Thanks,
Valerie
On 7/18/2016 12:48 PM, Sean Mullan wrote:
On 07/13/2016 08:10 PM, Valerie Peng wrote:
Sean,
Can you please review the following two webrevs?
Bug: https://bugs.openjdk.java.net/browse/JDK-8159488
Webrev: http://cr.openjdk.java.net/~valeriep/8159488/
Looks good except for this one:
127 // needed for reading Certs
128 permission java.io.FilePermission "<<ALL
FILES>>","read";
Why is that needed?
While making changes for 8159488, I noticed a problem with my
earlier
putback of 8154191 - the top level Modules.gmk was not integrated.
So, I filed 8161171: Missed the make/common/Modules.gmk file when
integrating JDK-8154191.
Can you also review this? It's essentially the same change as
the one
reviewed.
Bug: https://bugs.openjdk.java.net/browse/JDK-8161171
Webrev: http://cr.openjdk.java.net/~valeriep/8161171/webrev.00/
I'll skip this since Mandy already reviewed that one.
--Sean
