Commit 4046bc84 authored by Alfredo Di Napoli's avatar Alfredo Di Napoli

Fix policy checks with `isSharedWith`

This commit introduces a new DB function called `isSharedWith`
that can be used to check for team/public/private nodes and their
shared resources.
parent c085b660
...@@ -73,6 +73,8 @@ data AccessCheck ...@@ -73,6 +73,8 @@ data AccessCheck
= -- | Grants access if the input 'NodeId' is a descendant of the = -- | Grants access if the input 'NodeId' is a descendant of the
-- one for the logged-in user. -- one for the logged-in user.
AC_node_descendant NodeId AC_node_descendant NodeId
-- | Grants access if the input 'NodeId' is shared with the logged-in user.
| AC_node_shared NodeId
-- | Grants access if the input 'NodeId' /is/ the logged-in user. -- | Grants access if the input 'NodeId' /is/ the logged-in user.
| AC_user_node NodeId | AC_user_node NodeId
-- | Grants access if the logged-in user is the master user. -- | Grants access if the logged-in user is the master user.
...@@ -119,13 +121,14 @@ accessPolicyManager = AccessPolicyManager (\ur ac -> interpretPolicy ur ac) ...@@ -119,13 +121,14 @@ accessPolicyManager = AccessPolicyManager (\ur ac -> interpretPolicy ur ac)
check :: HasNodeError err => AuthenticatedUser -> AccessCheck -> DBCmd err AccessResult check :: HasNodeError err => AuthenticatedUser -> AccessCheck -> DBCmd err AccessResult
check (AuthenticatedUser loggedUserNodeId _loggedUserUserId) = \case check (AuthenticatedUser loggedUserNodeId loggedUserUserId) = \case
AC_always_deny AC_always_deny
-> pure $ Deny err500 -> pure $ Deny err500
AC_always_allow AC_always_allow
-> pure Allow -> pure Allow
AC_user_node requestedNodeId AC_user_node requestedNodeId
-> enforce err403 $ loggedUserNodeId == requestedNodeId -> do ownedByMe <- requestedNodeId `isOwnedBy` loggedUserUserId
enforce err403 $ (loggedUserNodeId == requestedNodeId || ownedByMe)
AC_master_user _requestedNodeId AC_master_user _requestedNodeId
-> do -> do
masterUsername <- _gc_masteruser <$> view hasConfig masterUsername <- _gc_masteruser <$> view hasConfig
...@@ -133,6 +136,8 @@ check (AuthenticatedUser loggedUserNodeId _loggedUserUserId) = \case ...@@ -133,6 +136,8 @@ check (AuthenticatedUser loggedUserNodeId _loggedUserUserId) = \case
enforce err403 $ masterNodeId == loggedUserNodeId enforce err403 $ masterNodeId == loggedUserNodeId
AC_node_descendant nodeId AC_node_descendant nodeId
-> enforce err403 =<< nodeId `isDescendantOf` loggedUserNodeId -> enforce err403 =<< nodeId `isDescendantOf` loggedUserNodeId
AC_node_shared nodeId
-> enforce err403 =<< nodeId `isSharedWith` loggedUserNodeId
------------------------------------------------------------------------------- -------------------------------------------------------------------------------
-- Smart constructors of access checks -- Smart constructors of access checks
...@@ -147,11 +152,11 @@ nodeSuper = BConst . Positive . AC_master_user ...@@ -147,11 +152,11 @@ nodeSuper = BConst . Positive . AC_master_user
nodeDescendant :: NodeId -> BoolExpr AccessCheck nodeDescendant :: NodeId -> BoolExpr AccessCheck
nodeDescendant = BConst . Positive . AC_node_descendant nodeDescendant = BConst . Positive . AC_node_descendant
-- FIXME(adinapoli) Checks temporarily disabled. nodeShared :: NodeId -> BoolExpr AccessCheck
nodeShared = BConst . Positive . AC_node_shared
nodeChecks :: NodeId -> BoolExpr AccessCheck nodeChecks :: NodeId -> BoolExpr AccessCheck
nodeChecks _nid = alwaysAllow nodeChecks nid = nodeUser nid `BOr` nodeSuper nid `BOr` nodeDescendant nid `BOr` nodeShared nid
where
_disabled = nodeUser _nid `BOr` nodeSuper _nid `BOr` nodeDescendant _nid
alwaysAllow :: BoolExpr AccessCheck alwaysAllow :: BoolExpr AccessCheck
alwaysAllow = BConst . Positive $ AC_always_allow alwaysAllow = BConst . Positive $ AC_always_allow
......
...@@ -65,7 +65,7 @@ resolveNodes ...@@ -65,7 +65,7 @@ resolveNodes
resolveNodes autUser mgr NodeArgs { node_id } = resolveNodes autUser mgr NodeArgs { node_id } =
-- FIXME(adn) We should have a way to enforce the access policy on -- FIXME(adn) We should have a way to enforce the access policy on
-- the public or public folders, instead of using 'alwaysAllow'. -- the public or public folders, instead of using 'alwaysAllow'.
withPolicy autUser mgr alwaysAllow $ dbNodes node_id withPolicy autUser mgr (nodeChecks $ NN.UnsafeMkNodeId node_id) $ dbNodes node_id
resolveNodesCorpus resolveNodesCorpus
:: (CmdCommon env) :: (CmdCommon env)
......
...@@ -71,7 +71,7 @@ resolveTree :: (CmdCommon env) ...@@ -71,7 +71,7 @@ resolveTree :: (CmdCommon env)
resolveTree autUser mgr TreeArgs { root_id } = resolveTree autUser mgr TreeArgs { root_id } =
-- FIXME(adn) We should have a way to enforce the access policy on -- FIXME(adn) We should have a way to enforce the access policy on
-- the public or public folders, instead of using 'alwaysAllow'. -- the public or public folders, instead of using 'alwaysAllow'.
withPolicy autUser mgr alwaysAllow $ dbTree root_id withPolicy autUser mgr (nodeChecks $ UnsafeMkNodeId root_id) $ dbTree root_id
dbTree :: (CmdCommon env) => dbTree :: (CmdCommon env) =>
Int -> GqlM e env (TreeFirstLevel (GqlM e env)) Int -> GqlM e env (TreeFirstLevel (GqlM e env))
......
...@@ -143,7 +143,8 @@ formatPGSQuery q a = mkCmd $ \conn -> PGS.formatQuery conn q a ...@@ -143,7 +143,8 @@ formatPGSQuery q a = mkCmd $ \conn -> PGS.formatQuery conn q a
-- TODO use runPGSQueryDebug everywhere -- TODO use runPGSQueryDebug everywhere
runPGSQuery' :: (PGS.ToRow a, PGS.FromRow b) => PGS.Query -> a -> DBCmd err [b] runPGSQuery' :: (PGS.ToRow a, PGS.FromRow b) => PGS.Query -> a -> DBCmd err [b]
runPGSQuery' q a = mkCmd $ \conn -> PGS.query conn q a runPGSQuery' q a = mkCmd $ \conn -> do
PGS.query conn q a
runPGSQuery :: ( PGS.FromRow r, PGS.ToRow q ) runPGSQuery :: ( PGS.FromRow r, PGS.ToRow q )
=> PGS.Query -> q -> DBCmd err [r] => PGS.Query -> q -> DBCmd err [r]
......
...@@ -20,6 +20,8 @@ Let a Root Node, return the Tree of the Node as a directed acyclic graph ...@@ -20,6 +20,8 @@ Let a Root Node, return the Tree of the Node as a directed acyclic graph
module Gargantext.Database.Query.Tree module Gargantext.Database.Query.Tree
( module Gargantext.Database.Query.Tree.Error ( module Gargantext.Database.Query.Tree.Error
, isDescendantOf , isDescendantOf
, isOwnedBy
, isSharedWith
, isIn , isIn
, tree , tree
, tree_flat , tree_flat
...@@ -377,6 +379,37 @@ isDescendantOf childId rootId = (== [Only True]) ...@@ -377,6 +379,37 @@ isDescendantOf childId rootId = (== [Only True])
WHERE t.id = ?; WHERE t.id = ?;
|] (childId, rootId) |] (childId, rootId)
isOwnedBy :: NodeId -> UserId -> DBCmd err Bool
isOwnedBy nodeId userId = (== [Only True])
<$> runPGSQuery [sql| SELECT COUNT(*) = 1 from nodes AS c where c.id = ? AND c.user_id = ? |] (nodeId, userId)
isSharedWith :: NodeId -> NodeId -> DBCmd err Bool
isSharedWith targetNode targetUserNode = (== [Only True])
<$> runPGSQuery [sql|
BEGIN;
SET TRANSACTION READ ONLY;
COMMIT;
WITH RECURSIVE SharePath AS (
SELECT nn.node1_id, nn.node2_id AS shared_node_id
FROM nodes_nodes nn
WHERE nn.node1_id IN (SELECT id FROM nodes WHERE parent_id = ?)
UNION ALL
SELECT nn.node1_id, nn.node2_id
FROM nodes_nodes nn
JOIN SharePath sp ON nn.node1_id = sp.shared_node_id
)
SELECT
EXISTS (
SELECT 1
FROM nodes n
JOIN SharePath sp ON n.parent_id = sp.shared_node_id
WHERE n.id = ?
OR n.parent_id = ?
) AS share_exists;
|] (targetUserNode, targetNode, targetNode)
-- TODO should we check the category? -- TODO should we check the category?
isIn :: NodeId -> DocId -> DBCmd err Bool isIn :: NodeId -> DocId -> DBCmd err Bool
isIn cId docId = ( == [Only True]) isIn cId docId = ( == [Only True])
......
...@@ -111,8 +111,7 @@ tests = sequential $ aroundAll withTestDBAndPort $ do ...@@ -111,8 +111,7 @@ tests = sequential $ aroundAll withTestDBAndPort $ do
it "forbids 'alice' to see others node private info" $ \((_testEnv, port), app) -> do it "forbids 'alice' to see others node private info" $ \((_testEnv, port), app) -> do
withApplication app $ do withApplication app $ do
withValidLogin port "alice" (GargPassword "alice") $ \token -> do withValidLogin port "alice" (GargPassword "alice") $ \token -> do
let _unused = protected token "GET" (mkUrl port "/node/1") "" `shouldRespondWith` 403 protected token "GET" (mkUrl port "/node/1") "" `shouldRespondWith` 403
in liftIO $ pendingWith "POLICY CHECK DISABLED FOR NOW (ISSUE #279)"
describe "GET /api/v1.0/tree" $ do describe "GET /api/v1.0/tree" $ do
it "unauthorised users shouldn't see anything" $ \((_testEnv, port), app) -> do it "unauthorised users shouldn't see anything" $ \((_testEnv, port), app) -> do
...@@ -128,5 +127,4 @@ tests = sequential $ aroundAll withTestDBAndPort $ do ...@@ -128,5 +127,4 @@ tests = sequential $ aroundAll withTestDBAndPort $ do
it "forbids 'alice' to see others node private info" $ \((_testEnv, port), app) -> do it "forbids 'alice' to see others node private info" $ \((_testEnv, port), app) -> do
withApplication app $ do withApplication app $ do
withValidLogin port "alice" (GargPassword "alice") $ \token -> do withValidLogin port "alice" (GargPassword "alice") $ \token -> do
let _unused = protected token "GET" (mkUrl port "/tree/1") "" `shouldRespondWith` 403 protected token "GET" (mkUrl port "/tree/1") "" `shouldRespondWith` 403
in liftIO $ pendingWith "POLICY CHECK DISABLED FOR NOW (ISSUE #279)"
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment