Temp keys policy onboarding #3854

chenriksson · 2017-05-02T00:18:05Z

No description provided.

joelverhagen · 2017-05-02T00:24:14Z

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

+        private static string[] GetUsernamesFromQuery(string query)
+        {
+            return query.Split(new[] { ',', '\r', '\n' }, StringSplitOptions.RemoveEmptyEntries)
+                .Select(username => username.Trim()).ToArray();


You should probably filter out empty results after trimming.

joelverhagen · 2017-05-02T00:25:17Z

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

+        private IEnumerable<User> FindUsers(string[] usernames)
+        {
+            return EntitiesContext.Users
+                .Where(u => usernames.Any(name => u.Username.Equals(name, StringComparison.OrdinalIgnoreCase)))


This properly becomes Username IN ('a', 'b', ...) in SQL? Or is this being evaluated client side?

Evaluated SQL-side, with similar query...

SELECT * FROM Users WHERE EXISTS( SELECT 1 AS [C1] FROM ( SELECT N'name1' AS [C1] FROM ( SELECT 1 AS X ) AS [SingleRowTable1] UNION ALL SELECT N'name2' AS [C1] FROM ( SELECT 1 AS X ) AS [SingleRowTable2] ) AS [UnionAll2] WHERE Users.[Username] = [UnionAll2].[C1])

joelverhagen · 2017-05-02T00:25:53Z

src/NuGetGallery/Areas/Admin/Views/SecurityPolicy/Index.cshtml

+    ViewBag.Title = "Security Policies";
+}
+
+<section>


Could we get a screenshot?

@joelverhagen sent

can you send everybody?

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

I think you can upload the image to GitHub so everybody can see.

Attached to conversation

joelverhagen · 2017-05-02T19:30:24Z

src/NuGetGallery/Areas/Admin/Views/SecurityPolicy/Index.cshtml

+                    return true;
+                };
+
+                this.generateValue = function (user, policyGroup) {


Do we really have to invent our own serialization here? Can't we just JSON encode?

joelverhagen · 2017-05-02T19:36:42Z

src/NuGetGallery/Security/UserSecurityPolicyGroup.cs

+                Policies = new []
+                {
+                    new UserSecurityPolicy(RequirePackageVerifyScopePolicy.PolicyName),
+                    new UserSecurityPolicy(RequireMinClientVersionForPushPolicy.PolicyName) { Value = "{\"v\":\"4.1.0\"}" }


Let's use JSON API to write this JSON string.

joelverhagen · 2017-05-02T19:38:33Z

src/NuGetGallery/Security/UserSecurityPolicyGroup.cs

+
+            foreach (var key in pushKeys)
+            {
+                key.Expires = DateTime.Now.AddDays(7);


This will fall into the typical expiration notification flow -- so user's might now know reason why this is expiring, right?

Also, DateTime.UtcNow

This could actually give some keys more time right? Shouldn't we only do this if the Expires is greater than 7 days in the future?

joelverhagen · 2017-05-02T19:40:52Z

src/NuGetGallery/Security/UserSecurityPolicyGroup.cs

+
+        private static IEnumerable<UserSecurityPolicyGroup> CreateUserPolicyGroups()
+        {
+            yield return new UserSecurityPolicyGroup()


If this is all constant, can't we just subclass UserSecurityPolicyGroup? This allows us to avoid the Action<User> and just make it an overridden method.

Agreed. This is really confusing as is...just make this an abstract class with SecurePushUserSecurityPolicyGroup a subclass with all of this logic.

joelverhagen · 2017-05-02T19:42:39Z

src/NuGetGallery/Security/UserSecurityPolicyGroup.cs

+    /// <summary>
+    /// Grouping of one or more user security policies for enrollment.
+    /// </summary>
+    public class UserSecurityPolicyGroup


It seems to me like a "policy group" is a bit redundant. Isn't this just another policy, a la "aggregate policy"? Maybe I'm off here but couldn't a policy either have it's own logic, be an aggregate of other policies, or both? I don't see why we need another type for a grouping when the UserSecurityPolicy type could describe it as well.

@joelverhagen @skofman1 per our discussion, will add a new column to UserSecurityPolicies table to track what was enrolled in - which could be a single policy or aggregate.

@chenriksson make sure that column handles the state of being enrolled in multiple aggregates.

e.g.
I'm enrolled in policy A through groups X and Y. I leave group X. I should still be enrolled in policy A through group Y.

joelverhagen

⌚️

skofman1 · 2017-05-03T17:24:49Z

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

+
+        public SecurityPolicyController(IEntitiesContext entitiesContext)
+        {
+            EntitiesContext = entitiesContext;


skofman1 · 2017-05-03T17:37:45Z

src/NuGetGallery/Security/UserSecurityPolicyGroup.cs

+        /// Get supported user security policy groups.
+        /// </summary>
+        private static List<UserSecurityPolicyGroup> _instances;
+        public static List<UserSecurityPolicyGroup> Instances


consider using Lazy

skofman1 · 2017-05-03T17:39:31Z

src/NuGetGallery/Security/UserSecurityPolicyGroup.cs

+        private static void OnEnroll_SecurePush(User user)
+        {
+            var pushKeys = user.Credentials.Where(c =>
+                c.Type.StartsWith(CredentialTypes.ApiKey.Prefix) &&


c.Type.StartsWith(CredentialTypes.ApiKey.Prefi [](start = 16, length = 46)

I think there is a method IsApiKey that has this logic

skofman1 · 2017-05-03T17:44:54Z

src/NuGetGallery/Areas/Admin/ViewModels/SecurityPolicySearchResult.cs

+    /// <summary>
+    /// Security policy group enrollments for a user.
+    /// </summary>
+    public class SecurityPolicyEnrollments


r [](start = 21, length = 1)

one class per file please

skofman1 · 2017-05-03T17:49:53Z

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

+                }
+            }
+
+            await EntitiesContext.SaveChangesAsync();


should we send a user an e-mail when he is enrolled? Perhaps Anand has ideas.

skofman1 · 2017-05-03T17:51:29Z

scottbommarito

I really want to see UserSecurityPolicyGroup split into subclasses. Otherwise looks good.

scottbommarito · 2017-05-03T22:44:18Z

src/NuGetGallery/Areas/Admin/ViewModels/SecurityPolicySearchResult.cs

+    /// <summary>
+    /// Security policy group enrollments for a user.
+    /// </summary>
+    public class SecurityPolicyEnrollments


Might be more clear if this was named UserSecurityPolicyEnrollments.

scottbommarito · 2017-05-03T22:49:32Z

src/NuGetGallery/Areas/Admin/Views/SecurityPolicy/Index.cshtml

+    ViewBag.Title = "Security Policies";
+}
+
+<section>


I think you can upload the image to GitHub so everybody can see.

scottbommarito · 2017-05-03T22:58:56Z

src/NuGetGallery/Areas/Admin/ViewModels/SecurityPolicyViewModel.cs

+        public IEnumerable<string> PolicyGroups { get; set; }
+
+        /// <summary>
+        /// Requested user enrollments to make. String format is 'username|policygroup'.


When you changed the serialization did you update this comment?

scottbommarito · 2017-05-03T23:15:24Z

src/NuGetGallery/Security/UserSecurityPolicyGroup.cs

+    /// <summary>
+    /// Grouping of one or more user security policies for enrollment.
+    /// </summary>
+    public class UserSecurityPolicyGroup


@chenriksson make sure that column handles the state of being enrolled in multiple aggregates.

e.g.
I'm enrolled in policy A through groups X and Y. I leave group X. I should still be enrolled in policy A through group Y.

scottbommarito · 2017-05-03T23:17:03Z

src/NuGetGallery/Security/UserSecurityPolicyGroup.cs

+
+        private static IEnumerable<UserSecurityPolicyGroup> CreateUserPolicyGroups()
+        {
+            yield return new UserSecurityPolicyGroup()


Agreed. This is really confusing as is...just make this an abstract class with SecurePushUserSecurityPolicyGroup a subclass with all of this logic.

scottbommarito · 2017-05-03T23:19:13Z

tests/NuGetGallery.Facts/Security/UserSecurityPolicyExtensionsFacts.cs

+    {
+        [Theory]
+        [InlineData("", "")]
+        [InlineData(null, "")]


should also check [InlineData("", null)] just in case

scottbommarito · 2017-05-03T23:20:40Z

src/NuGetGallery/Security/UserSecurityPolicyExtensions.cs

+        /// <summary>
+        /// Determine whether two security policies are equivalent.
+        /// </summary>
+        public static bool Matches(this UserSecurityPolicy first, UserSecurityPolicy second)


Why is this not an Equals method?

e.g. UserSecurityPolicy should implement IEquatable<UserSecurityPolicy>

scottbommarito · 2017-05-03T23:23:33Z

tests/NuGetGallery.Facts/Security/UserSecurityPolicyExtensionsFacts.cs

+            // Arrange.
+            var group = new UserSecurityPolicyGroup()
+            {
+                Policies = LoadPolicies(groupPolicies)


If you use MemberData instead of InlineData you can get rid of the serialization/deserialization.

scottbommarito · 2017-05-03T23:26:12Z

tests/NuGetGallery.Facts/Security/UserSecurityPolicyExtensionsFacts.cs

+        [Theory]
+        [InlineData("[[\"A\",\"\"]]")]
+        [InlineData("[[\"A\",\"B\"],[\"E\",\"\"]]")]
+        public void AddPoliciesAddsPolicies(string groupPolicies)


Do we want test cases around
1 - adding policies that are already added
2 - removing policies that are not enrolled

scottbommarito · 2017-05-03T23:31:08Z

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

+
+            await EntitiesContext.SaveChangesAsync();
+
+            return RedirectToAction("Index");


Usually these kind of operations will set a message on the page, e.g. User x was enrolled in y.

Do we want to do that here? It could be nontrivial to figure out what was changed otherwise.

chenriksson · 2017-05-04T21:07:35Z

@scottbommarito

joelverhagen · 2017-05-05T15:44:22Z

src/NuGetGallery/Security/SecurePushSubscription.cs

+        /// <summary>
+        /// Subscription name.
+        /// </summary>
+        public string Name


Name [](start = 22, length = 4)

Nit: => SubscriptionName

joelverhagen · 2017-05-05T15:51:14Z

src/NuGetGallery.Core/Entities/UserSecurityPolicy.cs

 using System.ComponentModel.DataAnnotations;

 namespace NuGetGallery
 {
    /// <summary>
    /// User-subscribed security policy.
    /// </summary>
-    public class UserSecurityPolicy : IEntity
+    public class UserSecurityPolicy : IEntity, IEquatable<UserSecurityPolicy>


IEquatable [](start = 47, length = 10)

IEquatable should override GetHashCode

joelverhagen · 2017-05-05T15:51:56Z

tests/NuGetGallery.Core.Facts/Entities/UserSecurityPolicyFacts.cs

+            Assert.True(first.Equals(second));
+        }
+
+        public static IEnumerable<UserSecurityPolicy[]> EqualsReturnsFalse_Data()


EqualsReturnsFalse_Data [](start = 56, length = 23)

nit make this a property

joelverhagen · 2017-05-05T15:52:27Z

src/NuGetGallery/Migrations/201705041614287_UserSecurityPolicies_SubscriptionColumn.cs

+    {
+        public override void Up()
+        {
+            AddColumn("dbo.UserSecurityPolicies", "Subscription", c => c.String(nullable: false, maxLength: 256));


nullable [](start = 80, length = 8)

What will existing records do with this?

Or are we assuming there is no data?

No data in the table yet.

Sounds good. Thanks.

joelverhagen · 2017-05-05T15:52:57Z

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

+    {
+        public IEntitiesContext EntitiesContext { get; }
+
+        public ISecurityPolicyService PolicyService { get; }


PolicyService [](start = 38, length = 13)

nit why public?

joelverhagen · 2017-05-05T15:53:40Z

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

+        }
+
+        [HttpGet]
+        public virtual ActionResult Search(string query)


Search [](start = 36, length = 6)

thoughts on unit testing this?

Ideally we could mock the context and policy service

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

@joelverhagen Tests added!

joelverhagen · 2017-05-05T15:55:52Z

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

+            var usernames = GetUsernamesFromQuery(query);
+            var users = FindUsers(usernames);
+            var usersNotFound = usernames
+                .Where(name => !users.Any(u => u.Username.Equals(name, StringComparison.OrdinalIgnoreCase)))


Where [](start = 17, length = 5)

linq except might be a bit clearer for this

joelverhagen · 2017-05-05T16:00:16Z

src/NuGetGallery/Security/SecurityPolicyService.cs

+                throw new ArgumentNullException(nameof(subscription));
+            }
+
+            if (!IsSubscribed(user, subscription))


IsSubscribed [](start = 17, length = 12)

This isn't thread safe. Two simultaneous enrolls to the same subscription would result in duplicate policies right? Or is there a optimistic concurrency on the user?

I'm afraid that's the case for a lot of our CUD actions... for scoped API keys I added "locks" in the page itself to prevent double clicks from creating duplicate calls.

The confirmation prompt should help guard against duplicate clicks. I don't think a lock in the gallery (per instance) or optimistic concurrency in the DB (per row) is the solution here.

Now that we have the Subscription column, we could add a unique index (Name, Subscription)... assuming we wouldn't have a subscription contain multiple of the same policies differing in value only. We could then silently ignore duplicate inserts (SqlException 2601). Thoughts?

(Name, Subscription) unique index seems right. That being said, it seems like the "is enrolled?" and unenroll code paths handle duplicates just fine.

joelverhagen · 2017-05-05T16:01:23Z

src/NuGetGallery/Security/SecurePushSubscription.cs

+        /// <summary>
+        /// Expire API keys with push capability on secure push enrollment.
+        /// </summary>
+        private static void SetPushApiKeysToExpire(User user, int expirationInDays = 7)


= 7 [](start = 83, length = 3)

why optional parameter? there is only one acceptable value here today right? seems like a private const more correct here

joelverhagen · 2017-05-05T16:02:14Z

src/NuGetGallery/Security/SecurityPolicyService.cs

@@ -14,14 +15,47 @@ namespace NuGetGallery.Security
    /// </summary>
    public class SecurityPolicyService : ISecurityPolicyService


SecurityPolicyService [](start = 17, length = 21)

I don't see any logging around these actions. Are we thinking that auditing is enough?

Was planning to do auditing and telemetry together in next PR.

joelverhagen · 2017-05-05T16:02:29Z

🕐

skofman1 · 2017-05-05T16:05:24Z

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

+
+        public SecurityPolicyController(IEntitiesContext entitiesContext, ISecurityPolicyService policyService)
+        {
+            if (entitiesContext == null)


nit: c# 7 introduces throw expressions: https://blogs.msdn.microsoft.com/dotnet/2016/08/24/whats-new-in-csharp-7-0/

I think I tried in an earlier commit and had a CI issue... but will try it.

Make sure you have the changes to build.ps1 from this commit. If it still fails, run it on the build machines queue and not the hosted agent queue.

skofman1 · 2017-05-05T16:09:42Z

src/NuGetGallery/ExtensionMethods.cs

@@ -461,8 +461,7 @@ public static Credential GetCurrentApiKeyCredential(this User user, IIdentity id
            var claimsIdentity = identity as ClaimsIdentity;
            var apiKey = claimsIdentity.GetClaimOrDefault(NuGetClaims.ApiKey);

-            return string.IsNullOrEmpty(apiKey) ? null
-                : user.Credentials.FirstOrDefault(c => c.Value == apiKey);
+            return user.Credentials.FirstOrDefault(c => c.Value == apiKey);


What will be the behavior if we authenticated with a credential that is not an API key?

The apiKey var would be empty/null, and user.Credentials.FirstOrDefault would return null. Same behavior, so the IsNullOrEmpty check is unnecessary.

This is actually a regression from PR #3855, since some test infra uses empty API keys.

dnfclas added the cla-already-signed label May 2, 2017

chenriksson requested review from joelverhagen, agr, loic-sharma, ryuyu, cristinamanum, skofman1 and scottbommarito May 2, 2017 00:19

joelverhagen reviewed May 2, 2017

View reviewed changes

joelverhagen requested changes May 2, 2017

View reviewed changes

chenriksson force-pushed the chenriks-tempkeys-onboard branch from 778bc17 to b0259c4 Compare May 2, 2017 21:27

skofman1 reviewed May 3, 2017

View reviewed changes

scottbommarito suggested changes May 3, 2017

View reviewed changes

chenriksson force-pushed the chenriks-tempkeys-onboard branch from b0259c4 to 63bb404 Compare May 4, 2017 21:03

chenriksson force-pushed the tempkeys branch from 9a3ab5c to 228a812 Compare May 4, 2017 21:40

chenriksson added 2 commits May 4, 2017 14:42

Temp keys onboarding

d1f38a8

Bug fixes

82af4f4

chenriksson added 5 commits May 4, 2017 14:42

Cleanup

003161f

PR feedback

c3bc293

Fix test failure with ToList

e334a41

PR feedback

a89ad42

PR feedback

3f840d3

chenriksson force-pushed the chenriks-tempkeys-onboard branch from 63bb404 to 3f840d3 Compare May 4, 2017 21:56

cleanup

7222a3c

joelverhagen reviewed May 5, 2017

View reviewed changes

skofman1 reviewed May 5, 2017

View reviewed changes

skofman1 approved these changes May 5, 2017

View reviewed changes

PR feedback

5cebd42

joelverhagen approved these changes May 5, 2017

View reviewed changes

chenriksson merged commit 52dc307 into tempkeys May 8, 2017

chenriksson deleted the chenriks-tempkeys-onboard branch May 8, 2017 15:35

chenriksson added a commit that referenced this pull request May 16, 2017

Temp keys policy onboarding (#3854)

fc1e0d6

chenriksson added a commit that referenced this pull request May 16, 2017

Temp keys policy onboarding (#3854)

a1bc005


		await EntitiesContext.SaveChangesAsync();

		return RedirectToAction("Index");

		@@ -14,14 +15,47 @@ namespace NuGetGallery.Security
		/// </summary>
		public class SecurityPolicyService : ISecurityPolicyService

Temp keys policy onboarding #3854

Temp keys policy onboarding #3854

Conversation

chenriksson commented May 2, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelverhagen 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

skofman1 commented May 3, 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

Choose a reason for hiding this comment

chenriksson commented May 4, 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

chenriksson May 5, 2017 • edited Loading

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

joelverhagen commented May 5, 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

chenriksson May 5, 2017 •

edited

Loading