Add telemetry to the authorization and signout message redirect urls,… #529
Conversation
| /// </summary> | ||
| private void EnsureTelemetryValues() | ||
| { | ||
| if (DisableTelemetryParameters) |
There was a problem hiding this comment.
Do we need to remove these parameters? If we called Clone() before EnsureTelemetryValues, only values set by the runtime OR user would be in the message. Adding to the message would be on an instance temporary message. If we make this change, the property could be named: AddTelemetryParameters.
There was a problem hiding this comment.
Yes, if you were guaranteed that Clone was always called before EnsureTelemetryValues. My concern is that, in some future iteration of the code, someone changes the flow to not call Clone first, and then they expect that setting DisableTelemetryValues to true, and then to false, would remove them again. I can see your point though; if you think we shouldn't set up the code to ever remove the parameters, I'm fine with changing it.
| @@ -137,12 +139,25 @@ public string CreateAuthenticationRequestUrl() | |||
| /// <returns>The uri to use for a redirect.</returns> | |||
| public string CreateLogoutRequestUrl() | |||
| { | |||
| EnsureTelemetryValues(); | |||
There was a problem hiding this comment.
You may want to consider adding telemetry on the cloned message. Otherwise turning it off, would result in the second message having the parameters.
polita
force-pushed
the
polita/498
branch
2 times, most recently
from
eaa7f2b to
0e898de
Compare
|
@brentschmaltz The PR is updated with your suggestions. Thanks... |
polita
force-pushed
the
polita/498
branch
2 times, most recently
from
fc30226 to
cd8b274
Compare
|
@brentschmaltz Can you CR? |
|
Updated with CR feedback from 5x change. |
| /// <summary> | ||
| /// Gets or sets whether parameters for the library and version are sent on the query string for all instances of <see cref="OpenIdConnectMessage"/>. | ||
| /// </summary> | ||
| public static bool EnableTelemetryParametersByDefault = true; |
There was a problem hiding this comment.
Could you make this a property also :-)
… along with a way to turn it off Integrate CR feedback Update unit test for code change Update with CR suggestion to add telemetry on the newly-cloned message Update with CR suggestion Update PR with changes to the design to allow a static switch to turn telemetry off universally Use the same reflection code we use for .NET Core Update syntax with feedback from CR Update with feedback from CR
|
@brentschmaltz Updated with the same property pattern from 5x using pre-C# 6.0 syntax. |
|
|
… along with a way to turn it off.
Also added tests for the telemetry parameters and the switch to turn it off.