PrincipalPermissionAttribute ctor should be obsolete as error

[GrabYourPitchforks]

We have a few work items to obsolete existing APIs that are legacy / unsupported / disrecommended. In most cases the behaviors of the APIs are either unchanged from their Full Framework implementations or throw PNSE when they're invoked.

However, per #31279, PrincipalPermissionAttribute is a special-case. In Full Framework, this attribute is generally honored by the runtime (even in full trust environments!) and leverages the CAS infrastructure to turn a declarative authn / authz check into a runtime-enforced check. However, in Core, this and other CAS attributes are ignored by the runtime. This leads to a "fail open" scenario.

I wanted to split this out from the other API proposals because I think the situation is bad enough where this merits obsoletion as error, and obsoleting an API as error is clearly a breaking change.

API proposal

namespace System.Security.Permissions
{
    public sealed class PrincipalPermissionAttribute : CodeAccessSecurityAttribute
    {
        [Obsolete("<message goes here>", error: true)] // ** NEW ATTRIBUTE ON EXISTING MEMBER **
        public PrincipalPermissionAttribute(SecurityAction action) { }
        public bool Authenticated { get { throw null; } set { } }
        public string Name { get { throw null; } set { } }
        public string Role { get { throw null; } set { } }
        public override IPermission CreatePermission() { throw null; }
    }
}

Of note is that this proposal only contemplates obsoleting the constructor as error. The reason for this is that we want anybody who has slapped a [PrincipalPermission] declaration on an API to see the compile-time error. It is not sufficient simply to make the constructor throw PNSE, as the constructor isn't actually invoked at runtime.

Marking only the constructor - not the entire type - with "as error" allows scenarios like the below to continue working.

MethodInfo theMethod = GetMethod();
if (theMethod.IsDefined(typeof(PrincipalPermissionAttribute))
{
    /* do something */
}

Obsoleting the constructor as error does not prevent us from obsoleting other members (or the type itself) as warning. There is some discussion about doing this to the CAS types all-up. But that's a matter for a different issue. I wanted to focus discussion here on the constructor given the breaking change nature of obsoleting this member as error.

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

[GrabYourPitchforks]

If there's opposition to obsoleting the ctor as error, an alternative proposal is to change the type's legal attribute targets to None. This would allow instantiation of the attribute type itself, but no member could be decorated with the attribute.

Though to be fair I don't know why anybody would ever want to instantiate CAS attributes rather than the CAS permission objects directly.

[terrajobst]

Video

• Looks good as proposed, but we should be adding an URL
• We should also provide a fixer(er) with the potential suggestions (apply ASP.NET Core attribute, using the thread's principal).
• We should consider a generic analyzer that warns on SecurityAction.Deny usage across all attributes

namespace System.Security.Permissions
{
    public partial class PrincipalPermissionAttribute
    {
        [Obsolete("<message goes here>", error: true, DiagnosticId="<diagnostic ID>", UrlFormat="<URL goes here>")]
        public PrincipalPermissionAttribute(SecurityAction action);
    }
}