[Organizations] Add Organization #5526

scottbommarito · 2018-02-21T19:11:46Z

Allows users to create their own organizations.

Manage Organizations page (with "Add" link):

Manage Organizations page (with "Add" link and existing orgs):

Create Organization page:

skofman1 · 2018-02-21T21:16:41Z

src/NuGetGallery/Services/IUserService.cs

@@ -41,5 +41,7 @@ public interface IUserService
        Task RequestTransformToOrganizationAccount(User accountToTransform, User adminUser);

        Task<bool> TransformUserToOrganization(User accountToTransform, User adminUser, string token);
+
+        Task<Organization> CreateOrganization(string username, string emailAddress, User adminUser);


organizationName

I think username or name is appropriate here because the fact that this is the organization's name and email is implied by the fact that the method is named CreateOrganization.

I noticed the UserService method has organizationName however so I will align it with here.

skofman1 · 2018-02-21T21:26:03Z

src/NuGetGallery/Services/UserService.cs

+            if (existingUsers.Any(u => u.EmailAddress == emailAddress))
+            {
+                throw new EntityException(Strings.EmailAddressBeingUsed, emailAddress);
+            }


This will perform two DB queries. The code to create a use uses one query:

NuGetGallery/src/NuGetGallery/Authentication/AuthenticationService.cs

Line 266 in 7774fd3

var existingUser = Entities.Users

I saw that code but thought this was syntactically prettier. Didn't realize that this requires two queries however so I'll revert it back.

skofman1 · 2018-02-21T21:31:17Z

src/NuGetGallery/Services/UserService.cs

+
+            OrganizationRepository.InsertOnCommit(organization);
+
+            if (string.IsNullOrEmpty(GetAzureActiveDirectoryCredentialTenant(adminUser)))


why do we need this check? I thoughs LoginDiscontinuationConfiguration takes care of this.

Nope, LoginDiscontinuationConfiguration only covers checking the whitelists and has nothing to do with AAD tenants.

I'm confused, I thought "Add new organization" flow was open for everybody, not just the configured list, and not just AAD but also MSA.

I thought "add new organization" would be scoped to the same users as the other org features. @anangaur should "add new organization" be open to the public?

We need to discuss with @anangaur about how onboarding changes when we open this to everyone.

Initial onboarding required (internal) Organizations to have an AAD policy such that everyone needed an AAD credential with tenant Id that matched the initial admin. I suspect we're going to change to require AAD or MSA for everyone.

I believe MSA now uses AADv2 with the personal/MSA tenant, so we probably just need to check for both AAD and MSA external credential types.

Discussed this with @anangaur previously. I believe we're going to add a checkbox that says "restrict this to your AAD tenant" or something like that that will enable this behavior. For now, however, this is going to be restricted to internal orgs.

Maybe open issue for making AAD tenant policy optional, since it's required before opening this feature to everyone?

skofman1 · 2018-02-21T21:38:27Z

src/NuGetGallery/Services/UserService.cs

+            }
+
+            // SubscribeOrganizationToTenantPolicy will commit
+            if (!await SubscribeOrganizationToTenantPolicy(organization, adminUser))


Consider not counting on SecurityPolicyService.SubscribeAsync to commit, but passing it a flag commit=false, and committing here. This pattern is used a lot in PackageService. Makes the code more predictable and readable.

makes sense, am fixing

skofman1 · 2018-02-21T21:40:36Z

Looks good! I wonder if the UI is friendly enough. Perhaps more text with guidelines, instructions, etc. @anangaur ?

anangaur · 2018-02-21T22:28:30Z

Some observations:

Use Add instead of Create
There is no need to show the Administrator line item.
Can you bring in logo section on the right and suggest that email is used to look up the logo from gravatar as suggested in the storyboard:
There is a different page when no Organizations are present:

scottbommarito · 2018-02-21T23:20:38Z

@anangaur

I think "create" is more appropriate than "add" because "add" implies that you are joining something to something else. "Create", on the other hand, implies that you are bringing something into existence. We are not adding the new organization to anything--we are creating it. Note that we use "create" for making a new account.

For example:
Can you send me the image you would like to display there?

anangaur · 2018-02-22T00:22:02Z

@scottbommarito I feel you add an organization to your account. Most likely adding your team name as an organization on NuGet.org. You most likely are not creating a new org. This is similar to whether you add or create a new package to your account. Subjective though. Glancing through other implementations, appcenter.ms also adds :)

scottbommarito · 2018-02-22T15:59:31Z

@anangaur Alright, will use add!

joelverhagen · 2018-02-22T21:08:12Z

src/NuGetGallery/Scripts/gallery/page-add-organization.js

+        var src = defaultImage;
+
+        var email = emailBox.val();
+        if (email.match(/^([0-9a-zA-Z]([-.\w]*[0-9a-zA-Z])*@([0-9a-zA-Z][-\w]*[0-9a-zA-Z]\.)+[a-zA-Z]{2,9})$/)) {


this should be simpler or come from server side

Found an implementation in RegisterViewModel that I have moved to Constants and passed in here.

joelverhagen · 2018-02-22T21:08:45Z

src/NuGetGallery/Scripts/gallery/page-add-organization.js

+        }
+    });
+
+    function md5(s) { function L(k, d) { return (k << d) | (k >>> (32 - d)) } function K(G, k) { var I, d, F, H, x; F = (G & 2147483648); H = (k & 2147483648); I = (G & 1073741824); d = (k & 1073741824); x = (G & 1073741823) + (k & 1073741823); if (I & d) { return (x ^ 2147483648 ^ F ^ H) } if (I | d) { if (x & 1073741824) { return (x ^ 3221225472 ^ F ^ H) } else { return (x ^ 1073741824 ^ F ^ H) } } else { return (x ^ F ^ H) } } function r(d, F, k) { return (d & F) | ((~d) & k) } function q(d, F, k) { return (d & k) | (F & (~k)) } function p(d, F, k) { return (d ^ F ^ k) } function n(d, F, k) { return (F ^ (d | (~k))) } function u(G, F, aa, Z, k, H, I) { G = K(G, K(K(r(F, aa, Z), k), I)); return K(L(G, H), F) } function f(G, F, aa, Z, k, H, I) { G = K(G, K(K(q(F, aa, Z), k), I)); return K(L(G, H), F) } function D(G, F, aa, Z, k, H, I) { G = K(G, K(K(p(F, aa, Z), k), I)); return K(L(G, H), F) } function t(G, F, aa, Z, k, H, I) { G = K(G, K(K(n(F, aa, Z), k), I)); return K(L(G, H), F) } function e(G) { var Z; var F = G.length; var x = F + 8; var k = (x - (x % 64)) / 64; var I = (k + 1) * 16; var aa = Array(I - 1); var d = 0; var H = 0; while (H < F) { Z = (H - (H % 4)) / 4; d = (H % 4) * 8; aa[Z] = (aa[Z] | (G.charCodeAt(H) << d)); H++ } Z = (H - (H % 4)) / 4; d = (H % 4) * 8; aa[Z] = aa[Z] | (128 << d); aa[I - 2] = F << 3; aa[I - 1] = F >>> 29; return aa } function B(x) { var k = "", F = "", G, d; for (d = 0; d <= 3; d++) { G = (x >>> (d * 8)) & 255; F = "0" + G.toString(16); k = k + F.substr(F.length - 2, 2) } return k } function J(k) { k = k.replace(/rn/g, "n"); var d = ""; for (var F = 0; F < k.length; F++) { var x = k.charCodeAt(F); if (x < 128) { d += String.fromCharCode(x) } else { if ((x > 127) && (x < 2048)) { d += String.fromCharCode((x >> 6) | 192); d += String.fromCharCode((x & 63) | 128) } else { d += String.fromCharCode((x >> 12) | 224); d += String.fromCharCode(((x >> 6) & 63) | 128); d += String.fromCharCode((x & 63) | 128) } } } return d } var C = Array(); var P, h, E, v, g, Y, X, W, V; var S = 7, Q = 12, N = 17, M = 22; var A = 5, z = 9, y = 14, w = 20; var o = 4, m = 11, l = 16, j = 23; var U = 6, T = 10, R = 15, O = 21; s = J(s); C = e(s); Y = 1732584193; X = 4023233417; W = 2562383102; V = 271733878; for (P = 0; P < C.length; P += 16) { h = Y; E = X; v = W; g = V; Y = u(Y, X, W, V, C[P + 0], S, 3614090360); V = u(V, Y, X, W, C[P + 1], Q, 3905402710); W = u(W, V, Y, X, C[P + 2], N, 606105819); X = u(X, W, V, Y, C[P + 3], M, 3250441966); Y = u(Y, X, W, V, C[P + 4], S, 4118548399); V = u(V, Y, X, W, C[P + 5], Q, 1200080426); W = u(W, V, Y, X, C[P + 6], N, 2821735955); X = u(X, W, V, Y, C[P + 7], M, 4249261313); Y = u(Y, X, W, V, C[P + 8], S, 1770035416); V = u(V, Y, X, W, C[P + 9], Q, 2336552879); W = u(W, V, Y, X, C[P + 10], N, 4294925233); X = u(X, W, V, Y, C[P + 11], M, 2304563134); Y = u(Y, X, W, V, C[P + 12], S, 1804603682); V = u(V, Y, X, W, C[P + 13], Q, 4254626195); W = u(W, V, Y, X, C[P + 14], N, 2792965006); X = u(X, W, V, Y, C[P + 15], M, 1236535329); Y = f(Y, X, W, V, C[P + 1], A, 4129170786); V = f(V, Y, X, W, C[P + 6], z, 3225465664); W = f(W, V, Y, X, C[P + 11], y, 643717713); X = f(X, W, V, Y, C[P + 0], w, 3921069994); Y = f(Y, X, W, V, C[P + 5], A, 3593408605); V = f(V, Y, X, W, C[P + 10], z, 38016083); W = f(W, V, Y, X, C[P + 15], y, 3634488961); X = f(X, W, V, Y, C[P + 4], w, 3889429448); Y = f(Y, X, W, V, C[P + 9], A, 568446438); V = f(V, Y, X, W, C[P + 14], z, 3275163606); W = f(W, V, Y, X, C[P + 3], y, 4107603335); X = f(X, W, V, Y, C[P + 8], w, 1163531501); Y = f(Y, X, W, V, C[P + 13], A, 2850285829); V = f(V, Y, X, W, C[P + 2], z, 4243563512); W = f(W, V, Y, X, C[P + 7], y, 1735328473); X = f(X, W, V, Y, C[P + 12], w, 2368359562); Y = D(Y, X, W, V, C[P + 5], o, 4294588738); V = D(V, Y, X, W, C[P + 8], m, 2272392833); W = D(W, V, Y, X, C[P + 11], l, 1839030562); X = D(X, W, V, Y, C[P + 14], j, 4259657740); Y = D(Y, X, W, V, C[P + 1], o, 2763975236); V = D(V, Y, X, W, C[P + 4], m, 1272893353); W = D(W, V, Y, X, C[P + 7], l, 4139469664); X = D(X, W, V, Y, C[P + 10], j, 3200236656); Y = D(Y, X, W, V, C[P + 13], o, 681279174); V = D(V, Y, X, W, C[P + 0], m, 3936430074); W = D(W, V, Y, X, C[P + 3], l, 3572445317); X = D(X, W, V, Y, C[P + 6], j, 76029189); Y = D(Y, X, W, V, C[P + 9], o, 3654602809); V = D(V, Y, X, W, C[P + 12], m, 3873151461); W = D(W, V, Y, X, C[P + 15], l, 530742520); X = D(X, W, V, Y, C[P + 2], j, 3299628645); Y = t(Y, X, W, V, C[P + 0], U, 4096336452); V = t(V, Y, X, W, C[P + 7], T, 1126891415); W = t(W, V, Y, X, C[P + 14], R, 2878612391); X = t(X, W, V, Y, C[P + 5], O, 4237533241); Y = t(Y, X, W, V, C[P + 12], U, 1700485571); V = t(V, Y, X, W, C[P + 3], T, 2399980690); W = t(W, V, Y, X, C[P + 10], R, 4293915773); X = t(X, W, V, Y, C[P + 1], O, 2240044497); Y = t(Y, X, W, V, C[P + 8], U, 1873313359); V = t(V, Y, X, W, C[P + 15], T, 4264355552); W = t(W, V, Y, X, C[P + 6], R, 2734768916); X = t(X, W, V, Y, C[P + 13], O, 1309151649); Y = t(Y, X, W, V, C[P + 4], U, 4149444226); V = t(V, Y, X, W, C[P + 11], T, 3174756917); W = t(W, V, Y, X, C[P + 2], R, 718787259); X = t(X, W, V, Y, C[P + 9], O, 3951481745); Y = K(Y, h); X = K(X, E); W = K(W, v); V = K(V, g) } var i = B(Y) + B(X) + B(W) + B(V); return i.toLowerCase() };


we need source, license sign off on this.

alternatively, make an endpoint on server side that says "give me a gravatar URL for this email"

Spoke with @skofman1 and found a version of MD5 with source and an MIT license. Have moved it into a separate file (md5.js) and added it to the page-add-organization bundle.

joelverhagen · 2018-02-22T22:23:23Z

src/NuGetGallery/Views/Organizations/Add.cshtml

+            <div>
+                <aside class="col-sm-3 col-sm-push-9">
+                    @Html.Label("Logo")
+                    <img src="@Url.Absolute("~/Content/gallery/img/default-package-icon-256x256.png")" class="owner-image img-responsive" height="332" id="gravatar-image" width="332" alt="gravatar" title="Organization logo">


SVG first, fall back to PNG.

Also, the height and width should match the PNG to avoid distortion

chenriksson · 2018-02-27T16:32:32Z

src/NuGetGallery/Views/Users/Organizations.cshtml

@@ -4,16 +4,25 @@
    Layout = "~/Views/Shared/Gallery/Layout.cshtml";
 }

-<section role="main" class="container main-container manage-packages">
+<section role="main" class="container main-container page-manage-packages">


Should we introduce page-manage-organizations? Or move common logic to something like common-list-view.less?

I think given that it's only used in two places it's fine to stay in its existing file. If a third page uses this we should move it to a shared file.

chenriksson · 2018-02-27T16:40:01Z

src/NuGetGallery/Controllers/OrganizationsController.cs

+            }
+            catch (Exception e)
+            {
+                errorMessage = e.GetUserSafeMessage();


Do we normally handle the general exceptions in our action methods? Is there a specific, known exception we're handling here?

Removed this and made the exception thrown if subscribing to the policy fails an EntityException instead.

chenriksson · 2018-02-27T16:42:53Z

src/NuGetGallery/ViewModels/AddOrganizationViewModel.cs

+
+namespace NuGetGallery
+{
+    public class AddOrganizationViewModel


Registering an organization account is very similar to registering a user account... so you could try to reuse code. For instance, why not reuse the RegisterViewModel, and then use inheritance to create a User-specific RegisterUserViewModel that also takes a password.

This might reduce the size of this change too - for instance, regex logic could remain on RegisterViewModel.

That's a good idea but

The regex still needs to move because it's used by the script

I'm not sure how to deal with the fact that the Display annotations for the two different models need to be different (e.g. register account's username shows "username" and not "organization name")

Also with the introduction of the recent AAD/MSA changes the register flow is going to be very different from this, so I think it makes sense to keep it separate.

chenriksson · 2018-02-27T16:49:27Z

src/NuGetGallery/Scripts/gallery/md5.js

+ *
+ **/
+
+(function (exports) {


This is just for displaying the gravatar at org creation time?

IMHO, I don't think we should do this... at least not as part of the same PR. Organization registration is very similar to User registration, so I think we should reuse as much of the code as possible in this PR. I would add the gravatar in a separate PR, and consider whether we should also do it for User registrations.

@anangaur asked for it specifically. It shows the Gravatar as you fill in the form.

With the MSA/AAD changes, organization registration flow is going to be a lot different than the register flow on the site--e.g. you don't enter an email, it gets it from the MSA account automatically--so I don't think it's super important to separate these PRs.

chenriksson · 2018-02-27T16:56:44Z

src/NuGetGallery/Services/UserService.cs

+
+            OrganizationRepository.InsertOnCommit(organization);
+
+            if (string.IsNullOrEmpty(GetAzureActiveDirectoryCredentialTenant(adminUser)))


We need to discuss with @anangaur about how onboarding changes when we open this to everyone.

Initial onboarding required (internal) Organizations to have an AAD policy such that everyone needed an AAD credential with tenant Id that matched the initial admin. I suspect we're going to change to require AAD or MSA for everyone.

I believe MSA now uses AADv2 with the personal/MSA tenant, so we probably just need to check for both AAD and MSA external credential types.

chenriksson

LGTM!

chenriksson · 2018-03-01T19:00:01Z

src/NuGetGallery/Services/IUserService.cs

@@ -41,5 +41,7 @@ public interface IUserService
        Task RequestTransformToOrganizationAccount(User accountToTransform, User adminUser);

        Task<bool> TransformUserToOrganization(User accountToTransform, User adminUser, string token);
+
+        Task<Organization> AddOrganization(string username, string emailAddress, User adminUser);


nit: would name AddOrganizationAsync... (I realize I didn't do a good job of this above)

chenriksson · 2018-03-01T19:03:38Z

src/NuGetGallery/Services/UserService.cs

+
+            OrganizationRepository.InsertOnCommit(organization);
+
+            if (string.IsNullOrEmpty(GetAzureActiveDirectoryCredentialTenant(adminUser)))


Maybe open issue for making AAD tenant policy optional, since it's required before opening this feature to everyone?

Scott Bommarito added 2 commits February 21, 2018 10:46

create organization page

2a35784

unit tests

a0d27cd

scottbommarito requested review from joelverhagen, agr, loic-sharma, xavierdecoster, shishirx34, nugetlurker, ryuyu, chenriksson, cristinamanum and skofman1 February 21, 2018 19:11

fix subtitle formatting

585be0b

skofman1 reviewed Feb 21, 2018

View reviewed changes

Scott Bommarito added 2 commits February 21, 2018 15:35

add commitChanges to security policy service

21d3b9f

remove administrator from create org form

cc61b7f

create -> add, logo changes based on email address

8ac19ff

joelverhagen reviewed Feb 22, 2018

View reviewed changes

style fixes

d168f55

joelverhagen reviewed Feb 22, 2018

View reviewed changes

joelverhagen approved these changes Feb 22, 2018

View reviewed changes

Scott Bommarito added 2 commits February 27, 2018 08:45

align empty organizations page with spec

19fbcc4

add subtext to organization add page

eb8625e

chenriksson reviewed Feb 27, 2018

View reviewed changes

scottbommarito changed the title ~~[Organizations] Create Organization~~ [Organizations] Add Organization Feb 27, 2018

Scott Bommarito added 2 commits February 27, 2018 12:38

remove getusersafe exception catching from Add

6603ffc

usersafeexception -> entityexception

c8255f2

chenriksson approved these changes Mar 1, 2018

View reviewed changes

Scott Bommarito added 2 commits March 1, 2018 11:58

addorganizationasync

17dabf7

Merge branch 'dev' into sb-orgadd

d62f72f

scottbommarito merged commit dc09a1d into dev Mar 1, 2018

scottbommarito deleted the sb-orgadd branch March 1, 2018 20:26

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

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


		OrganizationRepository.InsertOnCommit(organization);

		if (string.IsNullOrEmpty(GetAzureActiveDirectoryCredentialTenant(adminUser)))

[Organizations] Add Organization #5526

[Organizations] Add Organization #5526

Conversation

scottbommarito commented Feb 21, 2018 • 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

skofman1 commented Feb 21, 2018

anangaur commented Feb 21, 2018

scottbommarito commented Feb 21, 2018 • edited Loading

anangaur commented Feb 22, 2018 • edited Loading

scottbommarito commented Feb 22, 2018

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 Feb 27, 2018 • 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

chenriksson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottbommarito commented Feb 21, 2018 •

edited

Loading

scottbommarito commented Feb 21, 2018 •

edited

Loading

anangaur commented Feb 22, 2018 •

edited

Loading

scottbommarito Feb 27, 2018 •

edited

Loading