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

Fixup automatically created feed permissions #8037

Merged
merged 6 commits into from
Oct 20, 2021

Conversation

mmitche
Copy link
Member

@mmitche mmitche commented Oct 13, 2021

Tighten up the feed permissions used when creating new feeds on the fly, or when publishing signed assets, and do a bit of refactoring in the process.

  • Put internal feeds into the internal project, rather than the organization. This is where they should have been from the beginning. There was mainly confusion about project vs. org scoped feeds when this was created.
  • Tweak the default feed permissions so that:
    • Members of dnceng get read perms on the local view (this avoids an issue where we used to have to add people as Contributors). This is done by patching the local feed view after feed creation.
    • Admins get ownership
    • the internal and collection build service accounts get write access
  • Added some nice NYI exceptions that should fire if we ever change the organization or project that we are running in.
  • Update references to the task, in PublishSignedAssets.proj and SetupTargetFeedConfigV3, to use the new task parameters.

Refactoring:

  • Remove SetupTargetFeeds.proj (v2 support), which is unused. I chose to do this because it also contain a reference to CreateAzureDevOpsFeed
  • Set the default publishing version to 3
  • Remove IsInternal as a feed task creation parameter and replace it with the AzDO project. The AzDO project defines the visibility, and this cleans up a little of the dnceng-isms and enables moving them into some helper methods
  • Add helper methods for determining some aspects of the feed names
  • Refactor some variable names to make them clearer

To double check:

Tighten up the feed permissions used when creating new feeds on the fly, or when publishing signed assets, and do a bit of refactoring in the process.
- Put internal feeds into the internal project, rather than the organization. This is where they should have been from the beginning. There was mainly confusion about project vs. org scoped feeds when this was created.
- Tweak the default feed permissions so that:
  - Members of dnceng get read perms on the local view (this avoids an issue where we used to have to add people as Contributors). This is done by patching the local feed view after feed creation.
  - Admins get ownership
  - the internal and collection build service accounts get write access
- Added some nice NYI exceptions that should fire if we ever change the organization or project that we are running in.
- Update references to the task, in PublishSignedAssets.proj and SetupTargetFeedConfigV3, to use the new task parameters.

Refactoring:
- Remove SetupTargetFeeds.proj (v2 support), which is unused. I chose to do this because it also contain a reference to CreateAzureDevOpsFeed
- Set the default publishing version to 3
- Remove IsInternal as a feed task creation parameter and replace it with the AzDO project. The AzDO project defines the visibility, and this cleans up a little of the dnceng-isms and enables moving them into some helper methods
- Add helper methods for determining some aspects of the feed names
- Refactor some variable names to make them clearer
@mmitche
Copy link
Member Author

mmitche commented Oct 13, 2021

@mmitche mmitche changed the title Fixup automatically create feed permissions Fixup automatically created feed permissions Oct 13, 2021
Copy link
Member

@riarenas riarenas left a comment

Choose a reason for hiding this comment

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

LGTM. Some nits

Copy link
Contributor

@michellemcdaniel michellemcdaniel left a comment

Choose a reason for hiding this comment

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

Just some nits

When the new target feed specifications were created, they collapsed down the feed specs for shipping and non-shipping package asset selection in cases where the target would be the same feed. This subtlety was required though, since in case of a stable build, we want just the shipping packages to go to a separate, newly created feed. The current code correct substitutes this if the TargetFeedSpecifications are broken out for ShippingOnly and NonShippingOnly, but there are a number of channels that did not break them out. Instead, it tried to send shipping packages to the isolated feed, and all packages to the non-isolated feed.

To fix this, separate out the TargetFeedSpecifications and put a check into the constructor to ensure that this can't be done accidentally.
Also
- Do a little refactoring for clarity.
- Add a test to ensure that this throws
@mmitche mmitche enabled auto-merge (squash) October 20, 2021 02:55
@mmitche mmitche merged commit cb683d5 into dotnet:main Oct 20, 2021
@mmitche mmitche deleted the fix-feed-perms branch February 9, 2022 18:52
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