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.

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

Remove internal permissions types and most of SecurityHelper #969

Merged
merged 21 commits into from
Jun 25, 2019

Conversation

ojhad
Copy link
Contributor

@ojhad ojhad commented Jun 14, 2019

Opened an internal PR which should be merged first in order to avoid build errors.

Part if issue #241

  • Removed dependence on System.Security.Permission.dll for internal types that extend from System.Security.Permission.dll types
  • Deleted internal permission types
  • Deleted most of SecurityHelper

@ojhad ojhad requested review from rladuca and vatsan-madhavan June 14, 2019 23:21
@ghost ghost requested review from ryalanms and stevenbrix June 14, 2019 23:21
@vatsan-madhavan
Copy link
Member

vatsan-madhavan commented Jun 15, 2019

General observation: MS.Internal.WindowsBase.SecurityHelper in its entirely can be removed. It consists of DemandXXX() type helpers, and one CheckXX() method.

I realize that this suggestion doesn't directly overlap with the core purpose of this PR (which is to remove types that directly inherit from S.S.Permission types), and that you are planning on putting out a separate PR that excises code containing asserts/demands etc.

That said, some overlap in purpose is unavoidable , I think. For e..g., not following the logical thread to remove RightsManagementPermission etc. now would just lead to the inadvertent retention of dead code. In the next round of analysis, for e.g. some of these types would appear as if they have relationship to S.S.Permission types... and possibly lead us to ignore them. So better to remove them when we are looking closely at these types.

The rest of SecurityHelper (i.e., the parts that are not directly affected by this PR) itself can be handled now or later - that's upto you. I personally think it might be easier to delete the whole thing and adjust the codebase once, rather than doing it piecemeal.

@grubioe grubioe added the PR metadata: Label to tag PRs, to facilitate with triage label Jun 17, 2019
@rladuca
Copy link
Member

rladuca commented Jun 18, 2019

The rest of SecurityHelper (i.e., the parts that are not directly affected by this PR) itself can be handled now or later - that's upto you. I personally think it might be easier to delete the whole thing and adjust the codebase once, rather than doing it piecemeal.

In my eyes it makes sense, time permitting, to remove as much as possible of the CAS specific code as early as possible. We're digging around right now, let's do this before other items take precedence.

@ojhad ojhad force-pushed the user/ojhad/apisecuritypermissioninternal branch from 501fb41 to 51986c8 Compare June 21, 2019 23:15
@ojhad
Copy link
Contributor Author

ojhad commented Jun 21, 2019

The rest of SecurityHelper (i.e., the parts that are not directly affected by this PR) itself can be handled now or later - that's upto you. I personally think it might be easier to delete the whole thing and adjust the codebase once, rather than doing it piecemeal.

Removed most of the SecurityHelper methods. Left a couple methods which have some non-cas related logic and left ExtractAppDomainPermissionSetMinusSiteOfOrigin which is more difficult to remove (however it does not keep a dependency on System.Security.Permissions.dll).

Opened an internal PR as well which should be merged first in order to avoid errors.

@ojhad ojhad changed the title Remove System.Security.Permission.dll dependence for internal permissions types Remove internal permissions types and most of SecurityHelper Jun 21, 2019
@vatsan-madhavan
Copy link
Member

Removed most of the SecurityHelper methods. Left a couple methods which have some non-cas related logic and left ExtractAppDomainPermissionSetMinusSiteOfOrigin which is more difficult to remove (however it does not keep a dependency on System.Security.Permissions.dll).

Pretty straightforward to fix this. You can cherry pick my change from 49f8134

@vatsan-madhavan vatsan-madhavan added this to the Preview milestone Jun 24, 2019
@vatsan-madhavan
Copy link
Member

Consider cherry-picking 49f8134 into this before merging.

@dotMorten
Copy link
Contributor

Note: This PR also fixes #722

@vatsan-madhavan
Copy link
Member

30cbe44 has what you need to remove System.Xaml.XamlObjectWriterSettings.AccessLevel

@ojhad ojhad force-pushed the user/ojhad/apisecuritypermissioninternal branch from 2e045ba to e5fd477 Compare June 25, 2019 10:14
@ojhad ojhad force-pushed the user/ojhad/apisecuritypermissioninternal branch from e5fd477 to 0a1b4b5 Compare June 25, 2019 20:52
@ojhad ojhad merged commit 875b508 into master Jun 25, 2019
@vatsan-madhavan vatsan-madhavan deleted the user/ojhad/apisecuritypermissioninternal branch August 23, 2019 20:11
eFloh pushed a commit to eFloh/InternetRadio that referenced this pull request Jan 11, 2022
When `MediaElement.Source` is set to a HTTPS-Uri
- .NET 4.8 Project throws `NullReferenceException`
   at System.Windows.Media.MediaPlayerState.OpenMedia(Uri source)
   at System.Windows.Media.MediaPlayerState.SetSource(Uri source)
   at System.Windows.Media.MediaPlayerState.Open(Uri source)
- .NET Core 5.0 Application sucessfully plays the Source.

Plain HTTP works in both runtimes.

See
https://stackoverflow.com/questions/30702505/playing-media-from-https-site-in-media-element-throwing-null-reference-exception
dotnet/wpf#722
fixed and merged with dotnet/wpf#969
into .NET Core.
eFloh pushed a commit to eFloh/InternetRadio that referenced this pull request Jan 11, 2022
When `MediaElement.Source` is set to a HTTPS-Uri
- .NET 4.8 Project throws `NullReferenceException`
   at System.Windows.Media.MediaPlayerState.OpenMedia(Uri source)
   at System.Windows.Media.MediaPlayerState.SetSource(Uri source)
   at System.Windows.Media.MediaPlayerState.Open(Uri source)
- .NET Core 5.0 Application sucessfully plays the Source.

Plain HTTP works in both runtimes.

See
https://stackoverflow.com/questions/30702505/playing-media-from-https-site-in-media-element-throwing-null-reference-exception
dotnet/wpf#722
fixed and merged with dotnet/wpf#969
into .NET Core.
@ghost ghost locked as resolved and limited conversation to collaborators Apr 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants