Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Obsolete PrincipalPermissionAttribute ctor as error #37536

Merged
merged 3 commits into from
Jun 18, 2020
Merged

Obsolete PrincipalPermissionAttribute ctor as error #37536

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
08cdf3b
Obsolete PrincipalPermissionAttribute ctor as error
GrabYourPitchforks Jun 6, 2020
290ae3c
Update diagnostic id; maintain centralized list
GrabYourPitchforks Jun 10, 2020
bcd6662
Code review: remove ExcludeCurrentNetCoreAppFromPackage
GrabYourPitchforks Jun 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions docs/project/list-of-obsoletions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
List of Obsoletions
==================

Per https://github.com/dotnet/designs/blob/master/accepted/2020/better-obsoletion/better-obsoletion.md, we now have a strategy in place for marking existing APIs as `[Obsolete]`. This takes advantage of the new diagnostic id and URL template mechanisms introduced to `ObsoleteAttribute` in .NET 5.

When obsoleting an API, use the diagnostic ID `MSLIB####`, where _\#\#\#\#_ is the next four-digit identifier in the sequence, and add it to the list below. This helps us maintain a centralized location of all APIs that were obsoleted using this mechanism.

The URL template we use for obsoletions is `https://aka.ms/dotnet-warnings/{0}`.

Currently the identifiers `MSLIB0001` through `MSLIB0999` are carved out for obsoletions. If we wish to introduce analyzer warnings not related to obsoletion in the future, we should begin at a different range, such as `MSLIB2000`.

## Current obsoletions (`MSLIB0001` - `MSLIB0999`)

| Diagnostic ID | Description |
| :--------------- | :---------- |
| __`MSLIB0001`__ | (Reserved for `Encoding.UTF7`.) |
| __`MSLIB0002`__ | `PrincipalPermissionAttribute` is not honored by the runtime and must not be used. |
3 changes: 2 additions & 1 deletion eng/CodeAnalysis.ruleset
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,8 @@
<Rule Id="CA2237" Action="None" /> <!-- Mark ISerializable types with serializable -->
<Rule Id="CA2241" Action="Warning" /> <!-- Provide correct arguments to formatting methods -->
<Rule Id="CA2242" Action="Warning" /> <!-- Test for NaN correctly -->
<Rule Id="CA2243" Action="Warning" /> <!-- Attribute string literals should parse correctly -->
<!-- CA2243 temporarily disabled: https://github.com/dotnet/roslyn-analyzers/issues/3635 -->
<Rule Id="CA2243" Action="None" /> <!-- Attribute string literals should parse correctly -->
<Rule Id="CA2244" Action="None" /> <!-- Do not duplicate indexed element initializations -->
<Rule Id="CA2245" Action="Warning" /> <!-- Do not assign a property to itself. -->
<Rule Id="CA2246" Action="None" /> <!-- Assigning symbol and its member in the same statement. -->
Expand Down
3 changes: 3 additions & 0 deletions src/libraries/System.Security.Permissions/ref/System.Security.Permissions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1128,6 +1128,9 @@ public void FromXml(System.Security.SecurityElement elem) { }
[System.AttributeUsageAttribute(System.AttributeTargets.Class | System.AttributeTargets.Method, AllowMultiple=true, Inherited=false)]
public sealed partial class PrincipalPermissionAttribute : System.Security.Permissions.CodeAccessSecurityAttribute
{
#if CAS_OBSOLETIONS
[System.ObsoleteAttribute("PrincipalPermissionAttribute is not honored by the runtime and must not be used.", true, DiagnosticId = "MSLIB0002", UrlFormat = "https://aka.ms/dotnet-warnings/{0}")]
#endif
public PrincipalPermissionAttribute(System.Security.Permissions.SecurityAction action) : base (default(System.Security.Permissions.SecurityAction)) { }
public bool Authenticated { get { throw null; } set { } }
public string Name { get { throw null; } set { } }
Expand Down
3 changes: 3 additions & 0 deletions src/libraries/System.Security.Permissions/ref/System.Security.Permissions.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
<PropertyGroup>
<IsPartialFacadeAssembly Condition="$(TargetFramework.StartsWith('net4'))">true</IsPartialFacadeAssembly>
</PropertyGroup>
<PropertyGroup Condition="'$(TargetFramework)' == '$(NetCoreAppCurrent)'">
<DefineConstants>$(DefineConstants);CAS_OBSOLETIONS</DefineConstants>
</PropertyGroup>
<ItemGroup>
<Compile Include="System.Security.Permissions.cs" />
<Compile Include="System.Security.Permissions.Forwards.cs" />
Expand Down
3 changes: 3 additions & 0 deletions src/libraries/System.Security.Permissions/src/System.Security.Permissions.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
<ExcludeCurrentNetCoreAppFromPackage>true</ExcludeCurrentNetCoreAppFromPackage>
GrabYourPitchforks marked this conversation as resolved.
Show resolved Hide resolved
<ExcludeCurrentFullFrameworkFromPackage>true</ExcludeCurrentFullFrameworkFromPackage>
</PropertyGroup>
<PropertyGroup Condition="'$(TargetFramework)' == '$(NetCoreAppCurrent)'">
<DefineConstants>$(DefineConstants);CAS_OBSOLETIONS</DefineConstants>
</PropertyGroup>
<ItemGroup Condition="!$(TargetFramework.StartsWith('net4'))">
<Compile Include="System\ApplicationIdentity.cs" />
<Compile Include="System\Configuration\ConfigurationPermission.cs" />
Expand Down
3 changes: 3 additions & 0 deletions ...stem.Security.Permissions/src/System/Security/Permissions/PrincipalPermissionAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ namespace System.Security.Permissions
[AttributeUsage((AttributeTargets)(68), AllowMultiple = true, Inherited = false)]
public sealed partial class PrincipalPermissionAttribute : CodeAccessSecurityAttribute
{
#if CAS_OBSOLETIONS
[Obsolete("PrincipalPermissionAttribute is not honored by the runtime and must not be used.", error: true, DiagnosticId = "MSLIB0002", UrlFormat = "https://aka.ms/dotnet-warnings/{0}")]
#endif
public PrincipalPermissionAttribute(SecurityAction action) : base(default(SecurityAction)) { }
public bool Authenticated { get; set; }
public string Name { get; set; }
Expand Down
7 changes: 0 additions & 7 deletions src/libraries/System.Security.Permissions/tests/PermissionTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,13 +210,6 @@ public static void PrincipalPermissionCallMethods()
pp.ToXml();
}

[Fact]
public static void PrincipalPermissionAttributeCallMethods()
{
PrincipalPermissionAttribute ppa = new PrincipalPermissionAttribute(new Permissions.SecurityAction());
IPermission ip = ppa.CreatePermission();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since existing binaries will continue to work, seems valuable to continue to validate the behavior. Do we not have any way to write the test given the error obsoletion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could put a test binary in the test data repo (where we put compression test data)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These methods quite literally no-op. If you attempt to use them for real you'll get a NullReferenceException. IMO this test (and the tests for FileIOPermissionAttribute, etc.) isn't testing anything useful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's really a desire to keep this test, we could always Activator.CreateInstance it, since that won't produce the build error. But again, since the implementations are no-ops, I'm not sure what utility such a test has.

I'll defer to your collective judgment if you feel that such a test is useful. :)


[Fact]
public static void PublisherIdentityPermissionCallMethods()
{
Expand Down