Fix Flaky Tests: NameAndRoleClaimDelegates and RoleClaims #2873
Conversation
| @@ -112,6 +112,10 @@ public bool MapInboundClaims | |||
| if (!_mapInboundClaims && value && _inboundClaimTypeMap.Count == 0) | |||
| _inboundClaimTypeMap = new Dictionary<string, string>(DefaultInboundClaimTypeMap); | |||
|
|
|||
| // If the inbound claim type mapping is on and being turned off, make sure that the _inboundClaimTypeMap is empty. | |||
| if (_mapInboundClaims && !value && _inboundClaimTypeMap.Count > 0) | |||
| _inboundClaimTypeMap.Clear(); | |||
There was a problem hiding this comment.
This is logically what I believe should happen - but is there a reason why we didn't have this before?
There was a problem hiding this comment.
Quick thought: _mapInboundClaims flag should be checked if it's true before map is used. If it's false, the map should never be used. So this should not be needed.
There was a problem hiding this comment.
Do we have the same behavior in JsonWebTokenHandler?
There was a problem hiding this comment.
The test tests that the constructor of JwtSecurityTokenHandler sets up the map or not depending if the static flag DefaultMapInboundClaims was set or not; then it also checks if setting MapInboundClaims property initializes the map list.
Both MapInboundClaims and InboundClaimTypeMap are public. In our code we check the flag first before using the map. There are a couple of usage scenarios.
- MapInboundClaims is false and InboundClaimTypeMap is empty. Set flag to true. This will initialize the map to the default map values, which makes sense since we don't know if the map will be explicitly initialized, so we set it to default. The map can still be overwritten to non-default values.
- InboundClaimTypeMap is set to custom values. MapInboundClaims is set to true. This will not reset the map to default values because it already as values.
- InboundClaimTypeMap is set to custom values. MapInboundClaims is set to true, then set to false. With the previous code, the map will not be cleared and when the flag is set to true, custom values will remain. With updated code, the custom values will be cleared, and when the flag is set to true, map will be set to default values. This is the possible breaking change. Probably unlikely but still.
So I would only update the test to reset DefaultMapInboundClaims to true (default) and move it and add a collection attribute to it.
See explanation here.
Fixes #2841