Skip to content
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.

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

Feature: support automate function input secrets in C# SDK #3324

Merged
merged 5 commits into from
May 2, 2024

Conversation

cdriesler
Copy link
Member

@cdriesler cdriesler commented Apr 30, 2024

Description & motivation

Automate functions can flag certain inputs as secrets. This will guarantee that their values are redacted in the UI and in any logging during execution. In order for an input to be considered a secret, its JSONSchema property must have writeOnly specified as true.

In any given language's SDK, we generate Automate JSONSchemas from a class within the source code. In Python, this is currently done with a pydantic class SecretStr, but the built in DataAnnotations we use in C# do not provide a way to generate writeOnly.

For "extension" cases like this, Newtonsoft provides the ability to implement and attach our own serialization logic, and so we've reached for that to support secrets in the C# SDK.

Changes:

  • Adds SecretAttribute annotation to the C# SDK
  • Checks for its presence during a custom JSchemaGenerationProvider and sets writeOnly to true on the generated (in-flight generation) schema

Validation of changes:

  • Used in this branch of the SpeckleAutomateDotnetExample and verified generated schema includes writeOnly
  • We do not currently test this logic. Am happy to extract it/add some tests as a prerequisite for this PR but wanted to avoid a change-and-refactor situation.

References

@cdriesler cdriesler marked this pull request as ready for review April 30, 2024 11:16
@cdriesler cdriesler requested a review from BovineOx April 30, 2024 11:16
@gjedlicska gjedlicska self-requested a review April 30, 2024 11:46
Copy link
Contributor

@gjedlicska gjedlicska left a comment

Choose a reason for hiding this comment

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

lgtm

@AlanRynne
Copy link
Contributor

Hey @cdriesler! I noticed your PR is failing on the formatting checks, not sure if we've run you through the setup yet but maybe we should.

Meanwhile, you can just:

  • dotnet tools restore
  • dotnet csharpier ./

Which should run the right formatter for you ;)

@cdriesler
Copy link
Member Author

@AlanRynne ty! that did it. will definitely be finding you again for setup tips

@gjedlicska I had to make some changes for formatting but also for a strange typing on JSchemaGenerationProvider. (The #pragma bit). Re-requested review.

@cdriesler cdriesler requested a review from gjedlicska April 30, 2024 15:53
@JR-Morgan JR-Morgan self-requested a review May 2, 2024 11:06
@AlanRynne
Copy link
Contributor

Has the timeline to release this been discussed? Should it go into main and be hotfixed?

We're already days away from releasing 2.19 so if there is no 🔥 to put out let's target this into dev instead.

@JR-Morgan JR-Morgan force-pushed the WEB-850-Automate-SDK-secrets-handling branch from f9fb805 to 8fe01de Compare May 2, 2024 11:35
{
// `GetSchema` returning `null` indicates that the given type should not have a customised schema
// Nullability of JSchemaTypeGenerationContext appears to be incorrect.
#pragma warning disable CS8764 // Nullability of return type doesn't match overridden member (possibly because of nullability attributes).
Copy link
Member

Choose a reason for hiding this comment

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

I've opened an issue in the Netwonsoft repo, as the nullability of JSchemaGenerationProvider.GetSchema appears incorrect JamesNK/Newtonsoft.Json.Schema#341

@JR-Morgan JR-Morgan changed the base branch from main to dev May 2, 2024 11:47
@JR-Morgan JR-Morgan merged commit 9854cdc into dev May 2, 2024
32 checks passed
@JR-Morgan JR-Morgan deleted the WEB-850-Automate-SDK-secrets-handling branch May 2, 2024 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants