Merge pull request #179 from metabase/fix_org_permissions

fix org permissions check for Org that doesn't exist.

src/metabase/api/common.clj

 (ns metabase.api.common
   "Dynamic variables and utility functions/macros for writing API functions."
   (:require [compojure.core :refer [defroutes]]
+            [korma.core :refer :all :exclude [update]]
             [medley.core :refer :all]
             [metabase.api.common.internal :refer :all]
-            [metabase.db :refer :all])
+            [metabase.db :refer :all]
+            [metabase.db.internal :refer [entity->korma]])
   (:import com.metabase.corvus.api.ApiException))
 
+(declare check-404)
+
 ;;; ## DYNAMIC VARIABLES
 ;; These get bound by middleware for each HTTP request.
 
@@ -33,10 +37,21 @@
   "TODO - A very similar implementation exists in `metabase.models`. Find some way to combine them."
   [org-id]
   (when *current-user-id*
-    (when-let [{superuser? :is_superuser} (sel :one ['metabase.models.user/User :is_superuser] :id *current-user-id*)]
-      (if superuser? :admin
-          (when-let [{admin? :admin} (sel :one ['metabase.models.org-perm/OrgPerm :admin] :user_id *current-user-id* :organization_id org-id)]
-            (if admin? :admin :default))))))
+    (let [[{org-id :id
+            [{admin? :admin}] :org-perms}] (select (-> (entity->korma 'metabase.models.org/Org)                ; this is a complicated join but Org permissions checking
+                                                       (entity-fields [:id]))                                  ; is a very common case so optimization is worth it here
+                                                   (where {:id org-id})
+                                                   (with (-> (entity->korma 'metabase.models.org-perm/OrgPerm)
+                                                             (entity-fields [:admin]))
+                                                         (where {:organization_id org-id
+                                                                 :user_id *current-user-id*})))
+            superuser? (sel :one :field ['metabase.models.user/User :is_superuser] :id *current-user-id*)]
+      (check-404 org-id)
+      (cond
+        superuser?      :admin
+        admin?          :admin
+        (false? admin?) :default ; perm still exists but admin = false
+        :else           nil))))
 
 (defmacro org-perms-case
   "Evaluates BODY inside a case statement based on `*current-user*`'s perms for Org with ORG-ID.

src/metabase/db.clj

@@ -276,7 +276,7 @@
 
     (exists? User :id 100)"
   [entity & {:as kwargs}]
-  `(not (empty? (select ~entity
+  `(not (empty? (select (entity->korma ~entity)
                         (fields [:id])
                         (where ~kwargs)
                         (limit 1)))))

src/metabase/models/org.clj

 (ns metabase.models.org
   (:require [korma.core :refer :all]
             [metabase.api.common :refer :all]
-            [metabase.db :refer :all]))
+            [metabase.db :refer :all]
+            [metabase.models.org-perm :refer [OrgPerm]]))
 
 (defentity Org
-  (table :core_organization))
+  (table :core_organization)
+  (has-many OrgPerm {:fk :organization_id})
+  (transform #(clojure.set/rename-keys % {:core_userorgperm :org-perms})))
 
 (defn org-can-read
   "Does `*current-user*` have read permissions for `Org` with ORG-ID?"
@@ -32,4 +35,4 @@
     (merge defaults org)))
 
 (defmethod pre-cascade-delete Org [_ {:keys [id]}]
-  (cascade-delete 'metabase.models.org-perm/OrgPerm :organization_id id))
+  (cascade-delete OrgPerm :organization_id id))

src/metabase/models/org_perm.clj

 (ns metabase.models.org-perm
   (:require [korma.core :refer :all]
-            [metabase.db :refer :all]
-            [metabase.models.org :refer [Org]]))
+            [metabase.db :refer :all]))
 
 
 (defentity OrgPerm
@@ -10,7 +9,7 @@
 
 (defmethod post-select OrgPerm [_ {:keys [organization_id user_id] :as org-perm}]
   (assoc org-perm
-         :organization (delay (sel :one Org :id organization_id))
+         :organization (delay (sel :one 'metabase.models.org/Org :id organization_id))
          :user         (delay (sel :one 'metabase.models.user/User :id user_id))))
 
 

test/metabase/api/common_test.clj

@@ -2,7 +2,43 @@
   (:require [expectations :refer :all]
             [metabase.api.common :refer :all]
             [metabase.api.common.internal :refer :all]
-            [metabase.util :refer [regex= regex?]]))
+            [metabase.api.org-test :refer [create-org]]
+            [metabase.test-data :refer :all]
+            [metabase.test-data.create :refer [create-user]]
+            metabase.test-utils
+            [metabase.test.util :refer :all]
+            [metabase.util :refer [regex= regex?]])
+  (:import com.metabase.corvus.api.ApiException))
+
+;;; TESTS FOR CURRENT-USER-PERMS-FOR-ORG
+;; admins should get :admin
+(expect :admin
+    (with-current-user (user->id :rasta)
+      (current-user-perms-for-org @org-id)))
+
+;; superusers should always get :admin whether they have org perms or not
+(expect-let [{org-id :id} (create-org (random-name))]
+  :admin
+  (with-current-user (user->id :crowberto)
+      (current-user-perms-for-org org-id)))
+
+(expect :admin
+    (with-current-user (user->id :crowberto)
+      (current-user-perms-for-org @org-id)))
+
+;; other users should get :default or nil depending on if they have an Org Perm
+(expect :default
+    (with-current-user (user->id :lucky)
+      (current-user-perms-for-org @org-id)))
+
+(expect nil
+    (with-current-user (:id (create-user))
+      (current-user-perms-for-org @org-id)))
+
+;; Should get a 404 for an Org that doesn't exist
+(expect ApiException
+  (with-current-user (user->id :crowberto)
+    (current-user-perms-for-org 1000)))
 
 
 ;;; TESTS FOR CHECK (ETC)

test/metabase/test_data.clj

@@ -127,6 +127,8 @@
       user->token (fn [user]
                     (or (@tokens user)
                         (let [token (http/authenticate (user->credentials user))]
+                          (when-not token
+                            (throw (Exception. (format "Authentication failed for %s with credentials %s" user (user->credentials user)))))
                           (swap! tokens assoc user token)
                           token)))]
   (defn user->client