Porting System.Security.Claims #4933
Conversation
|
@weshaggard : Please have a look. |
| private ClaimsIdentity _subject; | ||
| private string _type; | ||
| private string _value; | ||
| private string _valueType; |
There was a problem hiding this comment.
Can any of these fields be readonly?
There was a problem hiding this comment.
they are used to hold property values that are just {get}. Today's coding practice is usually { private set }.
There was a problem hiding this comment.
But at the very least a field like _properties should be readonly.
There was a problem hiding this comment.
A user can add a property at any time.
There was a problem hiding this comment.
That doesn't affect whether the dictionary field can be readonly.
|
A couple other questions:
|
| private ClaimsIdentity _actor; | ||
| private string _authenticationType; | ||
| private object _bootstrapContext; | ||
| private Collection<IEnumerable<Claim>> _externalClaims = new Collection<IEnumerable<Claim>>(); |
There was a problem hiding this comment.
Could _externalClaims be typed as List<List<Claim>> instead of Collection<IEnumerable<Claim>>? It looks like this is only used internally and the IEnumerable<Claim>s are always List<T> already. This would be slightly more efficient than using Collection<T> and would avoid enumerator allocations when enumerating each item in the Claims property as List<T>'s struct enumerator would be used.
There was a problem hiding this comment.
Yes they are only used internally and was designed for holding claims from 'roleProviders'.
I don't see any reason to avoid improving this.
|
Opened a tracking issue for the proposed refactoring #4935 |
venkat-raman251
force-pushed
the
master
branch
from
f497210 to
a19c331
Compare
| <value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value> | ||
| </resheader> | ||
| <data name="InvalidOperation_ClaimCannotBeRemoved" xml:space="preserve"> | ||
| <value>The Claim '{0}' was not able to be removed. It is either not part of this Identity or it is a claim that is owned by the Principal that contains this Identity. For example, the Principal will own the claim when creating a GenericPrincipal with roles. The roles will be exposed through the Identity that is passed in the constructor, but not actually owned by the Identity. Similar logic exists for a RolePrincipal.</value> |
There was a problem hiding this comment.
Nit: "claim" is sometimes capitalized and sometimes not. Not clear to me the rhyme or reason for it.
There was a problem hiding this comment.
Since it is a type, how about 'Claim'? Capitalized.
There was a problem hiding this comment.
I'm happy either way, as long as we're consistent 😄
|
Are there any tests to go with the library? |
| <AdditionalProperties>TargetGroup=</AdditionalProperties> | ||
| </Project> | ||
| <Project Include="System.Security.Claims.csproj"> | ||
| <AdditionalProperties>TargetGroup=net46</AdditionalProperties> |
There was a problem hiding this comment.
This is a duplication of facade\System.Secrit.Claims.csproj configuration. As part of your change you should eliminate the facade folder completely given you have folded it into this project. Also condition this on TargetsWindows=true
There was a problem hiding this comment.
@venkat-raman251 please collapse these net46 configurations.
|
@dotnet-bot Test Innerloop Ubuntu Debug Build and Test please (Jenkins failure) |
| return new GenericIdentity(this); | ||
| } | ||
|
|
||
| public override IEnumerable<Claim> Claims |
There was a problem hiding this comment.
Just curious: why is the override necessary?
venkat-raman251
force-pushed
the
master
branch
from
a19c331 to
3536406
Compare
|
@weshaggard : Updated with all the changes. Also split into 2 commits to identify changes. |
|
LGTM |
Porting System.Security.Claims
No description provided.