From 3f0ff567a8223d54ce33120baa17b2506f2e6c98 Mon Sep 17 00:00:00 2001 From: Teemu Patja Date: Wed, 22 Mar 2017 22:20:52 +0200 Subject: [PATCH] Progressive Github permissioning & client-side oauth tokens * require only user:email oauth scope when signing up * if user wants to set bounties on repos, request additional oauth scopes * do not store github access tokens on server side and use client-side localStorage instead Fixes: #35 Fixes: #40 --- project.clj | 3 +- ...19-do-not-store-oauth-token-in-db.down.sql | 0 ...2119-do-not-store-oauth-token-in-db.up.sql | 1 + resources/sql/queries.sql | 11 +-- resources/templates/home.html | 6 +- src/clj/commiteth/db/users.clj | 9 +- src/clj/commiteth/github/core.clj | 27 ++++-- src/clj/commiteth/layout.clj | 4 +- src/clj/commiteth/routes/home.clj | 17 ++-- src/clj/commiteth/routes/redirect.clj | 14 +++- src/clj/commiteth/routes/services.clj | 23 +++--- src/cljs/commiteth/core.cljs | 22 +++-- src/cljs/commiteth/db.cljs | 4 +- src/cljs/commiteth/handlers.cljs | 82 ++++++++++++++----- src/cljs/commiteth/repos.cljs | 16 +++- src/cljs/commiteth/subscriptions.cljs | 5 ++ 16 files changed, 159 insertions(+), 85 deletions(-) create mode 100644 resources/migrations/20170322212119-do-not-store-oauth-token-in-db.down.sql create mode 100644 resources/migrations/20170322212119-do-not-store-oauth-token-in-db.up.sql diff --git a/project.clj b/project.clj index 5a200c6..f480101 100644 --- a/project.clj +++ b/project.clj @@ -46,7 +46,8 @@ [org.clojure/tools.nrepl "0.2.12"] [com.cemerick/piggieback "0.2.1"] [jarohen/chime "0.2.0"] - [com.andrewmcveigh/cljs-time "0.4.0"]] + [com.andrewmcveigh/cljs-time "0.4.0"] + [akiroz.re-frame/storage "0.1.1"]] :min-lein-version "2.0.0" :source-paths ["src/clj" "src/cljc"] diff --git a/resources/migrations/20170322212119-do-not-store-oauth-token-in-db.down.sql b/resources/migrations/20170322212119-do-not-store-oauth-token-in-db.down.sql new file mode 100644 index 0000000..e69de29 diff --git a/resources/migrations/20170322212119-do-not-store-oauth-token-in-db.up.sql b/resources/migrations/20170322212119-do-not-store-oauth-token-in-db.up.sql new file mode 100644 index 0000000..347529b --- /dev/null +++ b/resources/migrations/20170322212119-do-not-store-oauth-token-in-db.up.sql @@ -0,0 +1 @@ +ALTER TABLE "public"."users" DROP COLUMN "token"; diff --git a/resources/sql/queries.sql b/resources/sql/queries.sql index aee31af..968dd83 100644 --- a/resources/sql/queries.sql +++ b/resources/sql/queries.sql @@ -3,14 +3,13 @@ -- :name create-user! : {% style "https://cdnjs.cloudflare.com/ajax/libs/semantic-ui/2.1.8/semantic.min.css" %} diff --git a/src/clj/commiteth/db/users.clj b/src/clj/commiteth/db/users.clj index 6493a89..2e6f091 100644 --- a/src/clj/commiteth/db/users.clj +++ b/src/clj/commiteth/db/users.clj @@ -5,14 +5,13 @@ (:import [java.util Date])) (defn create-user - [user-id login name email avatar-url token] + [user-id login name email avatar-url] (jdbc/with-db-connection [con-db *db*] (db/create-user! con-db {:id user-id :login login :name name :email email - :token token :avatar_url avatar-url :address nil :created (new Date)}))) @@ -33,12 +32,6 @@ (jdbc/with-db-connection [con-db *db*] (db/update-user-address! con-db {:id user-id :address address}))) -(defn update-user-token - "Updates user token and returns updated user" - [user-id token] - (jdbc/with-db-connection [con-db *db*] - (db/update-user-token! con-db {:id user-id :token token}))) - (defn get-repo-owner "Gets repository owner by given repository id" [repo-id] diff --git a/src/clj/commiteth/github/core.clj b/src/clj/commiteth/github/core.clj index f750ec7..faacbe7 100644 --- a/src/clj/commiteth/github/core.clj +++ b/src/clj/commiteth/github/core.clj @@ -1,9 +1,11 @@ (ns commiteth.github.core - (:require [tentacles.repos :as repos] - [tentacles.users :as users] - [tentacles.repos :as repos] - [tentacles.issues :as issues] - [tentacles.core :as tentacles] + (:require [tentacles + [core :as tentacles] + [repos :as repos] + [oauth :as oauth] + [users :as users] + [repos :as repos] + [issues :as issues]] [ring.util.codec :as codec] [commiteth.config :refer [env]] [clj-http.client :as http] @@ -24,14 +26,23 @@ (defn self [] (:github-user env)) (defn self-password [] (:github-password env)) -(defn authorize-url [] +(defn authorize-url [scope] (let [params (codec/form-encode {:client_id (client-id) :redirect_uri (redirect-uri) - :scope "admin:repo_hook repo user:email admin:org_hook" + :scope scope :allow_signup true :state (str (UUID/randomUUID))})] (str "https://github.com/login/oauth/authorize" "?" params))) +(defn signup-authorize-url [] + (authorize-url "user:email")) + +(defn admin-authorize-url [] + (authorize-url "admin:repo_hook repo user:email admin:org_hook")) + +(defn access-settings-url [] + (str "https://github.com/settings/connections/applications/" (client-id))) + (defn post-for-token [code state] (http/post "https://github.com/login/oauth/access_token" @@ -75,10 +86,10 @@ (map #(merge {:owner-login (get-in % [:owner :login])} {:owner-type (get-in % [:owner :type])} + {:owner-avatar-url (get-in % [:owner :avatar_url])} (select-keys % repo-fields)) (repos/repos (merge (auth-params token) {:type "all" :all-pages true}))) - (filter #(not (:fork %))) (filter #(-> % :permissions :admin)))] (group-by :owner-login all-repos-with-admin-access))) diff --git a/src/clj/commiteth/layout.clj b/src/clj/commiteth/layout.clj index e60f384..00cb0fd 100644 --- a/src/clj/commiteth/layout.clj +++ b/src/clj/commiteth/layout.clj @@ -4,8 +4,7 @@ [markdown.core :refer [md-to-html-string]] [ring.util.http-response :refer [content-type ok]] [ring.util.anti-forgery :refer [anti-forgery-field]] - [ring.middleware.anti-forgery :refer [*anti-forgery-token*]] - [commiteth.github.core :as github])) + [ring.middleware.anti-forgery :refer [*anti-forgery-token*]])) (parser/set-resource-path! (clojure.java.io/resource "templates")) (parser/add-tag! :csrf-field (fn [_ _] (anti-forgery-field))) @@ -19,7 +18,6 @@ (parser/render-file template (assoc params - :authorize-url (github/authorize-url) :page template :csrf-token *anti-forgery-token*))) "text/html; charset=utf-8")) diff --git a/src/clj/commiteth/routes/home.clj b/src/clj/commiteth/routes/home.clj index 73bd117..eb0cef1 100644 --- a/src/clj/commiteth/routes/home.clj +++ b/src/clj/commiteth/routes/home.clj @@ -1,5 +1,6 @@ (ns commiteth.routes.home (:require [commiteth.layout :as layout] + [commiteth.github.core :as github] [compojure.core :refer [defroutes GET]] [ring.util.response :refer [redirect]] [ring.util.http-response :refer [ok header]] @@ -8,15 +9,21 @@ (defonce ^:const version (System/getProperty "commiteth.version")) -(defn home-page [{user-id :id login :login token :token}] - (layout/render "home.html" {:userId user-id +(defn home-page [{user-id :id + login :login + token :token + admin-token :admin-token}] + (layout/render "home.html" {:user-id user-id :login login :token token - :commitethVersion version})) + :admin-token admin-token + :authorize-url (github/signup-authorize-url) + :authorize-url-admin (github/admin-authorize-url) + :commiteth-version version})) (defroutes home-routes - (GET "/" {{identity :identity} :session} - (home-page identity)) + (GET "/" {{user :identity} :session} + (home-page user)) (GET "/logout" {session :session} (-> (redirect "/") (assoc :session (dissoc session :identity))))) diff --git a/src/clj/commiteth/routes/redirect.clj b/src/clj/commiteth/routes/redirect.clj index 3987545..7c84800 100644 --- a/src/clj/commiteth/routes/redirect.clj +++ b/src/clj/commiteth/routes/redirect.clj @@ -9,7 +9,8 @@ [ring.util.response :as response] [commiteth.layout :refer [render]] [cheshire.core :refer [generate-string]] - [clojure.tools.logging :as log])) + [clojure.tools.logging :as log] + [clojure.string :as str])) @@ -19,7 +20,7 @@ user-id :id avatar-url :avatar_url} user email (github/get-user-email token)] - (users/create-user user-id login name email avatar-url token))) + (users/create-user user-id login name email avatar-url))) (defn- get-or-create-user [token] @@ -28,17 +29,22 @@ user-id :id} user] (log/debug "get-or-create-user" user) (or - (users/update-user-token user-id token) + (users/get-user user-id) (create-user token user)))) (defroutes redirect-routes (GET "/callback" [code state] (let [resp (github/post-for-token code state) body (keywordize-keys (codec/form-decode (:body resp))) + scope (:scope body) access-token (:access_token body)] + (log/debug "github sign-in callback, response body:" body) (if (:error body) ;; Why does Mist browser sends two redirects at the same time? The latter results in 401 error. (response/redirect "/") - (let [user (get-or-create-user access-token)] + (let [admin-token? (str/includes? scope "repo") + token-key (if admin-token? :admin-token :token) + user (-> (get-or-create-user access-token) + (assoc token-key access-token))] (-> (response/redirect "/") (assoc :session {:identity user}))))))) diff --git a/src/clj/commiteth/routes/services.clj b/src/clj/commiteth/routes/services.clj index 49dad7c..ffa3019 100644 --- a/src/clj/commiteth/routes/services.clj +++ b/src/clj/commiteth/routes/services.clj @@ -61,12 +61,12 @@ (defn handle-toggle-repo [user params] (log/debug "handle-toggle-repo" user params) - (let [{token :token - user-id :id} user - {repo-id :id - full-repo :full_name - owner-avatar-url :owner-avatar-url - repo :name} params + (let [{user-id :id} user + {repo-id :id + full-repo :full_name + owner-avatar-url :owner-avatar-url + token :token + repo :name} params [owner _] (str/split full-repo #"/") db-user (users/get-user (:id user))] (if (empty? (:address db-user)) @@ -90,9 +90,9 @@ (defn in? [coll elem] (some #(= elem %) coll)) -(defn handle-get-user-repos [user] +(defn handle-get-user-repos [user token] (log/debug "handle-get-user-repos") - (let [github-repos (github/get-user-repos (:token user)) + (let [github-repos (github/get-user-repos token) enabled-repos (vec (repositories/get-enabled (:id user))) repo-enabled? (fn [repo] (in? enabled-repos (:id repo))) update-enabled (fn [repo] (assoc repo :enabled (repo-enabled? repo)))] @@ -164,7 +164,6 @@ :current-user user (ok {:user (dissoc (users/get-user (:id user)) - :token :email)})) (POST "/address" [] :auth-rules authenticated? @@ -179,10 +178,12 @@ (if (= 1 result) (ok) (internal-server-error))))) - (GET "/repositories" [] + (GET "/repositories" {:keys [params]} :auth-rules authenticated? :current-user user - (ok {:repositories (handle-get-user-repos user)})) + (ok {:repositories (handle-get-user-repos + user + (:token params))})) (GET "/enabled-repositories" [] :auth-rules authenticated? :current-user user diff --git a/src/cljs/commiteth/core.cljs b/src/cljs/commiteth/core.cljs index 3fda779..e669215 100644 --- a/src/cljs/commiteth/core.cljs +++ b/src/cljs/commiteth/core.cljs @@ -173,18 +173,22 @@ (defn load-user [] (if-let [login js/user] - (when-not (= login @active-user) - (println "active user changed, loading user data") - (reset! active-user login) - (rf/dispatch [:set-active-user - {:login login - :id (js/parseInt js/userId) - :token js/token}])) + (if (= login @active-user) + (do + (println "refresh with active user, updating tokens") + (rf/dispatch [:update-tokens js/token js/adminToken])) + (do + (println "active user changed, loading user data") + (reset! active-user login) + (rf/dispatch [:set-active-user + {:login login + :id (js/parseInt js/userId)} + js/token + js/adminToken]))) (reset! active-user nil))) (defn load-data [] - (doall - (map rf/dispatch + (doall (map rf/dispatch [[:load-open-bounties] [:load-activity-feed] [:load-top-hunters]])) diff --git a/src/cljs/commiteth/db.cljs b/src/cljs/commiteth/db.cljs index 356f826..303d60e 100644 --- a/src/cljs/commiteth/db.cljs +++ b/src/cljs/commiteth/db.cljs @@ -8,4 +8,6 @@ :open-bounties [] :owner-bounties {} :top-hunters [] - :activity-feed []}) + :activity-feed [] + :gh-token nil + :gh-admin-token nil}) diff --git a/src/cljs/commiteth/handlers.cljs b/src/cljs/commiteth/handlers.cljs index dc083ac..89d4141 100644 --- a/src/cljs/commiteth/handlers.cljs +++ b/src/cljs/commiteth/handlers.cljs @@ -1,8 +1,19 @@ (ns commiteth.handlers (:require [commiteth.db :as db] - [re-frame.core :refer [dispatch reg-event-db reg-event-fx reg-fx]] + [re-frame.core :refer [dispatch + reg-event-db + reg-event-fx + reg-fx + inject-cofx]] [ajax.core :refer [GET POST]] - [cuerdas.core :as str])) + [cuerdas.core :as str] + [akiroz.re-frame.storage + :as rf-storage + :refer [reg-co-fx!]])) + + +(rf-storage/reg-co-fx! :commiteth {:fx :store + :cofx :store}) (reg-fx :http @@ -23,13 +34,14 @@ :redirect (fn [{:keys [path]}] (println "redirecting to" path) - (set! (.-pathname js/location) path)) + (set! (.-pathname js/location) path))) - (reg-event-db - :initialize-db - (fn [_ _] - db/default-db))) +(reg-event-fx + :initialize-db + [(inject-cofx :store)] + (fn [{:keys [db store]} [_]] + {:db (merge db/default-db store)})) (reg-event-db :assoc-in @@ -55,11 +67,42 @@ (fn [db _] (dissoc db :flash-message))) + +(defn update-if-not-empty [m k val] + (conj m (when (not (empty? val)) [k val]))) + +(defn update-local-storage-tokens [ls token admin-token] + (into {} + (-> ls + (update-if-not-empty :gh-token token) + (update-if-not-empty :gh-admin-token admin-token)))) + (reg-event-fx :set-active-user - (fn [{:keys [db]} [_ user]] + (fn [{:keys [db store]} [_ user token admin-token]] {:db (assoc db :user user) - :dispatch [:load-user-profile]})) + :dispatch-n [[:update-tokens token admin-token] + [:load-user-profile]]})) + +(reg-event-fx + :update-tokens + [(inject-cofx :store)] + (fn [{:keys [db store]} [_ token admin-token]] + (let [ls-data (update-local-storage-tokens store token admin-token)] + (println "update-tokens, ls-data:" ls-data) + {:db (merge db ls-data) + :store ls-data + :dispatch [:load-user-repos]}))) + +;; copied from plumbing.core to avoid cljsbuild warnings +(defn dissoc-in + [m [k & ks]] + (when m + (if-let [res (and ks (dissoc-in (get m k) ks))] + (assoc m k res) + (let [res (dissoc m k)] + (when-not (empty? res) + res))))) (reg-event-fx :sign-out @@ -156,6 +199,7 @@ {:db (assoc db :repos-loading? true) :http {:method GET :url "/api/user/repositories" + :params {:token (:gh-admin-token db)} :on-success #(dispatch [:set-user-repos (:repositories %)]) :on-error #(dispatch [:set-flash-message :error "Failed to load repositories"]) @@ -175,7 +219,8 @@ (reg-event-fx :toggle-repo - (fn [{:keys [db]} [_ repo]] + [(inject-cofx :store)] + (fn [{:keys [db store]} [_ repo]] {:db (assoc db :repos (update-repo-state (:repos db) (:full_name repo) @@ -186,7 +231,12 @@ :on-success #(dispatch [:repo-toggle-success %]) :on-error #(dispatch [:repo-toggle-error repo %]) :finally #(println "finally" %) - :params (select-keys repo [:id :owner :owner-avatar-url :full_name :name])}})) + :params (merge {:token (:gh-admin-token store)} + (select-keys repo [:id + :owner + :owner-avatar-url + :full_name + :name]))}})) (reg-event-db @@ -288,16 +338,6 @@ :error (str "Failed to send transaction" e)]]}))))) -;; copied from plumbing.core to avoid cljsbuild warnings -(defn dissoc-in - [m [k & ks]] - (when m - (if-let [res (and ks (dissoc-in (get m k) ks))] - (assoc m k res) - (let [res (dissoc m k)] - (when-not (empty? res) - res))))) - (reg-event-fx :payout-confirmed (fn [{:keys [db]} [_ issue-id]] diff --git a/src/cljs/commiteth/repos.cljs b/src/cljs/commiteth/repos.cljs index 4085fb3..0051c95 100644 --- a/src/cljs/commiteth/repos.cljs +++ b/src/cljs/commiteth/repos.cljs @@ -54,8 +54,8 @@ (into [:div.ui.cards] (map repo-card group-repos))])))))) - -(defn repos-page [] +(defn repos-page-token-ok [] + (println "repos-token-ok") (let [repos-loading? (rf/subscribe [:repos-loading?])] (fn [] (if @repos-loading? @@ -63,3 +63,15 @@ [:div.ui.active.inverted.dimmer [:div.ui.text.loader "Loading"]]] [repos-list])))) + +(defn repos-page [] + (let [gh-admin-token (rf/subscribe [:gh-admin-token])] + (fn [] + (println "gh-admin-token" @gh-admin-token) + (if (empty? @gh-admin-token) + [:div.ui.container + [:div.ui.warning.message + [:i.warning.icon] + "To set bounties for your repositories or for organizations' repositories where you have access, you need to grant Commit ETH the required permissions"] + [:a.ui.button.small {:href js/authorizeUrlAdmin} "Enable"]] + [repos-page-token-ok])))) diff --git a/src/cljs/commiteth/subscriptions.cljs b/src/cljs/commiteth/subscriptions.cljs index 08e4b67..ef36de1 100644 --- a/src/cljs/commiteth/subscriptions.cljs +++ b/src/cljs/commiteth/subscriptions.cljs @@ -51,6 +51,11 @@ (fn [db _] (:activity-feed db))) +(reg-sub + :gh-admin-token + (fn [db _] + (:gh-admin-token db))) + (reg-sub :get-in (fn [db [_ path]]