-
Notifications
You must be signed in to change notification settings - Fork 644
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
AAD account checks on packages for safety reports #9360
Conversation
… safety reports on it.
@@ -114,6 +115,17 @@ public static bool IsAzureActiveDirectoryAccount(string type) | |||
return type?.Equals(External.AzureActiveDirectoryAccount, StringComparison.OrdinalIgnoreCase) ?? false; | |||
} | |||
|
|||
public static bool PackageHasNoAadOwners(Package package) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider moving this check to the Controller. We can move to NuGetGallery.Core if we need to re-use it.
@@ -1384,7 +1384,8 @@ public virtual ActionResult ReportAbuse(string id, string version) | |||
|
|||
var model = new ReportAbuseViewModel | |||
{ | |||
ReasonChoices = _featureFlagService.IsShowReportAbuseSafetyChangesEnabled() | |||
ReasonChoices = _featureFlagService.IsShowReportAbuseSafetyChangesEnabled() | |||
&& CredentialTypes.PackageHasNoAadOwners(package) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UTs?
return true; | ||
} | ||
|
||
return !owners.Where(o => o.Credentials.GetAzureActiveDirectoryCredential() != null).Any(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will !owners.Any(o => o.Credentials.GetAzureActiveDirectoryCredential() != null);
work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this is valid or not, but could you do return !package?.PackageRegistration?.Owners?.Any(o => o.Credentials.GetAzureActiveDirectoryCredential() != null) ?? true
? Just to mimic the other methods here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@skofman1 yes, that's cleaner--thanks. @camigthompson I'd like to leave it separate for readability, particularly as I'm separating our NRE-avoidance into its own block.
What should be the behavior if one of the package owners is an organization, and the org has collaborators that use AAD? |
@skofman1 This case is addressed now. Thanks for pointing it out! |
We'll allow safety report categories only on MSA-only account-owned packages as a first step.