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

Allow resource properties to be marked as secret #5380

Merged
merged 18 commits into from
Sep 23, 2024

Conversation

drewnoakes
Copy link
Member

@drewnoakes drewnoakes commented Aug 22, 2024

Fixes #5065
Fixes #5377

This change allows selected resource properties in the details view to support masking, and show as masked by default. This behaviour exists today for all environment variables, and has so-far not been available for resource properties.

The resource service specifies which properties may be "sensitive", and the UI shows them masked.

Currently, the app host hides container/executable command line arguments, but it can be amended to include other properties too. Other resource services (like ACA's) can customise this logic to their needs as well.

As part of this change, we create interface IPropertyGridItem that view models must implement to be bound to the PropertyGrid component. This allows the component to work directly with the items, rather than calling back to the component's user via parameters for callback logic that largely do the same thing.

We also avoid having the ResourceDetails code create copies of ResourcePropertyViewModel objects for binding the UI. This copying won't work with the new IPropertyGridItem as the PropertyGrid requires these to be the same mutable object over time. Removing this fixed the inability to unmask resource property values.

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?
    • Yes
      • Link to aspire-docs issue:
    • No
Microsoft Reviewers: Open in CodeFlow

@JamesNK

This comment was marked as outdated.

@mitchdenny

This comment was marked as outdated.

@drewnoakes

This comment was marked as outdated.

@davidfowl
Copy link
Member

@drewnoakes This is close to being done right?

@drewnoakes
Copy link
Member Author

It's close, but blocked on the (minor) bug mentioned above. The cause seems to be a UI update loop that prevents the popup from opening. I haven't made time to investigate yet, given other priorities, but will come back to it soon.

@drewnoakes
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@drewnoakes
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@davidfowl
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@drewnoakes
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@drewnoakes drewnoakes force-pushed the drnoakes/secret-resource-properties branch from a506f6a to e8a5be9 Compare September 23, 2024 00:34
@dotnet-policy-service dotnet-policy-service bot added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Sep 23, 2024
@drewnoakes drewnoakes enabled auto-merge (squash) September 23, 2024 09:43
@drewnoakes
Copy link
Member Author

@JamesNK please re-review and both @davidfowl and I are waiting to build on this for other work (e.g. #4974).

@drewnoakes drewnoakes merged commit aee5a84 into dotnet:main Sep 23, 2024
11 checks passed
@drewnoakes drewnoakes deleted the drnoakes/secret-resource-properties branch September 23, 2024 13:06
@github-actions github-actions bot locked and limited conversation to collaborators Oct 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent sort behavior in Resource Details view Allow resource properties to be marked as secret
4 participants