Proposed code fixer for obsoleted CAS APIs

[GrabYourPitchforks]

Per dotnet/designs#139, we want to obsolete some of the CAS-related APIs in .NET 5.0. The obsoleted APIs are primarily classes that derive from CodeAccessSecurityAttribute and CodeAccessPermission.

The fixer should help apps migrate away from these now-obsolete types.

For CodeAccessSecurityAttribute-derived which are obsolete (except PrincipalPermissionAttribute), the fixer should look at the SecurityAction parameter passed into the attribute ctor. If the value is one of SecurityAction.Assert, Demand, InheritanceDemand, LinkDemand, RequestMinimum, or RequestOptional, the fixer should recommend that the attribute declaration be removed entirely. If the SecurityAction enum value is anything else, the fixer should make no recommendation.

For CodeAccessPermission-derived types which are obsolete (n.b. PrincipalPermission does not subclass this type and should be excluded), the fixer should suggest calls to Assert and Demand be removed. If any other API is called, the fixer should make no recommendation.

Examples:

// fixer should recommend removal
[SecurityPermission(SecurityAction.Demand, ...)]
public void SomeMethod() { /* ... */ }

// fixer should NOT recommend removal
// dev needs to take a closer look at what they were trying to accomplish
[SecurityPermission(SecurityAction.Deny, ...)]
public void SomeMethod() { /* ... */ }

// note: this is already obsolete *as error*
// fixer should NOT recommend removal
// dev needs to take a closer look at what they were trying to accomplish
[PrincipalPermission(SecurityAction.Demand, ...)]
public void SomeMethod() { /* ... */ }

public void SomeMethod()
{
    // fixer should recommend removal of this line
    new SecurityPermission(...).Demand();

    // fixer should NOT recommend removal
    // dev needs to take a closer look at what they were trying to accomplish
    // n.b. this call will always result in PNSE at runtime
    new SecurityPermission(...).Deny();

    // fixer should NOT recommend removal; PrincipalPermission does not subclass CodeAccessPermission
    // dev needs to take a closer look at what they were trying to accomplish
    new PrincipalPermission(...).Demand().
}

For PrincipalPermissionAttribute and PrincipalPermission specifically, we should steer the developer toward using ASP.NET's built-in AuthN / AuthZ features.

[ghost]

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq
Notify danmosemsft if you want to be subscribed.

[GrabYourPitchforks]

To clarify the above "fixer shouldn't do anything" examples a bit...

// fixer should NOT recommend removal
// dev needs to take a closer look at what they were trying to accomplish
[SecurityPermission(SecurityAction.Deny, ...)]
public void SomeMethod() { /* ... */ }

// note: this is already obsolete *as error*
// fixer should NOT recommend removal
// dev needs to take a closer look at what they were trying to accomplish
[PrincipalPermission(SecurityAction.Demand, ...)]
public void SomeMethod() { /* ... */ }

In the cases above, the .NET Core runtime does not honor the attributes. The runtime itself behaves as if the attributes were not present. But the only logical reason for a dev to have written these attributes in the first place is that they expected the runtime to perform a check on method entry. The dev should be made aware of this disconnect: "You put these because presumably you wanted a side effect, but the current runtime will no-op when it sees them. What did you actually intend to do?"

public void SomeMethod()
{
    // fixer should NOT recommend removal
    // dev needs to take a closer look at what they were trying to accomplish
    // n.b. this call will always result in PNSE at runtime
    new SecurityPermission(...).Deny();
}

In the case above, the call to Deny has the side effect of always throwing PNSE. The fixer cannot suggest removing this because it will change the control flow of the method. Instead, the dev should be made aware of the fact that this is an "always PNSE" API, and they need to take the course of action that is appropriate for their scenario.

public void SomeMethod()
{
    // fixer should NOT recommend removal; PrincipalPermission does not subclass CodeAccessPermission
    // dev needs to take a closer look at what they were trying to accomplish
    new PrincipalPermission(...).Demand().
}

In the case above, the call to Demand may throw an exception. The fixer cannot suggest removing this because it may change the control flow of the method.

[jeffhandley]

@GrabYourPitchforks / @terrajobst - I've removed this from scope for .NET 6. Do you think we need to present this in API Review, or could we could ahead and mark it as up-for-grabs?