Merge pull request #52 from unisoncomputing/cp/org-role-constraints

Add additional constraints on org role/member management

Author: ChrisPenner

src/Share/Web/Share/Orgs/Impl.hs

@@ -1,3 +1,5 @@
+{-# LANGUAGE DataKinds #-}
+
 module Share.Web.Share.Orgs.Impl (server) where
 
 import Control.Lens
@@ -10,6 +12,7 @@ import Share.Postgres qualified as PG
 import Share.Postgres.Users.Queries qualified as UserQ
 import Share.Prelude
 import Share.User (User (..))
+import Share.Utils.Logging qualified as Logging
 import Share.Web.App
 import Share.Web.Authorization qualified as AuthZ
 import Share.Web.Authorization.Types
@@ -23,6 +26,29 @@ import Share.Web.Share.Roles (canonicalRoleAssignmentOrdering)
 import Share.Web.Share.Roles.Queries (displaySubjectsOf)
 import Unison.Util.Set qualified as Set
 
+data OrgError
+  = OrgMemberOfOrgError
+  | OrgMustHaveOwnerError
+  deriving stock (Show, Eq)
+  deriving (Logging.Loggable) via (Logging.ShowLoggable Logging.UserFault OrgError)
+
+instance ToServerError OrgError where
+  toServerError = \case
+    OrgMemberOfOrgError ->
+      ( ErrorID "org:org-member-of-org",
+        err400
+          { errBody = "Cannot add an org as a member of another org.",
+            errReasonPhrase = "Invalid Org Member"
+          }
+      )
+    OrgMustHaveOwnerError ->
+      ( ErrorID "org:must-have-owner",
+        err400
+          { errBody = "Cannot remove the only owner of an org.",
+            errReasonPhrase = "Invalid Org Member"
+          }
+      )
+
 server :: ServerT API.API WebApp
 server =
   let orgResourceServer orgHandle =
@@ -75,6 +101,7 @@ addRolesEndpoint :: UserHandle -> UserId -> AddRolesRequest -> WebApp ListRolesR
 addRolesEndpoint orgHandle caller (AddRolesRequest {roleAssignments}) = do
   orgId <- orgIdByHandle orgHandle
   _authZReceipt <- AuthZ.permissionGuard $ AuthZ.checkEditOrgRoles caller orgId
+  assertNoOrgSubjects roleAssignments
   PG.runTransaction do
     orgRoles <- OrgQ.addOrgRoles orgId roleAssignments
     ListRolesResponse True . canonicalRoleAssignmentOrdering <$> displaySubjectsOf (traversed . traversed) orgRoles
@@ -83,8 +110,12 @@ removeRolesEndpoint :: UserHandle -> UserId -> RemoveRolesRequest -> WebApp List
 removeRolesEndpoint orgHandle caller (RemoveRolesRequest {roleAssignments}) = do
   orgId <- orgIdByHandle orgHandle
   _authZReceipt <- AuthZ.permissionGuard $ AuthZ.checkEditOrgRoles caller orgId
-  PG.runTransaction do
+  PG.runTransactionOrRespondError do
     orgRoles <- OrgQ.removeOrgRoles orgId roleAssignments
+    OrgQ.doesOrgHaveOwner orgId >>= \case
+      False -> throwError OrgMustHaveOwnerError
+      True -> pure ()
+
     ListRolesResponse True . canonicalRoleAssignmentOrdering <$> displaySubjectsOf (traversed . traversed) orgRoles
 
 listMembersEndpoint :: UserHandle -> UserId -> WebApp OrgMembersListResponse
@@ -98,8 +129,12 @@ addMembersEndpoint :: UserHandle -> UserId -> OrgMembersAddRequest -> WebApp Org
 addMembersEndpoint orgHandle caller (OrgMembersAddRequest {members}) = do
   orgId <- orgIdByHandle orgHandle
   _authZReceipt <- AuthZ.permissionGuard $ AuthZ.checkEditOrgMembers caller orgId
-  PG.runTransaction do
+  PG.runTransactionOrRespondError do
     userIds <- UserQ.userIdsByHandlesOf Set.traverse (Set.fromList members)
+    hasOrgMember <- runMaybeT $ for_ userIds \userId -> do
+      MaybeT $ OrgQ.orgByUserId userId
+    when (isJust hasOrgMember) do
+      throwError OrgMemberOfOrgError
     OrgQ.addOrgMembers orgId userIds
     OrgMembersListResponse <$> OrgQ.listOrgMembers orgId
 
@@ -111,3 +146,15 @@ removeMembersEndpoint orgHandle caller (OrgMembersRemoveRequest {members}) = do
     userIds <- UserQ.userIdsByHandlesOf Set.traverse (Set.fromList members)
     OrgQ.removeOrgMembers orgId userIds
     OrgMembersListResponse <$> OrgQ.listOrgMembers orgId
+
+assertNoOrgSubjects :: [RoleAssignment ResolvedAuthSubject] -> WebApp ()
+assertNoOrgSubjects roleAssignments = do
+  let hasOrgSubject =
+        roleAssignments
+          & any
+            ( \RoleAssignment {subject} -> case subject of
+                OrgSubject {} -> True
+                _ -> False
+            )
+  when hasOrgSubject do
+    respondError OrgMemberOfOrgError

src/Share/Web/Share/Orgs/Queries.hs

@@ -11,6 +11,7 @@ module Share.Web.Share.Orgs.Queries
     removeOrgMembers,
     orgDisplayInfoOf,
     userDisplayInfoByOrgIdOf,
+    doesOrgHaveOwner,
   )
 where
 
@@ -178,3 +179,17 @@ removeOrgMembers orgId toRemove = do
           WHERE org_members.member_user_id = v.member_user_id
             AND org_members.org_id = #{orgId}
         |]
+
+doesOrgHaveOwner :: OrgId -> Transaction e Bool
+doesOrgHaveOwner orgId = do
+  queryExpect1Col
+    [sql|
+      SELECT EXISTS (
+        SELECT
+          FROM role_memberships rm
+          JOIN roles role ON rm.role_id = role.id
+          JOIN orgs org ON rm.resource_id = org.resource_id
+        WHERE org.id = #{orgId}
+          AND role.ref = 'org_owner'::role_ref
+      )
+    |]

transcripts/share-apis/orgs/org-cant-have-org-members.json

@@ -0,0 +1,8 @@
+{
+  "body": "Cannot add an org as a member of another org.",
+  "status": [
+    {
+      "status_code": 400
+    }
+  ]
+}

transcripts/share-apis/orgs/run.zsh

@@ -56,6 +56,13 @@ fetch "$transcript_user" POST org-add-members '/orgs/acme/members' '{
   ]
 }'
 
+# Can't add an org to another org
+fetch "$transcript_user" POST org-cant-have-org-members '/orgs/acme/members' '{
+  "members": [
+    "unison"
+  ]
+}'
+
 fetch "$transcript_user" GET org-get-members-after-adding '/orgs/acme/members'
 
 # Owner can remove members
@@ -65,4 +72,6 @@ fetch "$transcript_user" DELETE org-remove-members '/orgs/acme/members' '{
   ]
 }'
 
+
 fetch "$transcript_user" GET org-get-members-after-removing '/orgs/acme/members'
+

transcripts/share-apis/roles/grant-project-contributor.json

@@ -18,7 +18,7 @@
       },
       {
         "roles": [
-          "org_admin"
+          "org_owner"
         ],
         "subject": {
           "data": {

transcripts/share-apis/roles/org-remove-only-owner.json

@@ -0,0 +1,8 @@
+{
+  "body": "Cannot remove the only owner of an org.",
+  "status": [
+    {
+      "status_code": 400
+    }
+  ]
+}

transcripts/share-apis/roles/org-roles-list.json

@@ -4,7 +4,7 @@
     "role_assignments": [
       {
         "roles": [
-          "org_admin"
+          "org_owner"
         ],
         "subject": {
           "data": {

transcripts/share-apis/roles/revoke-project-contributor.json

@@ -4,7 +4,7 @@
     "role_assignments": [
       {
         "roles": [
-          "org_admin"
+          "org_owner"
         ],
         "subject": {
           "data": {

transcripts/share-apis/roles/run.zsh

@@ -15,6 +15,16 @@ fetch "$some_user" GET non-maintainer-project-view '/users/unison/projects/priva
 
 fetch "$test_user" GET org-roles-list '/orgs/unison/roles'
 
+# Org must always have an owner
+fetch "$test_user" DELETE org-remove-only-owner '/orgs/unison/roles' "
+{
+    \"role_assignments\": 
+    [ { \"subject\": {\"kind\": \"user\", \"id\": \"${test_user}\"}
+      , \"roles\": [\"org_owner\"]
+      }
+    ]
+}"
+
 # Giving this user the project_contributor role on the Unison org should give them access to the project owned by that org.
 # Typically you'd add the user to the org, but this is a good way to test JUST the resource role hierarchy.
 fetch "$test_user" POST grant-project-contributor '/orgs/unison/roles' "

transcripts/sql/inserts.sql

@@ -83,7 +83,7 @@ VALUES (
 INSERT INTO role_memberships(subject_id, resource_id, role_id)
   SELECT (SELECT u.subject_id FROM users u WHERE u.handle = 'test'),
          (SELECT org.resource_id FROM orgs org JOIN users orgu ON org.user_id = orgu.id WHERE orgu.handle = 'unison'), 
-         (SELECT r.id FROM roles r WHERE r.ref = 'org_admin');
+         (SELECT r.id FROM roles r WHERE r.ref = 'org_owner');
 
 INSERT INTO tours (
   user_id,