Review Request for 7026255 : Methods of Subject that throw SecurityException do not specify what permissions are required

[Sean Mullan]

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
>