PermissionsService 2.0 (just implementation) #5158

scottbommarito · 2017-12-09T01:03:37Z

This is the implementation of the new version of the Permissions Service, without changes to the existing code.

You may have seen this code before in #5097

scottbommarito · 2017-12-09T01:06:56Z

src/NuGetGallery/Helpers/PermissionsHelpers.cs

+        /// <summary>
+        /// Is <paramref name="currentPrincipal"/> allowed to perform an action with a requirement of <paramref name="permissionsRequirement"/> on <paramref name="packageRegistration"/>?
+        /// </summary>
+        public static bool IsRequirementSatisfied(PermissionsRequirement permissionsRequirement, IPrincipal currentPrincipal, PackageRegistration packageRegistration)


This file is mostly identical to the existent PermissionsService except:
1 - IsActionAllowed is now IsRequirementSatisfied
2 - Added a method for ReservedNamespaces
3 - Made some performance checks as suggested by @cristinamanum (specifically, we now check if having a specific kind of ownership would satisfy the requirement before checking if we have that ownership)

joelverhagen · 2017-12-11T22:34:24Z

src/NuGetGallery/Services/ActionsRequiringPermissions.cs

+        public static ActionRequiringPackagePermissions UploadNewPackageVersion =
+            new ActionRequiringPackagePermissions(
+                PermissionsRequirement.Owner | PermissionsRequirement.OrganizationAdmin | PermissionsRequirement.OrganizationCollaborator,
+                PermissionsRequirement.Owner | PermissionsRequirement.SiteAdmin);


PermissionsRequirement.SiteAdmin [](start = 47, length = 32)

This one is wrong. Admins shouldn't be able to upload new versions.

joelverhagen · 2017-12-11T22:36:44Z

src/NuGetGallery/Services/ActionRequiringReservedNamespacePermissions.cs

+    /// An action requiring permissions on a <see cref="ReservedNamespace"/> that can be done on behalf of another <see cref="User"/>.
+    /// </summary>
+    public class ActionRequiringReservedNamespacePermissions
+        : ActionRequiringEntityPermissions<IEnumerable<ReservedNamespace>>, IActionRequiringEntityPermissions<ActionOnNewPackageContext>


IEnumerable [](start = 43, length = 30)

Might be worth leaving a comment about why this is multiple namespaces, i.e. multiple namespaces could apply to a single ID.

joelverhagen · 2017-12-11T22:39:59Z

src/NuGetGallery/Services/ActionRequiringEntityPermissions.cs

+
+            var possibleAccountsOnBehalfOf = 
+                new[] { currentUser }
+                    .Union(GetOwners(entity));


Union [](start = 21, length = 5)

Are set semantics necessary here? Are we sure comparison of user instances is working? Concat may be simpler.

I wanted to get rid of duplicates. I will replace this with Concat and Distinct

Do you really care about duplicates here? Like most of the time it will be unique right? Distinct uses default comparer which is likely reference equals (or did we implement GetHashCode and Equals?)

yes, the values calculated here will be piped directly to the UI for upload and API key scenarios.

joelverhagen · 2017-12-11T22:40:59Z

src/NuGetGallery/Services/ActionRequiringEntityPermissions.cs

+                var failure = IsAllowed(currentUser, accountOnBehalfOf, entity);
+                if (failure == PermissionsCheckResult.Allowed)
+                {
+                    accountsAllowedOnBehalfOf = accountsAllowedOnBehalfOf.Concat(new[] { accountOnBehalfOf });


Concat [](start = 74, length = 6)

Use List, don't need LINQ for everything

makes sense

joelverhagen · 2017-12-11T22:43:58Z

src/NuGetGallery/Helpers/PermissionsHelpers.cs

+        /// </summary>
+        private static bool WouldSatisfy(PermissionsRequirement permissionsRequirementToCheck, PermissionsRequirement permissionsRequirementToSatisfy)
+        {
+            return (permissionsRequirementToCheck & permissionsRequirementToSatisfy) > 0;


[](start = 85, length = 1)

!= unless this is an unsigned flags or if you have tests asserting never negative flag values

== and != work with Flags?

I'm using the int logic here because the second parameter is a combination of multiple enum values, so I want to compare that they just have at least one value in common.

[Flags] is just an attribute. The important details is that the enum is an int (unless you changed that...)

No what I mean is we'll be comparing 0b0010 and 0b1010 and expect it to return true. I didn't think enum or Flags made this comparison return true.

Oh I see now

joelverhagen · 2017-12-11T22:47:16Z

src/NuGetGallery/Helpers/PermissionsHelpers.cs

+            if (entityOwners == null || !entityOwners.Any())
+            {
+                return false;
+            }


This breaks admin permissions (on things they would otherwise do)

fixed--it was not only a logic issue but a tests issues

joelverhagen · 2017-12-11T22:49:28Z

src/NuGetGallery/Helpers/PermissionsHelpers.cs

+                .OfType<Organization>()
+                .SelectMany(o => o.Members)
+                .Where(m => isUserMatch(m.Member))
+                .ToArray();


ToArray [](start = 17, length = 7)

ToList requires fewer allocations and is a better default for these one-off lists

joelverhagen · 2017-12-11T22:50:15Z

src/NuGetGallery/Helpers/PermissionsHelpers.cs

+                return true;
+            }
+
+            return false;


false [](start = 19, length = 5)

This is such a generic code path, I have a hard time thinking of cases that would fall through here. Might be worth commenting about that.

i did some restructuring that should make this clearer to understand

joelverhagen

⌚️

Very nice PR. Thanks for the break-down.

skofman1 · 2017-12-12T01:34:31Z

src/NuGetGallery/Helpers/PermissionsHelpers.cs

+            }
+
+            if (WouldSatisfy(PermissionsRequirement.SiteAdmin, permissionsRequirement) &&
+                isUserAdmin)


perf nit: always put the cheaper condition first so the 'and' operation will fail if it's false

cristinamanum · 2017-12-12T17:08:44Z

src/NuGetGallery/Services/ActionRequiringEntityPermissions.cs

+            accountsAllowedOnBehalfOf = Enumerable.Empty<User>();
+
+            var possibleAccountsOnBehalfOf = 
+                new[] { currentUser }


Is it ok to add the currentUser if the currentUser == null ?

yup, it's necessary--if the current user is logged out but still able to perform the action, we still want to return true

cristinamanum · 2017-12-12T17:24:27Z

src/NuGetGallery/Services/ActionRequiringReservedNamespacePermissions.cs

+            }
+
+            return reservedNamespaces.Any(rn => PermissionsHelpers.IsRequirementSatisfied(ReservedNamespacePermissionsRequirement, account, rn)) ?
+                PermissionsCheckResult.Allowed : PermissionsCheckResult.ReservedNamespaceFailure;


Why is it fine to have Any? Should not be permissions allowed for All the namespaces?

You only need one--see https://github.com/NuGet/NuGetGallery/blob/master/src/NuGetGallery/Services/ReservedNamespaceService.cs#L381

agr · 2017-12-12T18:38:44Z

src/NuGetGallery/Helpers/PermissionsHelpers.cs

+                return true;
+            }
+
+            if (entityOwners == null || !entityOwners.Any())


There is a multiple enumeration of an IEnumerable (between here and lines 124 and 129). Since it is unlikely could be fixed, please use something else (ICollection at least).

switched all methods in this class to use ICollection

agr · 2017-12-12T18:47:20Z

src/NuGetGallery/Services/ActionRequiringEntityPermissions.cs

+        /// <summary>
+        /// Determines whether <paramref name="currentUser"/> can perform this action on <paramref name="entity"/> on behalf of <paramref name="account"/>.
+        /// </summary>
+        public PermissionsCheckResult IsAllowed(User currentUser, User account, TEntity entity)


If it's not returning bool it shouldn't be called IsAllowed, same for overload below

renamed to CheckPermissions

agr · 2017-12-12T18:48:59Z

src/NuGetGallery/Services/ActionRequiringPackagePermissions.cs

+            PackageRegistrationPermissionsRequirement = packageRegistrationPermissionsRequirement;
+        }
+
+        public PermissionsCheckResult IsAllowed(User currentUser, User account, Package package)


Same comment as above: method name should be different.

renamed to CheckPermissions

agr · 2017-12-12T18:50:38Z

src/NuGetGallery/Services/ActionRequiringReservedNamespacePermissions.cs

+
+        protected override PermissionsCheckResult IsAllowedOnEntity(User account, IEnumerable<ReservedNamespace> reservedNamespaces)
+        {
+            if (!reservedNamespaces.Any())


Multiple enumeration of IEnumerable

changed to IReadOnlyCollection

agr · 2017-12-12T18:52:36Z

src/NuGetGallery/Services/PermissionsRequirement.cs

+        /// <summary>
+        /// No requirement--any user can satisfy the requirement.
+        /// </summary>
+        None = 1,


Replace values with 1 << 0, 1 << 1, 1 << 2, etc. maybe to emphasize bit masking?

agr · 2017-12-12T18:55:16Z

tests/NuGetGallery.Facts/Services/PermissionsHelpersFacts.cs

+
+            private static bool Includes(int i, ReturnsSatisfiedRequirementWhenExpected_State state)
+            {
+                return (i & (int)state) > 0;


Should be != instead of >, strictly speaking, but wouldn't be an issue unless we have more stuff in the ReturnsSatisfiedRequirementWhenExpected_State

chenriksson · 2017-12-12T18:05:36Z

src/NuGetGallery/Helpers/PermissionsHelpers.cs

+        /// </summary>
+        public static bool IsRequirementSatisfied(PermissionsRequirement permissionsRequirement, IPrincipal currentPrincipal, IEnumerable<User> entityOwners)
+        {
+            if (WouldSatisfy(PermissionsRequirement.None, permissionsRequirement))


There are 2 WouldSatisfy checks when prinipal is non-null. Recommend changing to:

if (currentPrincipal == null) { return WouldSatisfy(PermissionsRequirement.None, permissionsRequirement); }

chenriksson · 2017-12-12T18:05:59Z

src/NuGetGallery/Helpers/PermissionsHelpers.cs

+            if (currentUser == null)
+            {
+                return false;
+            }


see comment above about the WouldSatisfy check...

chenriksson · 2017-12-12T18:33:02Z

src/NuGetGallery/Services/ActionRequiringEntityPermissions.cs

+
+            foreach (var accountOnBehalfOf in possibleAccountsOnBehalfOf)
+            {
+                var failure = IsAllowed(currentUser, accountOnBehalfOf, entity);


Shouldn't need to check IsAllowed on currentUser, right?

Also, can you just do the IsAllowed check on org owners when populating the original list? i.e., GetOwners(entity).Where(o => IsAllowed(currentUser, o, entity))

currentUser is required in all scenarios now--"can currentUser perform this action on entity on behalf of accountOnBehalfOf" is how this function call should be read

I tried to make it all LINQ, but since I have to split the initial population of the list into two pieces (see the currentUser != null check) I think this is cleaner.

chenriksson · 2017-12-12T18:36:25Z

src/NuGetGallery/Services/ActionRequiringPackagePermissions.cs

+    /// An action requiring permissions on a <see cref="PackageRegistration"/> or <see cref="Package"/> that can be done on behalf of another <see cref="User"/>.
+    /// </summary>
+    public class ActionRequiringPackagePermissions
+        : ActionRequiringEntityPermissions<PackageRegistration>, IActionRequiringEntityPermissions<Package>


Seems unusual that the base class generic parameter is the package registration, but the interface parameter is the package version. Should/could both use package as the generic parameter?

base class extends the interface for PackageRegistration and provides the actual implementation

by extending the interface for Package here we allow users to pass in Package instead of PackageRegistration for convenience

chenriksson · 2017-12-12T18:49:57Z

src/NuGetGallery/Services/ActionsRequiringPermissions.cs

+        public static ActionRequiringPackagePermissions DisplayPrivatePackageMetadata =
+            new ActionRequiringPackagePermissions(
+                PermissionsRequirement.Owner | PermissionsRequirement.OrganizationAdmin | PermissionsRequirement.OrganizationCollaborator,
+                PermissionsRequirement.Owner | PermissionsRequirement.SiteAdmin);


nit: might be nice if these used named args, assuming shorter arg names:

new ActionRequiringPackagePermissions( onBehalfOfRequirement: PermissionsRequirement.Owner | PermissionsRequirement.OrganizationAdmin | PermissionsRequirement.OrganizationCollaborator, actionRequirement: PermissionsRequirement.Owner | PermissionsRequirement.SiteAdmin);

Or, maybe improve readability by introducing private fields for OnBehalfOf combinations:

static PermissionsRequirement OnBehalfOfOwnerOrOrganizationMember; static PermissionsRequirement OnBehalfOfOwnerOrOrganizationAdmin; new ActionRequiringPackagePermissions( OnBehalfOfOwnerOrOrganizationMember, PermissionsRequirement.Owner | PermissionsRequirement.SiteAdmin);

I like both of these--will do both

chenriksson · 2017-12-12T18:57:27Z

src/NuGetGallery/Services/PermissionsCheckResult.cs

+        /// <summary>
+        /// The reason an action is not allowed is not known.
+        /// </summary>
+        UnknownFailure,


Is this used outside tests?

it initially was, but is no longer. will remove

chenriksson · 2017-12-12T19:02:12Z

src/NuGetGallery/Services/PermissionsCheckResult.cs

+    /// Describes the result of checking if an <see cref="ActionsRequiringPermissions"/> is allowed.
+    /// </summary>
+    public enum PermissionsCheckResult
+    {


Do we need this class, or is a bool (success/failure) sufficient?

Seems intended to provide 4 possible results:

allowed

exception

on-behalf-of operation not allowed

entity operation not allowed

I'm not sure we need to distinguish 3 & 4, and 2 seems like it should be normal exception handling.

I think this enum is necessary to provide the necessary messaging to the user.

"You don't have permission to upload new versions on behalf of 'y'." or "User 'y' does not have permissions to upload new versions of 'x'." are much more descriptive than "You don't have permissions to do something but we don't know what."

chenriksson · 2017-12-12T19:03:07Z

src/NuGetGallery/Services/PermissionsRequirement.cs

+        /// <summary>
+        /// No user can satisfy the requirement.
+        /// </summary>
+        Unsatisfiable = 0,


How will this be used? If a requirement can't be satisfied, then we don't support it?

exists for syntactical cleanliness

skofman1 · 2017-12-12T19:43:54Z

src/NuGetGallery/Services/ActionRequiringReservedNamespacePermissions.cs

+        public ActionOnNewPackageContext(string packageId, IReservedNamespaceService reservedNamespaceService)
+        {
+            PackageId = packageId;
+            ReservedNamespaceService = reservedNamespaceService;


scottbommarito · 2017-12-13T01:08:54Z

@joelverhagen @chenriksson @skofman1 @agr

All feedback was addressed.

joelverhagen · 2017-12-13T19:04:22Z

src/NuGetGallery/Services/ActionRequiringReservedNamespacePermissions.cs

+    /// <summary>
+    /// Context object for checking permissions of an action involving a new package ID.
+    /// </summary>
+    public class ActionOnNewPackageContext


one class per file. makes github search by file much easier

chenriksson · 2017-12-13T22:54:01Z

src/NuGetGallery/Helpers/PermissionsHelpers.cs

+                .SelectMany(o => o.Members)
+                .Where(m => isUserMatch(m.Member))
+                .ToList()
+                .AsReadOnly();


Why do you need AsReadOnly? Seems like an unnecessary allocation for a collection only used internally.

I was thinking it might be cleaner if we guaranteed this was read-only but I suppose it doesn't really matter.

chenriksson · 2017-12-13T22:59:52Z

src/NuGetGallery/Services/ActionsRequiringPermissions.cs

+    {
+        private const PermissionsRequirement RequireOwnerOrSiteAdmin = PermissionsRequirement.Owner | PermissionsRequirement.SiteAdmin;
+        private const PermissionsRequirement RequireOwnerOrOrganizationAdmin = PermissionsRequirement.Owner | PermissionsRequirement.OrganizationAdmin;
+        private const PermissionsRequirement RequireOwnerOrOrganizationMember = PermissionsRequirement.Owner | PermissionsRequirement.OrganizationAdmin | PermissionsRequirement.OrganizationCollaborator;


nit: wrap lines?

chenriksson · 2017-12-13T23:08:05Z

src/NuGetGallery/Services/ActionRequiringEntityPermissions.cs

+        public bool TryGetAccountsIsAllowedOnBehalfOf(User currentUser, TEntity entity, out IEnumerable<User> accountsAllowedOnBehalfOf)
+        {
+            var accountsAllowedOnBehalfOfList = new List<User>();
+            accountsAllowedOnBehalfOf = accountsAllowedOnBehalfOfList;


why do you need the local variable?

the out parameter is an IEnumerable<User> and not a List<User>, so I need to either store this local variable or cast the out parameter.

I'll switch to casting the out parameter instead of having another local variable, it looks cleaner.

chenriksson · 2017-12-13T23:09:29Z

src/NuGetGallery/Services/ActionRequiringEntityPermissions.cs

+            foreach (var accountOnBehalfOf in possibleAccountsOnBehalfOf)
+            {
+                var failure = CheckPermissions(currentUser, accountOnBehalfOf, entity);
+                if (failure == PermissionsCheckResult.Allowed)


nit: failure sounds odd; maybe permissionsResult?

the enum used to be named PermissionsFailure and was renamed PermissionsCheckResult but this variable was never updated. It is now updated to result.

chenriksson · 2017-12-13T23:14:08Z

src/NuGetGallery/Services/ActionRequiringEntityPermissions.cs

+                        .Concat(currentUser.Organizations.Select(o => o.Organization));
+            }
+
+            possibleAccountsOnBehalfOf = possibleAccountsOnBehalfOf.Distinct(new UserEqualityComparer());


Why do we need Distinct here?

The membership table has PK of (OrganizationKey, MemberKey), so I don't think a User can have multiple memberships in the same Organization.

This is a distinct on not only the user's organizations, but also the owners of the entity we are checking permissions of (e.g. the owners of the package). Therefore, there can be duplicates.

chenriksson · 2017-12-13T23:35:50Z

src/NuGetGallery/Services/IActionRequiringEntityPermissions.cs

+    {
+        PermissionsCheckResult CheckPermissions(User currentUser, User account, TEntity entity);
+        PermissionsCheckResult CheckPermissions(IPrincipal currentPrincipal, User account, TEntity entity);
+        bool TryGetAccountsIsAllowedOnBehalfOf(User currentUser, TEntity entity, out IEnumerable<User> accountsAllowedOnBehalfOf);


Should this be TryGetAccountsAllowedOnBehalfOf?

chenriksson · 2017-12-14T00:03:34Z

@scottbommarito My only remaining feedback is with the distinct call... please look at that, then I'm good.

scottbommarito requested review from joelverhagen, agr, loic-sharma, xavierdecoster, shishirx34, nugetlurker, ryuyu, chenriksson, cristinamanum and skofman1 December 9, 2017 01:03

scottbommarito commented Dec 9, 2017

View reviewed changes

joelverhagen reviewed Dec 11, 2017

View reviewed changes

joelverhagen requested changes Dec 11, 2017

View reviewed changes

skofman1 reviewed Dec 12, 2017

View reviewed changes

cristinamanum reviewed Dec 12, 2017

View reviewed changes

agr reviewed Dec 12, 2017

View reviewed changes

chenriksson suggested changes Dec 12, 2017

View reviewed changes

skofman1 reviewed Dec 12, 2017

View reviewed changes

Scott Bommarito added 6 commits December 12, 2017 16:52

add actions requiring permissions

302eaec

pr fixes

7d1624e

permissionlevelsintersect -> wouldsatisfy

80aa059

fix permissionshelpers tests

b6c1b66

pr comments

3bdc6a0

const requirements

ea9b257

scottbommarito force-pushed the sb-permserv2 branch from 59e9650 to ea9b257 Compare December 13, 2017 00:52

better comments for reservenamespacepermissions

a515be3

joelverhagen reviewed Dec 13, 2017

View reviewed changes

joelverhagen approved these changes Dec 13, 2017

View reviewed changes

actiononnewpackagecontext in its own file

3dc7eb0

chenriksson reviewed Dec 13, 2017

View reviewed changes

chenriksson approved these changes Dec 14, 2017

View reviewed changes

style fixes

e55b725

scottbommarito merged commit 377e115 into sb-feature-orgpermissions Dec 14, 2017

scottbommarito pushed a commit that referenced this pull request Dec 21, 2017

PermissionsService 2.0 (just implementation) (#5158)

9619934

scottbommarito pushed a commit that referenced this pull request Dec 21, 2017

PermissionsService 2.0 (just implementation) (#5158)

9de1085

scottbommarito pushed a commit that referenced this pull request Dec 21, 2017

PermissionsService 2.0 (just implementation) (#5158)

10fd4e1

scottbommarito pushed a commit that referenced this pull request Jan 2, 2018

PermissionsService 2.0 (just implementation) (#5158)

8a43c6a

scottbommarito pushed a commit that referenced this pull request Jan 2, 2018

PermissionsService 2.0 (just implementation) (#5158)

9c67c29

scottbommarito pushed a commit that referenced this pull request Jan 8, 2018

PermissionsService 2.0 (just implementation) (#5158)

d3391b2

scottbommarito deleted the sb-permserv2 branch May 1, 2018 19:50

scottbommarito restored the sb-permserv2 branch July 22, 2019 23:18

scottbommarito deleted the sb-permserv2 branch July 22, 2019 23:19

PermissionsService 2.0 (just implementation) #5158

PermissionsService 2.0 (just implementation) #5158

Conversation

scottbommarito commented Dec 9, 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

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

agr Dec 12, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottbommarito Dec 12, 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

scottbommarito commented Dec 13, 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

agr Dec 12, 2017 •

edited

Loading

scottbommarito Dec 12, 2017 •

edited

Loading