Add ctor in CallerShouldAuditAttribute class

[RodgeFu]

Add constructors into the CallerShouldAuditAttribute class so that we can use them directly when generating management plain SDK code like [CallerShouldAudit("some url with detail info")].

Generating attribute code using properties like [CallerShouldAudit(Reason = "...")] hasn't been supported in the mgmt plain codegen now and it will be much higher cost to change mgmt plain codegen to add that support.

Thanks.

[jsquire]

LGTM

[pallavit]

Generating attribute code using properties like [CallerShouldAudit(Reason = "...")] hasn't been supported in the mgmt plain codegen now and it will be much higher cost to change mgmt plain codegen to add that support.

Is this something we ultimately want codegen to support? If so, do we have a tracking bug to support it? My worry is we would not know the reason why we are adding the audit attribute and in theory this overload can be used in both dataplane as well as mgmt plane SDKs now.

[annelo-msft]

@RodgeFu please address @pallavit's comment #40548 (comment) before merging.

[annelo-msft]

It would be good to get @tg-msft's feedback on this change as well, since he's been driving it on the data-plane side and may want to share a perspective from that viewpoint.

[ArthurMa1978]

Generating attribute code using properties like [CallerShouldAudit(Reason = "...")] hasn't been supported in the mgmt plain codegen now and it will be much higher cost to change mgmt plain codegen to add that support.

Is this something we ultimately want codegen to support? If so, do we have a tracking bug to support it? My worry is we would not know the reason why we are adding the audit attribute and in theory this overload can be used in both dataplane as well as mgmt plane SDKs now.

This is part of the security ask from Azure Trusted Platform and Azure Security Monitoring team: Azure/azure-sdk#5589.
Adn Ted already had a fix for Storage #39345.

[RodgeFu]

Generating attribute code using properties like [CallerShouldAudit(Reason = "...")] hasn't been supported in the mgmt plain codegen now and it will be much higher cost to change mgmt plain codegen to add that support.

Is this something we ultimately want codegen to support? If so, do we have a tracking bug to support it? My worry is we would not know the reason why we are adding the audit attribute and in theory this overload can be used in both dataplane as well as mgmt plane SDKs now.

@pallavit , The added ctor doesnt change the current behavior of CallerShouldAuditAttribute. Without the change, the attribute can still be used without giving any reason like [CallerShouldAudit]. It's because when no ctor is defined explicitly in a class, c# would add a parameterless ctor to the class by default. If you want to ensure a reason is given when using the attribute, we need to add a ctor with reason parameter and remove other ctor. But I think all the current usage of this attribute will be broken by this change and need to be updated to use the new ctor.

Actually I think it's suggested that at least one ctor should be defined in class explicitly to avoid potential unexpected build break in the future. Because when a class is defined without ctor, the default parameterless ctor will be used implicitly. but in the future, if someone add a ctor to that class with parameters for some reason, c# won't add the default parameterless ctor anymore which would break all the usages which expecting a parameterless ctor. And it's hard for people to be aware that he's making a breaking change because he's just adding a method especially when the class is used in separated libraries

[pallavit]

Thanks @RodgeFu I missed the fact there were no constructors defined previously. Thanks for the reply. LGTM.

[RodgeFu]

Thanks @pallavit .

@tg-msft , please let me know if you have more comments. I will try to merge the PR tomorrow if there is no more comment. thanks.

[tg-msft]

There's no reason to do this for the sake of versioning. In the future you'd just add the parameterless .ctor yourself.

I'm fine making the change if the generator isn't able to use attribute properties, but please file a bug to fix that.

I'm more concerned with the plan forward here. For data plane, we've only added this attribute to libraries that were authored by hand. We will not be adding it to anything generated. Instead, we've asked the Security folks to drive discussions with the TypeSpec team to get a decorator added there which we can use when generating all languages (i.e., Java should have an annotation for this, TypeScript should have a structured doc comment, etc.).

I think management plane should wait for the same thing. Your team shouldn't be deciding when to apply this attribute long term. It should be service teams when they're authoring their TypeSpec.

[heaths]

The added ctor doesnt change the current behavior of CallerShouldAuditAttribute.

If the goal was always to provide a reason for our libraries, then perhaps it's worth the relatively little effort now to make the ctor with a required (not nullable) reason the only ctor.

I've been talking with the security folks, and are discussing making this attribute a standard protocol - a shared source attribute across multiple services and toolsets. It doesn't behoove them to support different attributes for the same thing across different repos, and they agreed we'll set the standard here. So if a reason (docs) should always accompany the attribute, let's make it required.

Conceptually, this is no different than a suppression attribute that requires a justification. The attribute itself doesn't, but there's a Roslyn analyzer - which is enabled in the .NET and Azure SDK for .NET repos (we copy .NET's, with only one unrelated change) - to make Justification required. We have the opportunity here now to not require a separate analyzer to accomplish the same thing. The use case for this attribute is more targeted, so I think it's warranted.

[RodgeFu]

Agree with @heaths that we can make the Reason required now if it should be to avoid potential trouble in the future. I can update the PR to make the change, but I dont have much background about the Attribute. Could anyone help to confirm that's what we should do to the attribute? thanks.

@tg-msft , we are not making the decision for service team to add the attribute. we are just adding the support in our codegen to be able to include the attribute when generating code when corresponding directive (or the TypeSpec decorator you mentioned in the future) is defined. It will be helpful if you could also include us in the discussion about the decorator. thanks

[heaths]

The attribute will be used by (likely) a Roslyn analyzer they already have to raise warnings (or warnings as errors) during compilation requiring audits. The reason could be elevated into the warning message so callers can click to view more information about why they are seeing a warning, and potentially what they should look out for when auditing usage of these operations.

[RodgeFu]

merge this PR first to unblock mgmt codegen part. I would make another PR to make the Reason required if no one against it. Thanks.

[annelo-msft]

If this affects APIs used externally, would it be possible to add an entry to the CHANGELOG to indicate the change?

[heaths]

If this affects APIs used externally, would it be possible to add an entry to the CHANGELOG to indicate the change?

@annelo-msft it doesn't. It's an internal attribute.

[RodgeFu]

I tried to create a PR to make the Reason required at #40682 but seems the Attribute has been released in some data plane SDK and there are compatibility issues reported in the check. I am not familiar with data plain, so @tg-msft, could you help to double check how these issues should be handled? thanks.