{"action":"created","issue":{"url":"https://api.github.com/repos/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/300","labels_url":"https://api.github.com/repos/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/300/labels{/name}","comments_url":"https://api.github.com/repos/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/300/comments","events_url":"https://api.github.com/repos/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/300/events","html_url":"https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300","id":113941154,"number":300,"title":"RC1 fixes.","user":{"login":"brentschmaltz","id":3172421,"avatar_url":"https://avatars.githubusercontent.com/u/3172421?v=3","gravatar_id":"","url":"https://api.github.com/users/brentschmaltz","html_url":"https://github.com/brentschmaltz","followers_url":"https://api.github.com/users/brentschmaltz/followers","following_url":"https://api.github.com/users/brentschmaltz/following{/other_user}","gists_url":"https://api.github.com/users/brentschmaltz/gists{/gist_id}","starred_url":"https://api.github.com/users/brentschmaltz/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/brentschmaltz/subscriptions","organizations_url":"https://api.github.com/users/brentschmaltz/orgs","repos_url":"https://api.github.com/users/brentschmaltz/repos","events_url":"https://api.github.com/users/brentschmaltz/events{/privacy}","received_events_url":"https://api.github.com/users/brentschmaltz/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/labels/cla-not-required","name":"cla-not-required","color":"009800"}],"state":"closed","locked":false,"assignee":null,"milestone":null,"comments":8,"created_at":"2015-10-28T22:40:19Z","updated_at":"2015-11-18T23:07:31Z","closed_at":"2015-10-29T17:00:01Z","pull_request":{"url":"https://api.github.com/repos/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pulls/300","html_url":"https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300","diff_url":"https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300.diff","patch_url":"https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300.patch"},"body":"1. Formatting in logging.\r\n2. Add SecurityTokenDescriptor\r\n3. Use SigningCredentials only, remove SignatureProvider on JwtSecurityTokenHandler\r\n4. Remove statics on JwtSecurityTokenHandler\r\n5. OM assuming JwtSecurityToken on JwtSecurityTokenHandler\r\n6. Test Cleanup\r\n7. General code cleanup.\n<a href='#crh-start'></a><a href='#crh-data-%7B%22approved%22%3A%20%7B%22https%3A//github.com/tushargupta51%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%7D%7D%2C%20%22comments%22%3A%20%7B%22General%20Comment%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23issuecomment-152248710%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22%3Ashipit%3A%22%2C%20%22created_at%22%3A%20%222015-10-29T16%3A59%3A15Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%5D%2C%20%22title%22%3A%20%22General%20Comment%22%7D%2C%20%22Pull%203ecb3cf2c14c623bec2d4f8f734817622e4138db%20src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs%20429%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43336492%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22this%20can%20be%20replaced%20by%20return%20new%20JwtSecurityToken%28token%29%3B%22%2C%20%22created_at%22%3A%20%222015-10-29T00%3A23%3A33Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22I%20didn%27t%20do%20that%20because%2C%20I%20didn%27t%20want%20to%20do%20all%20the%20checks%20again%20in%20the%20ctor%20of%20JwtSecurityToken.%22%2C%20%22created_at%22%3A%20%222015-10-29T00%3A56%3A06Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs%3AL416-496%22%7D%2C%20%22Pull%203ecb3cf2c14c623bec2d4f8f734817622e4138db%20src/System.IdentityModel.Tokens/Base64UrlEncoder.cs%2057%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43338663%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22move%20this%20to%20LogMessages.cs%22%2C%20%22created_at%22%3A%20%222015-10-29T00%3A56%3A14Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22sure%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A50%3A36Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20src/System.IdentityModel.Tokens/Base64UrlEncoder.cs%3AL133-140%22%7D%2C%20%22Pull%203ecb3cf2c14c623bec2d4f8f734817622e4138db%20src/System.IdentityModel.Tokens.Jwt/JwtHeader.cs%2050%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43335006%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22There%20is%20some%20discrepancy%20in%202%20constructors.%20The%20result%20of%20new%20JwtHeader%28%29%20and%20new%20JwtHeader%28null%29%20will%20be%20different.%20That%20does%20not%20look%20right.%20Whatever%20we%20are%20doing%20when%20signingCredentials%20is%20null%2C%20shouldnt%20we%20that%20in%20the%20default%20ctor%20too%3F%22%2C%20%22created_at%22%3A%20%222015-10-29T00%3A01%3A07Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22the%20other%20constructor%20calls%20this%28null%29%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A04%3A25Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%2C%20%7B%22body%22%3A%20%22I%20noticed%20that.%20But%20my%20point%20is%20why%20there%20is%20a%20difference.%20The%20result%20of%20new%20JwtHeader%28%29%20and%20new%20JwtHeader%28null%29%20should%20be%20same.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A34%3A18Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22I%20think%20they%20are%2C%20I%20will%20add%20a%20test%20just%20to%20be%20sure.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A41%3A23Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%2C%20%7B%22body%22%3A%20%22No%2C%20they%20are%20not.%20In%20case%20of%20JwtHeader%28null%29%2C%20Alg%20will%20have%20None%20value%20and%20Typ%20is%20initialized%20to%20JwtConstants.HeaderType%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A50%3A56Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22Ah%2C%20my%20bad.%20They%20are%20equal.%20Test%20was%20not%20needed.%20I%20read%20that%20as%20base%28null%29%20and%20that%27s%20why%20thinking%20they%20will%20result%20in%20different%20dictionaries%2C%20didn%27t%20notice%20the%20ctor%20was%20chained.%20Sorry%20for%20the%20trouble.%22%2C%20%22created_at%22%3A%20%222015-10-29T04%3A07%3A55Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20src/System.IdentityModel.Tokens.Jwt/JwtHeader.cs%3AL31-74%22%7D%2C%20%22Pull%203ecb3cf2c14c623bec2d4f8f734817622e4138db%20src/System.IdentityModel.Tokens.Jwt/JwtSecurityToken.cs%2095%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43334398%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22this%20check%20can%20be%20made%20string.IsNullOrEmpty.%22%2C%20%22created_at%22%3A%20%222015-10-28T23%3A53%3A30Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22true%20we%20have%20that%20sprinkled%20through-out%20our%20code.%20But%20I%20choose%20not%20to%20clean%20it%20all%20up%20now.%22%2C%20%22created_at%22%3A%20%222015-10-29T00%3A48%3A35Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%2C%20%7B%22body%22%3A%20%22Since%20we%20are%20touching%20the%20code%20in%20this%20PR%2C%20we%20might%20as%20well%20change%20this%20one.%20WhiteSpace%20or%20empty%20signatures%20does%20not%20make%20any%20sense.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A33%3A27Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22I%20would%20like%20to%20do%20it%20in%20one%20pass%2C%20similar%20to%20how%20the%20Logging%20was%20changed.%20then%20we%20know%20we%20got%20it%20all.%20Just%20do%20a%20search%20and%20replace%2C%20piece%20meal%20makes%20no%20sense%20seeing%20how%20we%20have%20to%20go%20back.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A40%3A37Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20src/System.IdentityModel.Tokens.Jwt/JwtSecurityToken.cs%3AL84-111%22%7D%2C%20%22Pull%203ecb3cf2c14c623bec2d4f8f734817622e4138db%20src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs%20278%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43335933%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22In%20regex%20below%3A%5Cr%5Cnregex%20%3D%20new%20Regex%28JwtConstants.JsonCompactSerializationRegex%2C%20RegexOptions.None%2C%20TimeSpan.FromSeconds%282%29%29%5Cr%5Cn%5Cr%5Cnset%20the%20default%20to%20100%20ms%20as%20you%20set%20for%20the%20other%20one.%22%2C%20%22created_at%22%3A%20%222015-10-29T00%3A14%3A37Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22We%20will%20handle%20that%20in%20RC2.%5Cr%5CnActually%2C%20while%20I%20am%20making%20changes%20for%20other%20items%20you%20found%2C%20I%20will%20touch%20that%20up%20to%20100ms.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A02%3A48Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%2C%20%7B%22body%22%3A%20%22It%20will%20be%20inconsistent%20to%20allow%20100ms%20%28JwtSecurityToken%20constructor%29%20at%20one%20place%20and%202%20seconds%20%28CanReadToken%20method%29%20at%20another.%20We%20should%20either%20change%20both%20or%20none.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A38%3A04Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs%3AL273-284%22%7D%2C%20%22Pull%203ecb3cf2c14c623bec2d4f8f734817622e4138db%20src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs%20361%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43336167%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22nit%3A%20I%20don%27t%20think%20we%20need%20the%20boolean%20checks%20for%20other%20values%20here.%20We%20are%20going%20to%20check%20them%20again%20in%20the%20body.%20It%20could%20be%20just%20if%20%28SetDefaultTimesOnTokenCreation%20%29%20%7B%20...%20%7D%22%2C%20%22created_at%22%3A%20%222015-10-29T00%3A18%3A22Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22true%20enough%2C%20I%20will%20adjust%20the%20code.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A02%3A11Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs%3AL344-403%22%7D%2C%20%22Pull%203ecb3cf2c14c623bec2d4f8f734817622e4138db%20src/Microsoft.IdentityModel.Logging/IdentityModelEventSource.cs%20205%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43330387%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Should%20this%20message%20be%20logged%3F%20If%20so%2C%20am%20not%20sure%20if%20error%20is%20the%20right%20level.%22%2C%20%22created_at%22%3A%20%222015-10-28T23%3A02%3A59Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22I%20choose%20error%20as%20I%20wanted%20to%20make%20sure%20the%20user%20would%20see%20the%20details.%22%2C%20%22created_at%22%3A%20%222015-10-28T23%3A04%3A49Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%2C%20%7B%22body%22%3A%20%22But%20this%20message%20might%20not%20be%20as%20important%20as%20an%20%5C%22error%5C%22%20message.%20We%20are%20logging%20a%20message%20about%20unknown%20log%20level%20as%20an%20error.%20If%20a%20message%20comes%20with%20an%20unknown%20log%20level%2C%20we%20cannot%20assume%20that%20the%20message%20represents%20an%20error.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A25%3A39Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22the%20point%20is%20it%27s%20an%20error%20mainly%20because%20the%20level%20was%20basically%20wrong.%20So%20I%20think%20it%20is%20fine.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A36%3A25Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%2C%20%7B%22body%22%3A%20%22I%20understand%20that%20and%20it%20makes%20sense%20to%20log%20that%20particular%20information%20-%20%5C%22log%20level%20unknown%5C%22%20as%20error.%20But%20logging%20the%20associate%20message%20as%20an%20error%20might%20be%20too%20much.%22%2C%20%22created_at%22%3A%20%222015-10-29T03%3A52%3A09Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22While%2C%20then%20users%20will%20not%20see%20the%20message%2C%20which%20I%20think%20will%20make%20it%20hard%20to%20track%20down%20where%20the%20error%20is.%22%2C%20%22created_at%22%3A%20%222015-10-29T12%3A28%3A55Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20src/Microsoft.IdentityModel.Logging/IdentityModelEventSource.cs%3AL52-224%22%7D%2C%20%22Pull%203ecb3cf2c14c623bec2d4f8f734817622e4138db%20src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs%20608%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43337082%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22I%20don%27t%20think%20error%20is%20at%20the%20correct%20place.%20This%20should%20be%20logged%20if%20jwt%20is%20not%20of%20type%20JwtSecurityToken.%20For%20null%2C%20we%20can%20log%20a%20generic%20exception.%22%2C%20%22created_at%22%3A%20%222015-10-29T00%3A34%3A20Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22The%20error%20is%20meant%20to%20report%20that%20a%20SecurityToken%20other%20than%20a%20JwtSecurityToken%20was%20passed%20in.%20the%20Cast%20will%20return%20null%2C%20if%20the%20token%20is%20not%20derived%20from%20JwtSecuritytoken.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A00%3A02Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs%3AL587-629%22%7D%2C%20%22Pull%203ecb3cf2c14c623bec2d4f8f734817622e4138db%20src/System.IdentityModel.Tokens/SymmetricSignatureProvider.cs%2054%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43339206%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22The%20first%20argument%20should%20by%20key%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A06%3A23Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22I%20am%20trying%20to%20relate%20the%20algorithm%20to%20the%20key%20size.%20I%20think%20it%20would%20help%20users%20if%20the%20error%20message%20was%20modified.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A57%3A06Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20src/System.IdentityModel.Tokens/SymmetricSignatureProvider.cs%3AL61-87%22%7D%2C%20%22Pull%203ecb3cf2c14c623bec2d4f8f734817622e4138db%20src/System.IdentityModel.Tokens/RsaCryptoServiceProviderProxy.cs%2022%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43338815%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22delete%20LogMessages.IDX10507%20since%20that%20is%20not%20used%20now.%22%2C%20%22created_at%22%3A%20%222015-10-29T00%3A59%3A14Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22will%20do.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A54%3A10Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20src/System.IdentityModel.Tokens/RsaCryptoServiceProviderProxy.cs%3AL39-56%22%7D%2C%20%22Pull%203ecb3cf2c14c623bec2d4f8f734817622e4138db%20src/System.IdentityModel.Tokens.Jwt/JwtSecurityToken.cs%2072%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43334342%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22This%20check%20is%20not%20needed.%20The%20check%20below%20for%20string.IsNullorEmpty%20is%20good%20enough.%22%2C%20%22created_at%22%3A%20%222015-10-28T23%3A52%3A43Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22No%20it%20is%20not%2C%20because%20we%20don%27t%20hydrate%20the%20header%20from%20the%20string.%22%2C%20%22created_at%22%3A%20%222015-10-29T00%3A54%3A45Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%2C%20%7B%22body%22%3A%20%22Right.%20I%20did%20not%20notice%20the%20difference%20in%20the%20names.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A31%3A06Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20src/System.IdentityModel.Tokens.Jwt/JwtSecurityToken.cs%3AL84-111%22%7D%2C%20%22Pull%203ecb3cf2c14c623bec2d4f8f734817622e4138db%20src/Microsoft.IdentityModel.Logging/IdentityModelEventSource.cs%2057%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43330283%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22I%20don%27t%20think%20we%20need%202%20overrides%20of%20this%20method.%20We%20can%20modify%20the%20existing%20method%27s%20signature%20to%20%28string%20message%2C%20params%20object%5B%5D%20args%29.%20That%20way%2C%20we%20can%20call%20the%20method%20with%20or%20without%20args.%20All%20this%20variant%20is%20doing%20is%20formatting%20which%20we%20are%20doing%20in%20PrepareMessage%20already.%20We%20can%20have%20something%20like%20this%3A%5Cr%5Cn%20%20%20%20%20%20%20%20%5BEvent%283%2C%20Level%20%3D%20EventLevel.Warning%29%5D%5Cr%5Cn%20%20%20%20%20%20%20%20public%20void%20WriteWarning%28string%20message%2C%20params%20object%5B%5D%20args%29%5Cr%5Cn%20%20%20%20%20%20%20%20%7B%5Cr%5Cn%20%20%20%20%20%20%20%20%20%20%20%20if%20%28IsEnabled%28%29%20%26%26%20LogLevel%20%3E%3D%20EventLevel.Warning%29%5Cr%5Cn%20%20%20%20%20%20%20%20%20%20%20%20%7B%5Cr%5Cn%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20message%20%3D%20PrepareMessage%28EventLevel.Warning%2C%20message%2C%20args%29%3B%5Cr%5Cn%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20WriteEvent%283%2C%20message%29%3B%5Cr%5Cn%20%20%20%20%20%20%20%20%20%20%20%20%7D%5Cr%5Cn%20%20%20%20%20%20%20%20%7D%22%2C%20%22created_at%22%3A%20%222015-10-28T23%3A01%3A36Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22%40tushargupta51%20I%20though%20so%20also.%20However%2C%20the%20%27Event%27%20attribute%20imposes%20a%20very%20strict%20API.%22%2C%20%22created_at%22%3A%20%222015-10-28T23%3A04%3A06Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%2C%20%7B%22body%22%3A%20%22What%20does%20that%20mean%3F%20It%20gives%20any%20error%20if%20you%20try%20to%20do%20so%3F%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A44%3A42Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22Yes%20just%20try%20and%20code%20it%20up.%20It%20is%20kind%20of%20odd%2C%20caught%20me%20by%20surprise.%20I%20googled%20it%20and%20found%20the%20solution%20I%20implemented.%22%2C%20%22created_at%22%3A%20%222015-10-29T12%3A27%3A48Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%2C%20%7B%22body%22%3A%20%22Yeah%2C%20you%20are%20right.%20It%20seems%20that%20Event%20methods%20must%20match%20exactly%20the%20types%20of%20the%20WriteEvent%20overload%20it%20calls.%22%2C%20%22created_at%22%3A%20%222015-10-29T15%3A24%3A45Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20src/Microsoft.IdentityModel.Logging/IdentityModelEventSource.cs%3AL52-224%22%7D%2C%20%22Pull%203ecb3cf2c14c623bec2d4f8f734817622e4138db%20src/Microsoft.IdentityModel.Logging/LogHelper.cs%20113%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43331229%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22I%20don%27t%20think%20we%20need%20the%20formatting%20or%20the%20check%20here.%20We%20can%20just%20call%20IdentityModelEventSource.Logger.Write%28eventLevel%2C%20innerException%2C%20message%2C%20args%29%3B%20They%20are%20redundant%20here.%5Cr%5Cn%22%2C%20%22created_at%22%3A%20%222015-10-28T23%3A12%3A43Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22You%20are%20correct%2C%20I%20am%20being%20overly%20careful.%20Just%20in%20case%20the%20format%20string%20has%20params%20and%20the%20args%20is%20null%2C%20we%20would%20throw.%20This%20check%20sidesteps%20that%20possibility.%22%2C%20%22created_at%22%3A%20%222015-10-28T23%3A30%3A08Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%2C%20%7B%22body%22%3A%20%22This%20is%20not%20addressed.%20Where%20do%20we%20throw%20when%20args%20is%20null%3F%22%2C%20%22created_at%22%3A%20%222015-10-29T03%3A54%3A24Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22If%20the%20format%20string%20has%20%7B0%7D%20and%20args%20is%20null%2C%20the%20runtime%20will%20throw.%5Cr%5Cn%3E%20%20%20%20%20%20%20%20%20%20%20object%5B%5D%20args%20%3D%20null%3B%5Cr%5Cn%20%20%20%20%20%20%20%20%20%20%20%20try%5Cr%5Cn%20%20%20%20%20%20%20%20%20%20%20%20%7B%5Cr%5Cn%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20strFormatted%20%3D%20string.Format%28CultureInfo.InvariantCulture%2C%20%5C%22Null%20string%20format%3A%20%27%7B0%7D%27%2C%20%27%7B1%7D%27%5C%22%2C%20args%20%29%3B%5Cr%5Cn%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20Console.WriteLine%28%5C%22null%20args%20did%20NOT%20throw%5C%22%29%3B%5Cr%5Cn%20%20%20%20%20%20%20%20%20%20%20%20%7D%5Cr%5Cn%20%20%20%20%20%20%20%20%20%20%20%20catch%28Exception%20ex%29%5Cr%5Cn%20%20%20%20%20%20%20%20%20%20%20%20%7B%5Cr%5Cn%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20%20Console.WriteLine%28%5C%22null%20args%20threw%3A%20%27%7B0%7D%27%5C%22%2C%20ex%29%3B%5Cr%5Cn%20%20%20%20%20%20%20%20%20%20%20%20%7D%5Cr%5Cn%5Cr%5CnOutput%3A%5Cr%5Cn%3Enull%20args%20threw%3A%20%27System.ArgumentNullException%3A%20Value%20cannot%20be%20null.%5Cr%5Cn%3EParameter%20name%3A%20args%5Cr%5Cn%20%3E%20%20at%20System.String.Format%28IFormatProvider%20provider%2C%20String%20format%2C%20Object%5B%5D%20args%29%5Cr%5Cn%3E%20%20%20at%20CodeSnips.StringFormat.Run%28%29%20in%20C%3A%5C%5Cgithub%5C%5CAuthFactors%5C%5CCodeSnips%5C%5CStringFormat.cs%3Aline%2018%27%22%2C%20%22created_at%22%3A%20%222015-10-29T15%3A42%3A09Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%2C%20%7B%22body%22%3A%20%22But%20the%20following%20will%20not%20throw%3A%5Cr%5Cn%3Estring%20str%20%3D%20null%3B%5Cr%5Cn%5Cr%5Cn%3Evar%20strFormatted%20%3D%20string.Format%28CultureInfo.InvariantCulture%2C%20%5C%22Null%20string%20format%3A%20%27%7B0%7D%27%5C%22%2C%20str%29%3B%5Cr%5Cn%5Cr%5CnOutput%3A%5Cr%5Cn%3EString%20with%20null%20param%3A%20%27Null%20string%20format%3A%20%27%27%27.%5Cr%5Cn%22%2C%20%22created_at%22%3A%20%222015-10-29T15%3A44%3A46Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20src/Microsoft.IdentityModel.Logging/LogHelper.cs%3AL37-146%22%7D%2C%20%22Pull%203ecb3cf2c14c623bec2d4f8f734817622e4138db%20src/System.IdentityModel.Tokens/SymmetricSignatureProvider.cs%2072%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43339234%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22The%20order%20of%20parameters%20is%20incorrect.%20the%20first%20should%20be%20key.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A07%3A01Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22I%20also%20checked%20if%20algorithm%20is%20null.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A59%3A01Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20src/System.IdentityModel.Tokens/SymmetricSignatureProvider.cs%3AL61-87%22%7D%2C%20%22Pull%203ecb3cf2c14c623bec2d4f8f734817622e4138db%20src/Microsoft.IdentityModel.Logging/TextWriterEventListener.cs%2024%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43331698%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Why%20throwing%20ex%3F%20throw%20LogHelper.LogException%3CInvalidOperationException%3E%28ex%2C%20LogMessages.MIML11001%29%3B%20will%20throw%20the%20InvalidOperationException%20with%20the%20innerexception%20inside.%22%2C%20%22created_at%22%3A%20%222015-10-28T23%3A19%3A04Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22this%20one%20was%20tricky.%20because%20the%20exception%20is%20unknown.%20%20I%20basically%20wanted%20to%20log%20it%20and%20then%20re-%20throw.%22%2C%20%22created_at%22%3A%20%222015-10-28T23%3A26%3A29Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%2C%20%7B%22body%22%3A%20%22The%20inner%20exception%20would%20still%20be%20thrown%20when%20you%20throw%20the%20InvalidOperationException.%20We%20do%20that%20in%20ConfigurationManager%20too.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A29%3A21Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22LogHelper.LogException%20doesn%27t%20throw%2C%20and%20I%20wasn%27t%20convinced%20that%20InvalidOperationException%20would%20give%20a%20good%20message%20to%20the%20user.%20It%20was%20just%20a%20call%20that%20throwing%20the%20existing%20was%20probably%20as%20good%20as%20doing%20anything%20else.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A39%3A24Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%2C%20%7B%22body%22%3A%20%22Didn%27t%20notice%20that%20carefully.%20Why%20not%20use%20ex.GetType%28%29%20then%20when%20calling%20LogException%20method.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A49%3A30Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20src/Microsoft.IdentityModel.Logging/TextWriterEventListener.cs%3AL54-88%22%7D%2C%20%22Pull%203ecb3cf2c14c623bec2d4f8f734817622e4138db%20src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs%20421%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43336523%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22This%20is%20part%20of%20CanReadToken%20check.%20We%20can%20remove%20it%20from%20here.%22%2C%20%22created_at%22%3A%20%222015-10-29T00%3A24%3A02Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22I%20didn%27t%20do%20that%20because%20I%20wanted%20to%20throw%20the%20exact%20reason%20the%20Read%20failed.%22%2C%20%22created_at%22%3A%20%222015-10-29T00%3A56%3A56Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs%3AL416-496%22%7D%2C%20%22Pull%203ecb3cf2c14c623bec2d4f8f734817622e4138db%20src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs%20539%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43336962%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22does%20string.Format%20call%20the%20ToString%28%29%20method%20on%20non-string%20objects%3F%22%2C%20%22created_at%22%3A%20%222015-10-29T00%3A31%3A40Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22Yes%2C%20I%20wrote%20a%20test%20to%20validate.%22%2C%20%22created_at%22%3A%20%222015-10-29T00%3A57%3A42Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs%3AL497-508%22%7D%2C%20%22Pull%203ecb3cf2c14c623bec2d4f8f734817622e4138db%20src/System.IdentityModel.Tokens.Jwt/JwtSecurityToken.cs%20150%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43335099%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22This%20is%20related%20to%20my%20above%20comment%20about%20the%20JwtHeader%20ctors.%20Why%20we%20need%20to%20do%20this%20check%3F%22%2C%20%22created_at%22%3A%20%222015-10-29T00%3A02%3A09Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22Without%20the%20header%2C%20you%20don%27t%20have%20a%20well-formed%20Jwt.%20.%20%20I%20am%20not%20sure%20what%20you%20are%20driving%20at.%20This%20constructor%20requires%20a%20header%20and%20payload%20as%20before%2C%20all%20that%20have%20changed%20is%20the%20logging.%22%2C%20%22created_at%22%3A%20%222015-10-29T00%3A53%3A16Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%2C%20%7B%22body%22%3A%20%22I%20am%20referring%20to%20this%20check%3A%20signingCredentials%20%3D%3D%20null%20%3F%20Earlier%2C%20we%20were%20calling%20new%20JwtHeader%28signingCredentials%29%20without%20checking%20if%20its%20null%20or%20not.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A36%3A06Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22%40tushargupta51%20this%20constructor%20used%20to%20be%20internal.%20We%20should%20probably%20think%20about%20that%20again.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A43%3A58Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20src/System.IdentityModel.Tokens.Jwt/JwtSecurityToken.cs%3AL142-154%22%7D%2C%20%22Pull%203ecb3cf2c14c623bec2d4f8f734817622e4138db%20src/System.IdentityModel.Tokens/SecurityTokenDescriptor.cs%205%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43338941%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22How%20we%20can%20support%20multiple%20audiences%20with%20STD%3F%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A01%3A29Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22We%20don%27t.%20they%20will%20have%20to%20add%20claims.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A54%3A53Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20src/System.IdentityModel.Tokens/SecurityTokenDescriptor.cs%3AL33-51%22%7D%2C%20%22Pull%203ecb3cf2c14c623bec2d4f8f734817622e4138db%20src/Microsoft.IdentityModel.Logging/LogHelper.cs%2018%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43331101%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22use%20the%20LogMessages.IDX10000%20instead%20of%20the%20harcoded%20string%22%2C%20%22created_at%22%3A%20%222015-10-28T23%3A11%3A17Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22ok%2C%20but%20we%20will%20have%20to%20put%20it%20into%20Logging.%5Cr%5Cn%22%2C%20%22created_at%22%3A%20%222015-10-28T23%3A25%3A13Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%2C%20%7B%22body%22%3A%20%22Yeah%20right.%20There%20is%20one%20already%20in%20the%20LogMessages.%20It%20uses%20the%20prefix%20MIML...but%20thats%20ok.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A26%3A53Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/803796%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/tushargupta51%22%7D%7D%2C%20%7B%22body%22%3A%20%22that%20is%20why%20I%20hardcoded%20it%2C%20as%20I%20didn%27t%20want%20to%20upset%20all%20the%20tests.%22%2C%20%22created_at%22%3A%20%222015-10-29T01%3A36%3A57Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3172421%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/brentschmaltz%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20src/Microsoft.IdentityModel.Logging/LogHelper.cs%3AL37-146%22%7D%7D%2C%20%22processed%22%3A%20%5B%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23issuecomment-152248710%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43330283%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43330387%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43330540%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43330588%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43331101%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43331229%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43331698%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43332205%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43332308%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43332640%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43334342%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43334398%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43335006%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43335099%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43335933%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43336167%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43336492%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43336523%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43336962%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43337082%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43338116%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43338487%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43338579%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43338656%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43338663%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43338687%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43338735%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43338815%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43338849%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43338941%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43338994%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43339029%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43339102%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43339206%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43339234%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43340116%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43340174%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43340294%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43340368%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43340478%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43340513%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43340607%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43340623%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43340651%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43340704%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43340757%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43340804%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43340847%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43340940%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43340971%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43341165%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43341218%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43341231%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43341364%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43341397%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43341488%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43341565%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43346694%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43346749%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43347241%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43378272%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43378368%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43399305%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43402059%22%2C%20%22https%3A//github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300%23discussion_r43402456%22%5D%7D'></a>\n<a href='https://www.codereviewhub.com/'><img src='http://www.codereviewhub.com/site/github-bar.png' height=40></a>\n\n<img src='http://www.codereviewhub.com/site/github-approved-avatar.png'><a href='https://github.com/tushargupta51'><img src='https://avatars.githubusercontent.com/u/803796?v=3' width=34 height=34></a>\n\n- [ ] <a href='#crh-comment-Pull 3ecb3cf2c14c623bec2d4f8f734817622e4138db src/Microsoft.IdentityModel.Logging/IdentityModelEventSource.cs 57'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300#discussion_r43330283'>File: src/Microsoft.IdentityModel.Logging/IdentityModelEventSource.cs:L52-224</a></b>\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> I don't think we need 2 overrides of this method. We can modify the existing method's signature to (string message, params object[] args). That way, we can call the method with or without args. All this variant is doing is formatting which we are doing in PrepareMessage already. We can have something like this:\n[Event(3, Level = EventLevel.Warning)]\npublic void WriteWarning(string message, params object[] args)\n{\nif (IsEnabled() && LogLevel >= EventLevel.Warning)\n{\nmessage = PrepareMessage(EventLevel.Warning, message, args);\nWriteEvent(3, message);\n}\n}\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> @tushargupta51 I though so also. However, the 'Event' attribute imposes a very strict API.\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> What does that mean? It gives any error if you try to do so?\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> Yes just try and code it up. It is kind of odd, caught me by surprise. I googled it and found the solution I implemented.\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> Yeah, you are right. It seems that Event methods must match exactly the types of the WriteEvent overload it calls.\n- [ ] <a href='#crh-comment-Pull 3ecb3cf2c14c623bec2d4f8f734817622e4138db src/Microsoft.IdentityModel.Logging/IdentityModelEventSource.cs 205'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300#discussion_r43330387'>File: src/Microsoft.IdentityModel.Logging/IdentityModelEventSource.cs:L52-224</a></b>\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> Should this message be logged? If so, am not sure if error is the right level.\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> I choose error as I wanted to make sure the user would see the details.\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> But this message might not be as important as an \"error\" message. We are logging a message about unknown log level as an error. If a message comes with an unknown log level, we cannot assume that the message represents an error.\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> the point is it's an error mainly because the level was basically wrong. So I think it is fine.\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> I understand that and it makes sense to log that particular information - \"log level unknown\" as error. But logging the associate message as an error might be too much.\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> While, then users will not see the message, which I think will make it hard to track down where the error is.\n- [ ] <a href='#crh-comment-Pull 3ecb3cf2c14c623bec2d4f8f734817622e4138db src/Microsoft.IdentityModel.Logging/LogHelper.cs 18'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300#discussion_r43331101'>File: src/Microsoft.IdentityModel.Logging/LogHelper.cs:L37-146</a></b>\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> use the LogMessages.IDX10000 instead of the harcoded string\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> ok, but we will have to put it into Logging.\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> Yeah right. There is one already in the LogMessages. It uses the prefix MIML...but thats ok.\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> that is why I hardcoded it, as I didn't want to upset all the tests.\n- [ ] <a href='#crh-comment-Pull 3ecb3cf2c14c623bec2d4f8f734817622e4138db src/Microsoft.IdentityModel.Logging/LogHelper.cs 113'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300#discussion_r43331229'>File: src/Microsoft.IdentityModel.Logging/LogHelper.cs:L37-146</a></b>\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> I don't think we need the formatting or the check here. We can just call IdentityModelEventSource.Logger.Write(eventLevel, innerException, message, args); They are redundant here.\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> You are correct, I am being overly careful. Just in case the format string has params and the args is null, we would throw. This check sidesteps that possibility.\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> This is not addressed. Where do we throw when args is null?\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> If the format string has {0} and args is null, the runtime will throw.\n>           object[] args = null;\ntry\n{\nstrFormatted = string.Format(CultureInfo.InvariantCulture, \"Null string format: '{0}', '{1}'\", args );\nConsole.WriteLine(\"null args did NOT throw\");\n}\ncatch(Exception ex)\n{\nConsole.WriteLine(\"null args threw: '{0}'\", ex);\n}\nOutput:\n>null args threw: 'System.ArgumentNullException: Value cannot be null.\n>Parameter name: args\n>  at System.String.Format(IFormatProvider provider, String format, Object[] args)\n>   at CodeSnips.StringFormat.Run() in C:\\github\\AuthFactors\\CodeSnips\\StringFormat.cs:line 18'\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> But the following will not throw:\n>string str = null;\n>var strFormatted = string.Format(CultureInfo.InvariantCulture, \"Null string format: '{0}'\", str);\nOutput:\n>String with null param: 'Null string format: '''.\n- [ ] <a href='#crh-comment-Pull 3ecb3cf2c14c623bec2d4f8f734817622e4138db src/Microsoft.IdentityModel.Logging/TextWriterEventListener.cs 24'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300#discussion_r43331698'>File: src/Microsoft.IdentityModel.Logging/TextWriterEventListener.cs:L54-88</a></b>\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> Why throwing ex? throw LogHelper.LogException<InvalidOperationException>(ex, LogMessages.MIML11001); will throw the InvalidOperationException with the innerexception inside.\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> this one was tricky. because the exception is unknown.  I basically wanted to log it and then re- throw.\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> The inner exception would still be thrown when you throw the InvalidOperationException. We do that in ConfigurationManager too.\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> LogHelper.LogException doesn't throw, and I wasn't convinced that InvalidOperationException would give a good message to the user. It was just a call that throwing the existing was probably as good as doing anything else.\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> Didn't notice that carefully. Why not use ex.GetType() then when calling LogException method.\n- [ ] <a href='#crh-comment-Pull 3ecb3cf2c14c623bec2d4f8f734817622e4138db src/System.IdentityModel.Tokens.Jwt/JwtSecurityToken.cs 72'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300#discussion_r43334342'>File: src/System.IdentityModel.Tokens.Jwt/JwtSecurityToken.cs:L84-111</a></b>\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> This check is not needed. The check below for string.IsNullorEmpty is good enough.\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> No it is not, because we don't hydrate the header from the string.\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> Right. I did not notice the difference in the names.\n- [ ] <a href='#crh-comment-Pull 3ecb3cf2c14c623bec2d4f8f734817622e4138db src/System.IdentityModel.Tokens.Jwt/JwtSecurityToken.cs 95'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300#discussion_r43334398'>File: src/System.IdentityModel.Tokens.Jwt/JwtSecurityToken.cs:L84-111</a></b>\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> this check can be made string.IsNullOrEmpty.\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> true we have that sprinkled through-out our code. But I choose not to clean it all up now.\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> Since we are touching the code in this PR, we might as well change this one. WhiteSpace or empty signatures does not make any sense.\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> I would like to do it in one pass, similar to how the Logging was changed. then we know we got it all. Just do a search and replace, piece meal makes no sense seeing how we have to go back.\n- [ ] <a href='#crh-comment-Pull 3ecb3cf2c14c623bec2d4f8f734817622e4138db src/System.IdentityModel.Tokens.Jwt/JwtHeader.cs 50'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300#discussion_r43335006'>File: src/System.IdentityModel.Tokens.Jwt/JwtHeader.cs:L31-74</a></b>\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> There is some discrepancy in 2 constructors. The result of new JwtHeader() and new JwtHeader(null) will be different. That does not look right. Whatever we are doing when signingCredentials is null, shouldnt we that in the default ctor too?\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> the other constructor calls this(null)\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> I noticed that. But my point is why there is a difference. The result of new JwtHeader() and new JwtHeader(null) should be same.\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> I think they are, I will add a test just to be sure.\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> No, they are not. In case of JwtHeader(null), Alg will have None value and Typ is initialized to JwtConstants.HeaderType\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> Ah, my bad. They are equal. Test was not needed. I read that as base(null) and that's why thinking they will result in different dictionaries, didn't notice the ctor was chained. Sorry for the trouble.\n- [ ] <a href='#crh-comment-Pull 3ecb3cf2c14c623bec2d4f8f734817622e4138db src/System.IdentityModel.Tokens.Jwt/JwtSecurityToken.cs 150'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300#discussion_r43335099'>File: src/System.IdentityModel.Tokens.Jwt/JwtSecurityToken.cs:L142-154</a></b>\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> This is related to my above comment about the JwtHeader ctors. Why we need to do this check?\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> Without the header, you don't have a well-formed Jwt. .  I am not sure what you are driving at. This constructor requires a header and payload as before, all that have changed is the logging.\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> I am referring to this check: signingCredentials == null ? Earlier, we were calling new JwtHeader(signingCredentials) without checking if its null or not.\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> @tushargupta51 this constructor used to be internal. We should probably think about that again.\n- [ ] <a href='#crh-comment-Pull 3ecb3cf2c14c623bec2d4f8f734817622e4138db src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs 278'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300#discussion_r43335933'>File: src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs:L273-284</a></b>\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> In regex below:\nregex = new Regex(JwtConstants.JsonCompactSerializationRegex, RegexOptions.None, TimeSpan.FromSeconds(2))\nset the default to 100 ms as you set for the other one.\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> We will handle that in RC2.\nActually, while I am making changes for other items you found, I will touch that up to 100ms.\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> It will be inconsistent to allow 100ms (JwtSecurityToken constructor) at one place and 2 seconds (CanReadToken method) at another. We should either change both or none.\n- [ ] <a href='#crh-comment-Pull 3ecb3cf2c14c623bec2d4f8f734817622e4138db src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs 361'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300#discussion_r43336167'>File: src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs:L344-403</a></b>\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> nit: I don't think we need the boolean checks for other values here. We are going to check them again in the body. It could be just if (SetDefaultTimesOnTokenCreation ) { ... }\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> true enough, I will adjust the code.\n- [ ] <a href='#crh-comment-Pull 3ecb3cf2c14c623bec2d4f8f734817622e4138db src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs 429'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300#discussion_r43336492'>File: src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs:L416-496</a></b>\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> this can be replaced by return new JwtSecurityToken(token);\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> I didn't do that because, I didn't want to do all the checks again in the ctor of JwtSecurityToken.\n- [ ] <a href='#crh-comment-Pull 3ecb3cf2c14c623bec2d4f8f734817622e4138db src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs 421'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300#discussion_r43336523'>File: src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs:L416-496</a></b>\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> This is part of CanReadToken check. We can remove it from here.\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> I didn't do that because I wanted to throw the exact reason the Read failed.\n- [ ] <a href='#crh-comment-Pull 3ecb3cf2c14c623bec2d4f8f734817622e4138db src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs 539'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300#discussion_r43336962'>File: src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs:L497-508</a></b>\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> does string.Format call the ToString() method on non-string objects?\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> Yes, I wrote a test to validate.\n- [ ] <a href='#crh-comment-Pull 3ecb3cf2c14c623bec2d4f8f734817622e4138db src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs 608'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300#discussion_r43337082'>File: src/System.IdentityModel.Tokens.Jwt/JwtSecurityTokenHandler.cs:L587-629</a></b>\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> I don't think error is at the correct place. This should be logged if jwt is not of type JwtSecurityToken. For null, we can log a generic exception.\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> The error is meant to report that a SecurityToken other than a JwtSecurityToken was passed in. the Cast will return null, if the token is not derived from JwtSecuritytoken.\n- [ ] <a href='#crh-comment-Pull 3ecb3cf2c14c623bec2d4f8f734817622e4138db src/System.IdentityModel.Tokens/Base64UrlEncoder.cs 57'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300#discussion_r43338663'>File: src/System.IdentityModel.Tokens/Base64UrlEncoder.cs:L133-140</a></b>\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> move this to LogMessages.cs\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> sure\n- [ ] <a href='#crh-comment-Pull 3ecb3cf2c14c623bec2d4f8f734817622e4138db src/System.IdentityModel.Tokens/RsaCryptoServiceProviderProxy.cs 22'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300#discussion_r43338815'>File: src/System.IdentityModel.Tokens/RsaCryptoServiceProviderProxy.cs:L39-56</a></b>\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> delete LogMessages.IDX10507 since that is not used now.\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> will do.\n- [ ] <a href='#crh-comment-Pull 3ecb3cf2c14c623bec2d4f8f734817622e4138db src/System.IdentityModel.Tokens/SecurityTokenDescriptor.cs 5'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300#discussion_r43338941'>File: src/System.IdentityModel.Tokens/SecurityTokenDescriptor.cs:L33-51</a></b>\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> How we can support multiple audiences with STD?\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> We don't. they will have to add claims.\n- [ ] <a href='#crh-comment-Pull 3ecb3cf2c14c623bec2d4f8f734817622e4138db src/System.IdentityModel.Tokens/SymmetricSignatureProvider.cs 54'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300#discussion_r43339206'>File: src/System.IdentityModel.Tokens/SymmetricSignatureProvider.cs:L61-87</a></b>\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> The first argument should by key\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> I am trying to relate the algorithm to the key size. I think it would help users if the error message was modified.\n- [ ] <a href='#crh-comment-Pull 3ecb3cf2c14c623bec2d4f8f734817622e4138db src/System.IdentityModel.Tokens/SymmetricSignatureProvider.cs 72'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300#discussion_r43339234'>File: src/System.IdentityModel.Tokens/SymmetricSignatureProvider.cs:L61-87</a></b>\n- <a href='https://github.com/tushargupta51'><img border=0 src='https://avatars.githubusercontent.com/u/803796?v=3' height=16 width=16'></a> The order of parameters is incorrect. the first should be key.\n- <a href='https://github.com/brentschmaltz'><img border=0 src='https://avatars.githubusercontent.com/u/3172421?v=3' height=16 width=16'></a> I also checked if algorithm is null.\n\n\n<a href='https://www.codereviewhub.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300?mark_as_completed=1'><img src='http://www.codereviewhub.com/site/github-mark-as-completed.png' height=26></a>&nbsp;<a href='https://www.codereviewhub.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300?approve=1'><img src='http://www.codereviewhub.com/site/github-approve.png' height=26></a>&nbsp;<a href='https://www.codereviewhub.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300?approve=0'><img src='http://www.codereviewhub.com/site/github-undo-approve.png' height=26></a>&nbsp;<a href='https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300'><img src='http://www.codereviewhub.com/site/github-refresh.png' height=26></a>\n<a href='#crh-end'></a>"},"comment":{"url":"https://api.github.com/repos/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/comments/157893953","html_url":"https://github.com/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/pull/300#issuecomment-157893953","issue_url":"https://api.github.com/repos/AzureAD/azure-activedirectory-identitymodel-extensions-for-dotnet/issues/300","id":157893953,"user":{"login":"PinpointTownes","id":6998306,"avatar_url":"https://avatars.githubusercontent.com/u/6998306?v=3","gravatar_id":"","url":"https://api.github.com/users/PinpointTownes","html_url":"https://github.com/PinpointTownes","followers_url":"https://api.github.com/users/PinpointTownes/followers","following_url":"https://api.github.com/users/PinpointTownes/following{/other_user}","gists_url":"https://api.github.com/users/PinpointTownes/gists{/gist_id}","starred_url":"https://api.github.com/users/PinpointTownes/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/PinpointTownes/subscriptions","organizations_url":"https://api.github.com/users/PinpointTownes/orgs","repos_url":"https://api.github.com/users/PinpointTownes/repos","events_url":"https://api.github.com/users/PinpointTownes/events{/privacy}","received_events_url":"https://api.github.com/users/PinpointTownes/received_events","type":"User","site_admin":false},"created_at":"2015-11-18T23:07:31Z","updated_at":"2015-11-18T23:07:31Z","body":"Ooops, looks like you missed my question about the CLA and the mapping properties :fearful: "}}