Support for policy propagation #4061

chenriksson · 2017-06-06T17:52:03Z

Added propagation policy which can be manually subscribed from admin security policies view
Added 'add owner' confirmation from ManageUsers view, with policy warning if propagation detected
Modified add owner request email to include policy warning if propagation detected
Added email to existing owners once new owner is added, with policy warning if propagation detected
Removed unused queries from ManagePackageOwnersViewModel

Will email various scenarios w/ expected communication to the team shortly for review... since they differ slightly from @anangaur 's experience spec.

skofman1 · 2017-06-06T18:56:40Z

src/NuGetGallery/Controllers/JsonApiController.cs

        {
+            if (string.IsNullOrEmpty(id))
+            {
+                throw new ArgumentNullException(nameof(id));


ArgumentNullException [](start = 26, length = 21)

ArgumentException since if id is empty ArgumentNullException will be confusing

skofman1 · 2017-06-06T18:59:25Z

src/NuGetGallery/Controllers/JsonApiController.cs

            var package = _packageService.FindPackageRegistrationById(id);
            if (package == null)
            {
-                return Json(new { success = false, message = "Package not found" });
+                return Task.FromResult(Json(new { success = false, message = "Package not found." }));


messages should be in Strings.resx

done, except for existing and new emails in MessageService

loic-sharma · 2017-06-06T22:28:22Z

src/NuGetGallery/App_Start/DefaultDependenciesModule.cs

@@ -206,10 +206,13 @@ protected override void Load(ContainerBuilder builder)
                .AsSelf()
                .As<ISecurityPolicyService>()
                .InstancePerLifetimeScope();
-
+            


Empty white space here.

loic-sharma · 2017-06-06T22:53:07Z

src/NuGetGallery/Views/Packages/ManagePackageOwners.cshtml

+                        success: function (data) {
+                            if (data.success) {
+                                if (data.confirmation) {
+                                    if (!confirm(data.confirmation)) {


Merge this with the previous conditional?

skofman1 · 2017-06-07T00:02:08Z

src/NuGetGallery/Security/RequirePackageVerifyScopePolicy.cs

@@ -11,7 +10,7 @@ namespace NuGetGallery.Security
    /// </summary>
    public class RequirePackageVerifyScopePolicy : UserSecurityPolicyHandler
    {
-        public const string PolicyName = "RequirePackageVerifyScopePolicy";
+        public const string PolicyName = nameof(RequirePackageVerifyScopePolicy);


skofman1 · 2017-06-07T00:25:43Z

src/NuGetGallery/Controllers/PackagesController.cs

+                    policyMessage = SecurePushMessages.NoticeOfPoliciesSubscribedByPolicyOwner(policyMessageOwners, user);
+                }
+            }
+
            ConfirmOwnershipResult result = await _packageService.ConfirmPackageOwnerAsync(package, user, token);


token verification might fail. This will happened only in ConfirmPackageOwnerAsync method. With the current code this will be after subscription propagation was applied.

Thanks - good catch!

chenriksson · 2017-06-07T20:16:58Z

src/NuGetGallery/Security/SecurePushMessages.cs

+
+{PolicyDescriptions}
+
+Note this step cannot be easily undone. If you are unsure and/or need more information, please contact support@nuget.org.


Need to update to use 'Gallery.GalleryOwner' for support@nuget.org.

loic-sharma · 2017-06-07T20:55:33Z

src/NuGetGallery/Controllers/JsonApiController.cs

            }
-            if (!package.IsOwner(HttpContext.User))
+
+            var propagatingOwners = package.Owners.Where(o => RequireSecurePushForCoOwnersPolicy.IsSubscribed(o)).Select(o => o.Username);


NIT: This can be Where(RequireSecurePushForCoOwnersPolicy.IsSubscriptedTo). If you change this line, there are a few more in this pull request that you can update as well.

loic-sharma · 2017-06-08T18:02:59Z

src/NuGetGallery/Controllers/PackagesController.cs

+            {
+                foreach (var owner in package.Owners)
+                {
+                    if (await SubscribeToSecurePushAsync(owner))


Is this likely to fail? If so, how would we find and then add missing secure push policies? Would it be better to move SubscriptToSecurePushAsync to the PackageService's AddPackageOwnerAsync, and if SubscriptToSecurePushAsync fails for any of the owners, then the overall AddPackageOwnerAsync operation fails.

I agree with Loic here. Consider spliting ConfirmPackageOwnerAsync to two methods: one to evaluate the token and return true/false, and another to actually apply the ownership. HandleNewPackageOwnerAsync can run after verifications of ownership but before actually applying.

In reply to: 120957931 [](ancestors = 120957931)

Can do; was trying to avoid modifying the existing PackageService.ConfirmPackageOwnerAsync which is committed in a separate transaction.

we can still keep it in a separate transaction, it's just better to fail adding the owner (and the user can retry that), then to fail adding policies.

On a related note, if a user is subscribed to the policy as the result of adding a new owner, but adding the owner eventually fails, I think we should unsubscribe that use from the policy.

@loic-sharma @skofman1 @scottbommarito Code was updated to validate the owner request token before applying policy subscription(s). I didn't add logic to unsubscribe should SQL to add owner fail. This is currently hard since there are multiple points of failure: add owner transaction, remove owner request transaction, auditing. Failing more gracefully is probably something we can generally do better, but I think the current behavior here is sufficient for now.

scottbommarito

Need to handle the case when subscribing fails.

scottbommarito · 2017-06-08T20:18:40Z

src/NuGetGallery/Controllers/JsonApiController.cs

            {
-                return Json(new { success = false, message = string.Format(CultureInfo.InvariantCulture, "Sorry, {0} hasn't verified their email account yet and we cannot proceed with the request.", username) });
+                var propagatingOwners = package.Owners.Where(RequireSecurePushForCoOwnersPolicy.IsSubscribed).Select(o => o.Username);


This code appears extremely similar to the code above in GetAddPackageOwnerConfirmationMessage, but the message emitted is different. I would suggest moving it to a helper method that returns the list of propagating owners and propagating pending owners.

This would also improve the readability of the code, because it will be immediately obvious what the code is doing--finding the propagating users.

scottbommarito · 2017-06-08T20:29:28Z

src/NuGetGallery/Controllers/JsonApiController.cs

        }

-        [HttpPost]
-        public async Task<JsonResult> RemovePackageOwner(string id, string username)
+        private Task<JsonResult> ManagePackageOwnerAsync(string id, string username, Func<PackageRegistration, User, User, Task<JsonResult>> actionAsync)


I think the code would be much more readable if you changed this design to remove the delegates.

I would suggest the Try- model.

For example:

old

return ManagePackageOwnerAsync(id, user, func);

new:

var canManageOwners = TryManagePackageOwners(id, username, out var jsonResult, out var package, out var newOwner, out var currentOwner); if (!canManageOwners) { return jsonResult; } return func(package, newOwner, currentOwner);

scottbommarito · 2017-06-08T20:56:33Z

src/NuGetGallery/Controllers/PackagesController.cs

+            {
+                foreach (var owner in package.Owners)
+                {
+                    if (await SubscribeToSecurePushAsync(owner))


On a related note, if a user is subscribed to the policy as the result of adding a new owner, but adding the owner eventually fails, I think we should unsubscribe that use from the policy.

scottbommarito · 2017-06-08T20:57:18Z

src/NuGetGallery/Security/ISecurityPolicyService.cs

@@ -26,7 +26,13 @@ public interface ISecurityPolicyService
        /// <summary>
        /// Subscribe a user to one or more security policies.
        /// </summary>
-        Task SubscribeAsync(User user, IUserSecurityPolicySubscription subscription);
+        /// <returns></returns>


nit: get rid of this empty <returns></returns>

scottbommarito · 2017-06-08T21:16:00Z

tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs

                var model = ResultAssert.IsView<PackageOwnerConfirmationModel>(result);
                Assert.Equal(confirmationResult, model.Result);
                Assert.Equal("foo", model.PackageId);
            }
+
+            [Fact]
+            public async Task SubscribesOwnersToSecurePushAndSendsEmailIfNewOwnerRequires()


I would suggest putting these inside a TestContainer so that these tests are separated from the rest of the TheConfirmOwnerMethod tests, because they are specifically related to RequireSecurePushForCoOwnersPolicy.

scottbommarito · 2017-06-08T21:18:31Z

tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs

+            }
+
+            [Fact]
+            public async Task SubscribesNewOwnerToSecurePushAndSendsEmailIfOwnerRequires()


We need failure state tests:

subscribing new owner fails

new owner should not be added

old owners shouldn't be subscribed if they weren't before

subscribing old owners fails

new owner should not be added

new owner and other old owners shouldn't be subscribed if they weren't before

chenriksson · 2017-06-09T23:05:10Z

Still looking into the following:

[Secure push] Admin tools - when clicking SecurePush table header user is not checked #4079 (Admin/SecurityPolicies header checkbox)
When adding an owner with propagating secure push, the warning is in a JavaScript alert #4081 (Replacement for modal dialog?)
Retain user search results on /admin policy subscription
Display secure push info on packages view / manage package owners, for owners/admins
Should PackagesController::ConfirmOwner handle unsubscription on AddPackageOwnerAsync failure. Currently this can be complicated since it spans multiple transactions.

chenriksson · 2017-06-12T18:23:20Z

Screenshot for DisplayPackage.cshtml (if owner or admin only):

Screenshot for ManagePackageOwners.cshtml confirmation (no policies):

Screenshot for ManagePackageOwners.cshtml confirmation (with policies):

scottbommarito

looks awesome!

agr · 2017-06-12T21:55:52Z

src/NuGetGallery/Areas/Admin/Controllers/SecurityPolicyController.cs

+        private User FindUser(string username)
+        {
+            return EntitiesContext.Users
+                .FirstOrDefault(u => username.Equals(u.Username, StringComparison.OrdinalIgnoreCase));


Does EF properly understand the string.Equals call?..
I mean, if the server side collation settings are case sensitive, would this properly do the case insensitive comparison?
If not, then this code is confusing and should be replaced with a simple ==.

It's worked so far, but likely haven't tested against case sensitive collation. I verified that AuthenticationService is using '==', so I'll update to match.

agr · 2017-06-12T22:11:29Z

src/NuGetGallery/Areas/Admin/Controllers/SecurityPolicyController.cs

            {
-                foreach (var subscription in PolicyService.UserSubscriptions)
+                var user = FindUser(r.Key);


Check r.Key for null before calling the FindUser?
(and another instance few lines below)

Also, FindUser will return null if the user doesn't exist. Should this case be handled?

agr · 2017-06-12T22:15:29Z

src/NuGetGallery/Security/SecurityPolicyService.cs

+        {
+            if (string.IsNullOrEmpty(subscriptionName))
+            {
+                throw new ArgumentException(nameof(subscriptionName));


ArgumentNullException?

Per @skofman1 I should throw ArgException for IsNullOrEmpty. I'll update the other APIs that take subscriptionName to throw ArgException for consistency.

Maybe check using the IsNullOrWhitespace then?

Left as IsNullOrEmpty since it's used most consistently in the Gallery. Maybe we should revisit throughout at some point.

loic-sharma · 2017-06-12T22:26:08Z

src/NuGetGallery/Areas/Admin/Controllers/SecurityPolicyController.cs

            {
-                foreach (var subscription in PolicyService.UserSubscriptions)
+                var user = FindUser(r.Key);


Also, FindUser will return null if the user doesn't exist. Should this case be handled?

- Added inline confirmation when adding new package owner - Added package URL link to package owner request emails - Added new notification to co-owners when package owner request is confirmed - Added secure push policy messaging to communication above (confirmation, request, and notification) - Added secure push policy messaging to package view for owners and admins - Fixed bug on security policy admin view where toggle all broken if multiple subscriptions - Updated security policy admin view to not reload page on update postback

* Fixed Report Abuse Page's Accessibility (#4001) Fixes #4002. Relevant to [VSTS #395879](https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_workitems?id=395879&_a=edit). This help-text bug was for the sign-in page, but this also affects the Report Abuse page. * Fixed accessibility of email field in report abuse page * Fixed signature accessibility * Use ServerCommon's Response Code Processor (#3999) * Added Nuget.Services.Logging; moved to TelemetryResponseCodeProcessor * Added binding redirect for AI * Removing the "WITH(ONLINE=ON)" from the index creation as per #3952 (#4004) * Removing the "WITH(ONLINE=ON)" from the index creation as per #3952 (#4004) (#4030) * Remove validation rules that block uploading valid semver2 versions #3645 (#3757) * Add support for semVerLevel query parameter to V2 endpoints (#3714) * adding new optional semVerLevel query parameter to v2 odata endpoints * adding new optional semVerLevel query parameter to v2 autocomplete endpoints * Applying semVerLevel filter on v2 OData endpoints * Use [FromUri] attribute on semVerLevel (avoids having single quotes in the parameter value) * Ensure navigation links on v2 feeds use normalized version * Clarifying comment on Get(Id=,Version=) v2 API * Properly default to semver2 inclusion on Get(Id=,Version=) * Compare NormalizedVersion to be able to retrieve matching SemVer2 package versions for a given normalized version string. * Code review feedback * Update and fix broken test data * Keep legacy version compliance checks in place for non-SemVer2 versions (#3761) * Keep legacy version compatibility checks in place for non-semver2 versions * Added comment to clarify the reasons behind the legacy version check. * Fix typo * Rename test for clarity * code review feedback * Set SemVerLevelKey after setting Dependencies * #3861 V2 NuGetEntityTypeSerializer Id link patcher must retain curated feed name (#3864) * Support Is(Absolute)Latest for SemVer2 + semVerLevel for SearchService (#3842) * LuceneIndexingService in Gallery should take into account IsLatest(Stable)SemVer2 (#3863) * POST VerifyPackage version validation should use ToFullString comparison * ODataV2CuratedFeedController should support semver2 by default when requesting a specific version, and compare on NormalizedVersion * Refactor NuGetEntityTypeSerializer + unit test coverage (#3879) * Use NormalizedVersion in URLs contained in PackageAddedNotice (#3886) * Add nullcheck and use TryParse. (#3890) * Show full version on package details page (#3887) * Highlight semver2 packages on package details view (#3893) * Fix bug in IsLatest(Stable)SemVer2 (#3895) * VerifyPackage on ApiController should treat version as optional parameter (#3903) * UpdateIsLatest not resetting IsLatest(Stable)SemVer2 on previous latest versions (#3909) * Fix malformed URL in redirect after package upload (#3915) * Minor fix for search results package URLs (when to use version or not in the URL) * UrlHelper extension for Package should use NormalizedVersion (#3925) * Default to latest stable semver2 on package details page (#3930) * User profile page does not show SemVer 2.0.0 packages #3911 (#3933) * Fix functional test failure SearchMicrosoftDotNetCuratedFeed #3941 (#3942) * Fix System.NotSupportedException on User profile page (#3943) * Fix Functional Test failure for ODataFeeds.V2FeedExtendedTests.FindPackagesByIdTest #3947 (#3948) * Fix load test failure due to incorrect test setup (#3957) * Hijack IsLatest(Stable)Version OData filter when semVerLevel=2.0.0 (#3966) * Detect if package only differ by metadata and show optimal user-facing error message (#3970) * Update Semver2 package details message with final nuget client version #3897 (#3988) * On package validation failure an actionable error message should be displayed. #3916 (#4031) * Make downloads link on home page a proper link (#4052) * Fix the date format on stats page (#4057) * Update telemetry processors (#4059) * Reorder SemVer2Latest migration to match deployment history (#4062) * Average download shown incorrectly when its 1.x #4039 (#4040) * Average download shown incorrectly when its 1.x #4039 * Moved logic to viewmodel and added UTs * SemVer2 - Missing db index on Packages table #498 (#4073) * SemVer2 - Missing db index on Packages table #498 * SemVer2 - Missing db index on Packages table for partial search #499 (#4074) * Package-Versions autocomplete endpoint does not properly handle semVerLevel when using the db #4086 (#4087) * Package-Versions autocomplete endpoint does not properly handle semVerLevel when using the db #4086 (#4087) * v2 package-versions auto-complete endpoint should exclude deleted versions #4092 (#4093) * Remove auto-refresh AJAX call for total stats on home page #4090 (#4091) * v2 package-versions auto-complete endpoint should exclude unlisted versions #4092 (#4099) * Support for policy propagation (#4061) - Added inline confirmation when adding new package owner - Added package URL link to package owner request emails - Added new notification to co-owners when package owner request is confirmed - Added secure push policy messaging to communication above (confirmation, request, and notification) - Added secure push policy messaging to package view for owners and admins - Fixed bug on security policy admin view where toggle all broken if multiple subscriptions - Updated security policy admin view to not reload page on update postback * Preserve original 409 exception in AI logs (#4136) * System.ArgumentNullException GET packages/DisplayPackage #4204 (#4210) * Resolve merge conflicts

dnfclas added the cla-already-signed label Jun 6, 2017

skofman1 reviewed Jun 6, 2017

View reviewed changes

loic-sharma reviewed Jun 6, 2017

View reviewed changes

skofman1 requested changes Jun 7, 2017

View reviewed changes

chenriksson commented Jun 7, 2017

View reviewed changes

loic-sharma reviewed Jun 7, 2017

View reviewed changes

chenriksson requested review from joelverhagen, agr, loic-sharma, shishirx34, ryuyu, cristinamanum, skofman1 and scottbommarito and removed request for joelverhagen June 7, 2017 21:15

chenriksson force-pushed the chenriks-propagate branch from 83bf7cc to f2defca Compare June 7, 2017 23:06

loic-sharma reviewed Jun 8, 2017

View reviewed changes

scottbommarito suggested changes Jun 8, 2017

View reviewed changes

This was referenced Jun 12, 2017

[Secure push] Admin tools - when clicking SecurePush table header user is not checked #4079

Closed

When adding an owner with propagating secure push, the warning is in a JavaScript alert #4081

Closed

skofman1 approved these changes Jun 12, 2017

View reviewed changes

scottbommarito approved these changes Jun 12, 2017

View reviewed changes

agr reviewed Jun 12, 2017

View reviewed changes

loic-sharma approved these changes Jun 12, 2017

View reviewed changes

agr approved these changes Jun 12, 2017

View reviewed changes

Temp keys propagation

93fe14c

chenriksson added 15 commits June 12, 2017 15:50

PR feedback

7922cd8

PR feedback: moving strings to resx

bc5a6dd

Use config for support email

50ab258

Bug fixes

5639581

PR feedback and fixes

95ca519

Fixes and tweaks

4cf444f

PR and bug bash feedback

e7ae5a0

Fix for #4079

9f2687c

Fix for 'Display secure push info on packages view...

cfefa06

Fix build warning

39a47ca

Fix for #4081

a488aaf

Change button text

350b332

Change policy post to async

da9e0c1

Rethrow on subscribe failure

54c8e5a

PR feedback

7afc8df

chenriksson force-pushed the chenriks-propagate branch from 2bd0946 to 7afc8df Compare June 12, 2017 22:51

chenriksson merged commit e4bd4cb into dev Jun 12, 2017

chenriksson deleted the chenriks-propagate branch July 5, 2017 15:56


		{PolicyDescriptions}

		Note this step cannot be easily undone. If you are unsure and/or need more information, please contact support@nuget.org.

Support for policy propagation #4061

Support for policy propagation #4061

Conversation

chenriksson commented Jun 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottbommarito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenriksson commented Jun 9, 2017 • edited Loading

chenriksson commented Jun 12, 2017

scottbommarito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenriksson commented Jun 9, 2017 •

edited

Loading