Skip to content

Fix the issue that string is returned instead of object and add one more property in the telemetry #415

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 11, 2024
Merged

Fix the issue that string is returned instead of object and add one more property in the telemetry #415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions src/Common/AzurePSCmdlet.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ private IOutputSanitizer OutputSanitizer
{
get
{
if (AzureSession.Instance != null && AzureSession.Instance.TryGetComponent<IOutputSanitizer>(nameof(IOutputSanitizer), out var outputSanitizer))
if (AzureSession.Instance.TryGetComponent<IOutputSanitizer>(nameof(IOutputSanitizer), out var outputSanitizer))
return outputSanitizer;

return null;
Expand Down Expand Up @@ -407,11 +407,15 @@ private void WriteSecretsWarningMessage()
if (_qosEvent?.SanitizerInfo != null)
{
var sanitizerInfo = _qosEvent.SanitizerInfo;
if (sanitizerInfo.ShowSecretsWarning)
if (sanitizerInfo.ShowSecretsWarning && sanitizerInfo.SecretsDetected)
{
if (sanitizerInfo.DetectedProperties?.Count > 0)
if (sanitizerInfo.DetectedProperties.Count == 0)
{
WriteWarning(string.Format(Resources.DisplaySecretsWarningMessage, MyInvocation.InvocationName, string.Join(", ", sanitizerInfo.DetectedProperties)));
WriteWarning(string.Format(Resources.DisplaySecretsWarningMessageWithoutProperty, MyInvocation.InvocationName));
}
else
{
WriteWarning(string.Format(Resources.DisplaySecretsWarningMessageWithProperty, MyInvocation.InvocationName, string.Join(", ", sanitizerInfo.DetectedProperties)));
}
}
}
Expand Down Expand Up @@ -544,7 +548,7 @@ protected void WriteSurvey()

private void SanitizeOutput(object sendToPipeline)
{
if (OutputSanitizer != null && OutputSanitizer.RequireSecretsDetection)
if (OutputSanitizer?.RequireSecretsDetection == true)
{
OutputSanitizer.Sanitize(sendToPipeline, out var telemetry);
_qosEvent?.SanitizerInfo.Combine(telemetry);
Expand Down Expand Up @@ -767,7 +771,7 @@ protected virtual void InitializeQosEvent()
}
}

_qosEvent.SanitizerInfo = new SanitizerTelemetry();
_qosEvent.SanitizerInfo = new SanitizerTelemetry(OutputSanitizer?.RequireSecretsDetection == true);
}

private void RecordDebugMessages()
Expand Down
7 changes: 4 additions & 3 deletions src/Common/MetricHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,11 +475,12 @@ private void PopulateSanitizerPropertiesFromQos(AzurePSQoSEvent qos, IDictionary
eventProperties["secrets-warning"] = qos.SanitizerInfo.ShowSecretsWarning.ToString();
if (qos.SanitizerInfo.ShowSecretsWarning)
{
bool secretsDetected = qos.SanitizerInfo.DetectedProperties.Count > 0;
bool secretsDetected = qos.SanitizerInfo.SecretsDetected;
eventProperties["secrets-detected"] = secretsDetected.ToString();
if (secretsDetected)
{
eventProperties.Add("secrets-detected-properties", string.Join(";", qos.SanitizerInfo.DetectedProperties));
var detectedProperties = qos.SanitizerInfo.DetectedProperties.Count == 0 ? "[None]" : string.Join(";", qos.SanitizerInfo.DetectedProperties);
eventProperties.Add("secrets-detected-properties", detectedProperties);
}
if (qos.SanitizerInfo.HasErrorInDetection && qos.SanitizerInfo.DetectionError != null)
{
Expand Down Expand Up @@ -706,7 +707,7 @@ public override string ToString()

sb.Append($"; IsSuccess: {IsSuccess}; Duration: {Duration}");

if (SanitizerInfo?.ShowSecretsWarning == true)
if (SanitizerInfo.ShowSecretsWarning)
{
sb.Append($"; SanitizeDuration: {SanitizerInfo.SanitizeDuration}");
}
Expand Down
13 changes: 11 additions & 2 deletions src/Common/Properties/Resources.Designer.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion src/Common/Properties/Resources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1760,7 +1760,10 @@ Note : Go to {0} for steps to suppress this breaking change warning, and other i
<data name="SurveyPreface" xml:space="preserve">
<value>[Survey] Help us improve Azure PowerShell by sharing your experience. This survey should take about 5 minutes. Run 'Open-AzSurveyLink' to open in browser. Learn more at {0}</value>
</data>
<data name="DisplaySecretsWarningMessage" xml:space="preserve">
<data name="DisplaySecretsWarningMessageWithProperty" xml:space="preserve">
<value>The output of cmdlet {0} may compromise security by showing the following secrets: {1}. Learn more at https://go.microsoft.com/fwlink/?linkid=2258844</value>
</data>
<data name="DisplaySecretsWarningMessageWithoutProperty" xml:space="preserve">
<value>The output of cmdlet {0} may compromise security by showing secrets. Learn more at https://go.microsoft.com/fwlink/?linkid=2258844</value>
</data>
</root>
8 changes: 8 additions & 0 deletions src/Common/Sanitizer/SanitizerTelemetry.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ public class SanitizerTelemetry
{
public bool ShowSecretsWarning { get; set; } = false;

public bool SecretsDetected { get; set; } = false;

public HashSet<string> DetectedProperties { get; set; } = new HashSet<string>();

public bool HasErrorInDetection { get; set; } = false;
Expand All @@ -29,11 +31,17 @@ public class SanitizerTelemetry

public TimeSpan SanitizeDuration { get; set; }

public SanitizerTelemetry(bool showSecretsWarning)
Copy link
Member

@isra-fel isra-fel Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is technically a breaking change, because the default constructor (the one without parameters) will be hidden by this. Are we sure about the backward compatibility?

Copy link
Contributor Author

@vidai-msft vidai-msft Apr 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change was due to the value in the telemetry may be not consistent with the DisplaySecretsWarning option. Previously the ShowSecretsWarning in the telemetry relied on if the real detection method was invoked. If not, then the value would be the default (false). However, the real value of DisplaySecretsWarning may be true. The initialization of the SanitizerTelemetry just happens in Az.Accounts module (For SDK, Authentication proj; for AutoRest, AzModule). So it won't cause any real breaking change and build error.

{
ShowSecretsWarning = showSecretsWarning;
}

public void Combine(SanitizerTelemetry telemetry)
{
if (telemetry != null)
{
ShowSecretsWarning = ShowSecretsWarning || telemetry.ShowSecretsWarning;
SecretsDetected = SecretsDetected || telemetry.SecretsDetected;
DetectedProperties.UnionWith(telemetry.DetectedProperties);
HasErrorInDetection = HasErrorInDetection || telemetry.HasErrorInDetection;
DetectionError = DetectionError ?? telemetry.DetectionError;
Expand Down