On 08/12/2014 09:44 PM, Xuelei Fan wrote:
In the new file:
653-677, 719-721:
It would be nice to mention "if a security manager is installed," ...
Good point, changed.
656 * is thrown if the caller does not have the proper permissions.
Do we want to point out the actual modify permissions?
This is pointed out in the following paragraph:
* <p> While iterating through the {@code Set},
* a {@code SecurityException} is thrown
* if the caller does not have a {@link PrivateCredentialPermission}
* to access a particular Credential. The {@code Iterator}
* is nevertheless advanced to the next element in the {@code Set}.
721 * <code>SecurityException</code> will be thrown.
---------------
Do you want to use the {@code SecurityException} style?
Fixed.
772~777
The words may be able to shorten as:
@throws SecurityException if the caller does not have
a {@link PrivateCredentialPermission} permission
to access the private credentials for this or
the provided {@code Subject}
Good suggestion, changed.
1540 static class AuthPermissionHolder {
1541 static final AuthPermission DO_AS_PERMISSION =
1542 new AuthPermission("doAs");
I'm not sure why define this innner class. Looks like this permissions
can be defined as static final variables. Otherwise, it might be better
to define AuthPermissionHolder as static final class, or enum.
The main benefit of the AuthPermissionHolder class is that it only
allocates memory for these constants when the class is loaded which is
when one of the security-sensitive methods is invoked and an SM is
installed. So there is a small amount of memory savings when you are not
using an SM. I think it is still worthwhile in that case. I don't think
there are any major benefits switching to an enum here, but I did add
the final keyword to the class.
Updated webrev:
http://cr.openjdk.java.net/~mullan/webrevs/7026255/webrev.01/
Thanks,
Sean
Otherwise, looks fine to me.
Xuelei
On 8/12/2014 11:08 PM, Sean Mullan wrote:
This is a clarification in the javax.security.auth.Subject javadocs to
indicate what permissions are required for methods that throw
SecurityException:
http://cr.openjdk.java.net/~mullan/webrevs/7026255/webrev.00/
I also took the opportunity to fix a couple of other minor issues: added
@Override annotations, removed spurious <p> tags, and changed @exception
to @throws.
Thanks,
Sean
