Schema for organization membership requests #5204
Conversation
|
Let me know when to review this--I assume schema is going to change slightly after the discussion we had today. |
|
@scottbommarito Per our discussion, I'll see if I can persist the |
| @@ -22,6 +22,8 @@ namespace NuGetGallery.Authentication | |||
| { | |||
| public class AuthenticationService | |||
| { | |||
| private const string tenantIdClaimType = "http://schemas.microsoft.com/identity/claims/tenantid"; | |||
There was a problem hiding this comment.
I believe the claim type will change with my AAD work, this is fine for now.
chenriksson
requested review from
agr,
shishirx34,
ryuyu,
skofman1,
scottbommarito and
anangaur
and removed request for
anangaur
|
@shishirx34 Do you know if tenantId would ever change? I'm guessing only if someone creates a new AAD tenant... |
|
I don't think the tenantid would change. |
|
@scottbommarito This is ready for review when you have a chance. I removed TenantId column from the Organizations table, and will instead do security policies in a separate PR. |
| @@ -2,6 +2,7 @@ | |||
| // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | |||
|
|
|||
| using System.Collections.Generic; | |||
| using System.ComponentModel.DataAnnotations; | |||
There was a problem hiding this comment.
nit: is this needed?
| /// <summary> | ||
| /// Organization membership requests, for a non-organization <see cref="User"/> account. | ||
| /// </summary> | ||
| public virtual ICollection<MembershipRequest> OrganizationRequests { get; set; } |
There was a problem hiding this comment.
These are requests for a user to become an organization, with an admin specified by the request, correct?
I would explicitly state that. Perhaps name this field OrganizationMigrationRequest or something.
There was a problem hiding this comment.
OrganizationRequests are requests to join an organization.
Now that there are separate tables, I have OrganizationMigrationRequest below for the migration. Another bonus I see for the separate tables is that I'm also able to constrain this to an optional 1:1 relation, so there's only ever 1 migration request for an account... I like that.
There was a problem hiding this comment.
There's also Organization.MemberRequests now, which are requests to join an organization - from the Organization direction.
There was a problem hiding this comment.
Oh so these are requests to join an organization AFTER it's migrated?
| /// | ||
| /// Relationship is on the User table because organization transformation may not be complete. | ||
| /// </summary> | ||
| public virtual ICollection<MembershipRequest> MemberRequests { get; set; } |
There was a problem hiding this comment.
The more I look at this the more concerned I am that keeping MembershipRequest a single class for both organization migration and adding members to existing organizations instead of two separate classes is ultimately more confusing to use. There should only be one OrganizationMigrationRequest for a user at once and MembershipRequests should only be connected to an existing Organization. Given this API, neither of those things are guaranteed, and the code will have to constantly check that those conditions are not violated.
There was a problem hiding this comment.
Going to split into 2 separate request tables.
| @@ -29,5 +29,10 @@ public Organization(string name) : base(name) | |||
| /// Organization Memberships to this organization. | |||
| /// </summary> | |||
| public virtual ICollection<Membership> Members { get; set; } | |||
|
|
|||
| /// <summary> | |||
| /// Organization membership requests, for an organization <see cref="Organization"/> account. | |||
There was a problem hiding this comment.
for an organization is now redundant given this is on the Organization object.
| /// <summary> | ||
| /// Organization membership requests, for a non-organization <see cref="User"/> account. | ||
| /// </summary> | ||
| public virtual ICollection<MembershipRequest> OrganizationRequests { get; set; } |
There was a problem hiding this comment.
Oh so these are requests to join an organization AFTER it's migrated?
New membership request table will be used for confirming membership in organizations. Request can result from an organization adding new members, or from the initial migration where the first admin account is added. Design meeting scheduled for tomorrow will include this.
Also adds tenantId column to Credential, which can be used to enforce organization policy that AAD tenant ids match.
/CC: @anangaur