From e19ff1d43beb9f6561a57d4b9c4d6c8298576aae Mon Sep 17 00:00:00 2001 From: iandyh Date: Thu, 21 Sep 2023 10:37:48 +0900 Subject: [PATCH 01/12] trigger is limited to project owners only --- shibuya/api/main.go | 2 +- shibuya/api/utils.go | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/shibuya/api/main.go b/shibuya/api/main.go index df4d7730..6715327d 100644 --- a/shibuya/api/main.go +++ b/shibuya/api/main.go @@ -657,7 +657,7 @@ func (s *ShibuyaAPI) collectionDeploymentHandler(w http.ResponseWriter, r *http. } func (s *ShibuyaAPI) collectionTriggerHandler(w http.ResponseWriter, r *http.Request, params httprouter.Params) { - collection, err := getCollection(params.ByName("collection_id")) + collection, err := checkCollectionOwnership(r, params) if err != nil { s.handleErrors(w, err) return diff --git a/shibuya/api/utils.go b/shibuya/api/utils.go index bfe56440..236d00bc 100644 --- a/shibuya/api/utils.go +++ b/shibuya/api/utils.go @@ -3,6 +3,9 @@ package api import ( "net/http" "strings" + + "github.com/julienschmidt/httprouter" + "github.com/rakutentech/shibuya/shibuya/model" ) func retrieveClientIP(r *http.Request) string { @@ -12,3 +15,22 @@ func retrieveClientIP(r *http.Request) string { } return strings.Split(t, ",")[0] } + +func checkCollectionOwnership(r *http.Request, params httprouter.Params) (*model.Collection, error) { + account := model.GetAccountBySession(r) + if account == nil { + return nil, makeLoginError() + } + collection, err := getCollection(params.ByName("collection_id")) + if err != nil { + return nil, err + } + project, err := model.GetProject(collection.ProjectID) + if err != nil { + return nil, err + } + if _, ok := account.MLMap[project.Owner]; !ok { + return nil, makeNoPermissionErr("") + } + return collection, nil +} From da9f27f1f4f8ff76de3bd5d4c2d24ae25e22e215 Mon Sep 17 00:00:00 2001 From: iandyh Date: Fri, 22 Sep 2023 11:06:38 +0900 Subject: [PATCH 02/12] update to new api calls --- shibuya/api/main.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/shibuya/api/main.go b/shibuya/api/main.go index 6715327d..9c397256 100644 --- a/shibuya/api/main.go +++ b/shibuya/api/main.go @@ -5,7 +5,7 @@ import ( "encoding/json" "errors" "fmt" - "io/ioutil" + "io" "net/http" "strconv" "time" @@ -538,7 +538,7 @@ func (s *ShibuyaAPI) collectionUploadHandler(w http.ResponseWriter, r *http.Requ s.handleErrors(w, makeInvalidResourceError("file")) return } - raw, err := ioutil.ReadAll(file) + raw, err := io.ReadAll(file) if err != nil { s.handleErrors(w, makeInvalidRequestError("invalid file")) return From 29025a68a47617d78bb855703e3cd7de69f89dcc Mon Sep 17 00:00:00 2001 From: iandyh Date: Fri, 22 Sep 2023 11:16:11 +0900 Subject: [PATCH 03/12] add check to viewing the collection. Also admins should always have the permission --- shibuya/api/main.go | 2 +- shibuya/api/utils.go | 17 ++++++++++++++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/shibuya/api/main.go b/shibuya/api/main.go index 9c397256..18c3cd5c 100644 --- a/shibuya/api/main.go +++ b/shibuya/api/main.go @@ -492,7 +492,7 @@ func (s *ShibuyaAPI) collectionDeleteHandler(w http.ResponseWriter, r *http.Requ } func (s *ShibuyaAPI) collectionGetHandler(w http.ResponseWriter, r *http.Request, params httprouter.Params) { - collection, err := getCollection(params.ByName("collection_id")) + collection, err := checkCollectionOwnership(r, params) if err != nil { s.handleErrors(w, err) return diff --git a/shibuya/api/utils.go b/shibuya/api/utils.go index 236d00bc..2425eeff 100644 --- a/shibuya/api/utils.go +++ b/shibuya/api/utils.go @@ -5,6 +5,7 @@ import ( "strings" "github.com/julienschmidt/httprouter" + "github.com/rakutentech/shibuya/shibuya/config" "github.com/rakutentech/shibuya/shibuya/model" ) @@ -16,6 +17,17 @@ func retrieveClientIP(r *http.Request) string { return strings.Split(t, ",")[0] } +func isAdmin(account *model.Account) bool { + for _, ml := range account.ML { + for _, admin := range config.SC.AuthConfig.AdminUsers { + if ml == admin { + return true + } + } + } + return false +} + func checkCollectionOwnership(r *http.Request, params httprouter.Params) (*model.Collection, error) { account := model.GetAccountBySession(r) if account == nil { @@ -30,7 +42,10 @@ func checkCollectionOwnership(r *http.Request, params httprouter.Params) (*model return nil, err } if _, ok := account.MLMap[project.Owner]; !ok { - return nil, makeNoPermissionErr("") + if !isAdmin(account) { + return nil, makeNoPermissionErr("") + } + } return collection, nil } From a1ababd81ad27118cf0ccfed378e0b870f87c36b Mon Sep 17 00:00:00 2001 From: iandyh Date: Fri, 22 Sep 2023 11:26:44 +0900 Subject: [PATCH 04/12] improve the isadmin logic --- shibuya/api/utils.go | 15 +-------------- shibuya/model/user.go | 11 +++++++++++ shibuya/ui/handler.go | 11 +---------- 3 files changed, 13 insertions(+), 24 deletions(-) diff --git a/shibuya/api/utils.go b/shibuya/api/utils.go index 2425eeff..c5040d81 100644 --- a/shibuya/api/utils.go +++ b/shibuya/api/utils.go @@ -5,7 +5,6 @@ import ( "strings" "github.com/julienschmidt/httprouter" - "github.com/rakutentech/shibuya/shibuya/config" "github.com/rakutentech/shibuya/shibuya/model" ) @@ -17,17 +16,6 @@ func retrieveClientIP(r *http.Request) string { return strings.Split(t, ",")[0] } -func isAdmin(account *model.Account) bool { - for _, ml := range account.ML { - for _, admin := range config.SC.AuthConfig.AdminUsers { - if ml == admin { - return true - } - } - } - return false -} - func checkCollectionOwnership(r *http.Request, params httprouter.Params) (*model.Collection, error) { account := model.GetAccountBySession(r) if account == nil { @@ -42,10 +30,9 @@ func checkCollectionOwnership(r *http.Request, params httprouter.Params) (*model return nil, err } if _, ok := account.MLMap[project.Owner]; !ok { - if !isAdmin(account) { + if !account.IsAdmin() { return nil, makeNoPermissionErr("") } - } return collection, nil } diff --git a/shibuya/model/user.go b/shibuya/model/user.go index f8c88049..e50f2ba9 100644 --- a/shibuya/model/user.go +++ b/shibuya/model/user.go @@ -39,3 +39,14 @@ func GetAccountBySession(r *http.Request) *Account { } return a } + +func (a *Account) IsAdmin() bool { + for _, ml := range a.ML { + for _, admin := range config.SC.AuthConfig.AdminUsers { + if ml == admin { + return true + } + } + } + return false +} diff --git a/shibuya/ui/handler.go b/shibuya/ui/handler.go index 082fb219..d8692c44 100644 --- a/shibuya/ui/handler.go +++ b/shibuya/ui/handler.go @@ -45,16 +45,7 @@ func (u *UI) homeHandler(w http.ResponseWriter, r *http.Request, params httprout http.Redirect(w, r, "/login", http.StatusSeeOther) return } - IsAdmin := false -outer: - for _, ml := range account.ML { - for _, admin := range config.SC.AuthConfig.AdminUsers { - if ml == admin { - IsAdmin = true - break outer - } - } - } + IsAdmin := account.IsAdmin() enableSid := config.SC.EnableSid resultDashboardURL := config.SC.DashboardConfig.Url + config.SC.DashboardConfig.RunDashboard engineHealthDashboardURL := config.SC.DashboardConfig.Url + config.SC.DashboardConfig.EnginesDashboard From 12bae5c219b7e59148fa32619b72027d99eaa6b4 Mon Sep 17 00:00:00 2001 From: iandyh Date: Tue, 10 Oct 2023 15:01:06 +0900 Subject: [PATCH 05/12] alert the user if they do not have permission --- shibuya/ui/static/js/collection.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/shibuya/ui/static/js/collection.js b/shibuya/ui/static/js/collection.js index 05b2640e..4278756a 100644 --- a/shibuya/ui/static/js/collection.js +++ b/shibuya/ui/static/js/collection.js @@ -161,7 +161,7 @@ var Collection = Vue.component("collection", { this.collection = resp.body; }, function (resp) { - console.log(resp.body); + alert(resp.body.message); } ); this.$http.get("collections/" + this.collection_id + '/status').then( @@ -170,7 +170,7 @@ var Collection = Vue.component("collection", { this.updateCache(this.collection_status.status); }, function (resp) { - console.log(resp.body); + alert(resp.body.message); } ); }, @@ -394,4 +394,4 @@ var Collection = Vue.component("collection", { ); } } -}); \ No newline at end of file +}); From 14d52dcb9532705674f11d34256bb0a1f4c7b5b6 Mon Sep 17 00:00:00 2001 From: iandyh Date: Tue, 10 Oct 2023 15:01:18 +0900 Subject: [PATCH 06/12] more helpful error message --- shibuya/api/utils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shibuya/api/utils.go b/shibuya/api/utils.go index c5040d81..d8b463c2 100644 --- a/shibuya/api/utils.go +++ b/shibuya/api/utils.go @@ -31,7 +31,7 @@ func checkCollectionOwnership(r *http.Request, params httprouter.Params) (*model } if _, ok := account.MLMap[project.Owner]; !ok { if !account.IsAdmin() { - return nil, makeNoPermissionErr("") + return nil, makeNoPermissionErr("You are not the owner of the collection") } } return collection, nil From 2944f8a2d86b1754d147c71fc334e3439e8a44d3 Mon Sep 17 00:00:00 2001 From: iandyh Date: Tue, 10 Oct 2023 15:01:42 +0900 Subject: [PATCH 07/12] checking ownership for all the operations on the collection --- shibuya/api/main.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/shibuya/api/main.go b/shibuya/api/main.go index 18c3cd5c..833b6da8 100644 --- a/shibuya/api/main.go +++ b/shibuya/api/main.go @@ -640,7 +640,7 @@ func (s *ShibuyaAPI) collectionEnginesDetailHandler(w http.ResponseWriter, r *ht } func (s *ShibuyaAPI) collectionDeploymentHandler(w http.ResponseWriter, r *http.Request, params httprouter.Params) { - collection, err := getCollection(params.ByName("collection_id")) + collection, err := checkCollectionOwnership(r, params) if err != nil { s.handleErrors(w, err) return @@ -669,7 +669,7 @@ func (s *ShibuyaAPI) collectionTriggerHandler(w http.ResponseWriter, r *http.Req } func (s *ShibuyaAPI) collectionTermHandler(w http.ResponseWriter, r *http.Request, params httprouter.Params) { - collection, err := getCollection(params.ByName("collection_id")) + collection, err := checkCollectionOwnership(r, params) if err != nil { s.handleErrors(w, err) return @@ -681,7 +681,7 @@ func (s *ShibuyaAPI) collectionTermHandler(w http.ResponseWriter, r *http.Reques } func (s *ShibuyaAPI) collectionStatusHandler(w http.ResponseWriter, r *http.Request, params httprouter.Params) { - collection, err := getCollection(params.ByName("collection_id")) + collection, err := checkCollectionOwnership(r, params) if err != nil { s.handleErrors(w, err) return @@ -694,7 +694,7 @@ func (s *ShibuyaAPI) collectionStatusHandler(w http.ResponseWriter, r *http.Requ } func (s *ShibuyaAPI) collectionPurgeHandler(w http.ResponseWriter, r *http.Request, params httprouter.Params) { - collection, err := getCollection(params.ByName("collection_id")) + collection, err := checkCollectionOwnership(r, params) if err != nil { s.handleErrors(w, err) return From 567508d611755a10379a79606df0e9089484a44f Mon Sep 17 00:00:00 2001 From: iandyh Date: Fri, 1 Dec 2023 12:29:39 +0900 Subject: [PATCH 08/12] check ownership --- shibuya/api/collection.go | 2 +- shibuya/api/main.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/shibuya/api/collection.go b/shibuya/api/collection.go index 8e2fd749..e12ffe7a 100644 --- a/shibuya/api/collection.go +++ b/shibuya/api/collection.go @@ -25,7 +25,7 @@ func getCollection(collectionID string) (*model.Collection, error) { } func (s *ShibuyaAPI) collectionConfigGetHandler(w http.ResponseWriter, req *http.Request, params httprouter.Params) { - collection, err := getCollection(params.ByName("collection_id")) + collection, err := checkCollectionOwnership(req, params) if err != nil { s.handleErrors(w, err) return diff --git a/shibuya/api/main.go b/shibuya/api/main.go index 833b6da8..be71bb14 100644 --- a/shibuya/api/main.go +++ b/shibuya/api/main.go @@ -626,7 +626,7 @@ func (s *ShibuyaAPI) collectionUploadHandler(w http.ResponseWriter, r *http.Requ } func (s *ShibuyaAPI) collectionEnginesDetailHandler(w http.ResponseWriter, r *http.Request, params httprouter.Params) { - collection, err := getCollection(params.ByName("collection_id")) + collection, err := checkCollectionOwnership(r, params) if err != nil { s.handleErrors(w, err) return From 39a9613349231eea480907ee5ea0581b1400a039 Mon Sep 17 00:00:00 2001 From: iandyh Date: Fri, 1 Dec 2023 14:14:51 +0900 Subject: [PATCH 09/12] unify how we check the ownership --- shibuya/api/main.go | 28 +++++++++------------------- 1 file changed, 9 insertions(+), 19 deletions(-) diff --git a/shibuya/api/main.go b/shibuya/api/main.go index be71bb14..52b9b8c2 100644 --- a/shibuya/api/main.go +++ b/shibuya/api/main.go @@ -453,20 +453,11 @@ func (s *ShibuyaAPI) collectionDeleteHandler(w http.ResponseWriter, r *http.Requ s.handleErrors(w, makeLoginError()) return } - collection, err := getCollection(params.ByName("collection_id")) - if err != nil { - s.handleErrors(w, err) - return - } - project, err := model.GetProject(collection.ProjectID) + collection, err := checkCollectionOwnership(r, params) if err != nil { s.handleErrors(w, err) return } - if _, ok := account.MLMap[project.Owner]; !ok { - s.handleErrors(w, makeNoPermissionErr("You don't have permission")) - return - } if config.SC.ExecutorConfig.Cluster.OnDemand { operator := controller.NewGCPOperator(collection.ID, 0) pool := operator.GetNodePool() @@ -531,6 +522,11 @@ func (s *ShibuyaAPI) collectionUpdateHandler(w http.ResponseWriter, _ *http.Requ } func (s *ShibuyaAPI) collectionUploadHandler(w http.ResponseWriter, r *http.Request, params httprouter.Params) { + collection, err := checkCollectionOwnership(r, params) + if err != nil { + s.handleErrors(w, err) + return + } e := new(model.ExecutionWrapper) r.ParseMultipartForm(1 << 20) //parse 1 MB of data file, _, err := r.FormFile("collectionYAML") @@ -552,22 +548,16 @@ func (s *ShibuyaAPI) collectionUploadHandler(w http.ResponseWriter, r *http.Requ s.handleErrors(w, makeInvalidResourceError("YAML file")) return } - collectionID, _ := strconv.ParseInt(params.ByName("collection_id"), 10, 64) - if (e.Content.CollectionID != 0) && (e.Content.CollectionID != collectionID) { + if e.Content.CollectionID != collection.ID { s.handleErrors(w, makeInvalidRequestError("collection ID mismatch")) return } - collection, err := model.GetCollection(collectionID) + project, err := model.GetProject(collection.ProjectID) if err != nil { + log.Error(err) s.handleErrors(w, err) return } - project, _ := model.GetProject(collection.ProjectID) - account := model.GetAccountBySession(r) - if _, ok := account.MLMap[project.Owner]; !ok { - s.handleErrors(w, makeNoPermissionErr("You don't own the project")) - return - } totalEnginesRequired := 0 for _, ep := range e.Content.Tests { plan, err := model.GetPlan(ep.PlanID) From f877f19cb74e7edce1affc150d349cb6954bfba5 Mon Sep 17 00:00:00 2001 From: iandyh Date: Fri, 1 Dec 2023 14:16:46 +0900 Subject: [PATCH 10/12] add ownership check --- shibuya/api/main.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/shibuya/api/main.go b/shibuya/api/main.go index 52b9b8c2..49b93dbe 100644 --- a/shibuya/api/main.go +++ b/shibuya/api/main.go @@ -353,7 +353,7 @@ func (s *ShibuyaAPI) collectionFilesGetHandler(w http.ResponseWriter, _ *http.Re } func (s *ShibuyaAPI) collectionFilesUploadHandler(w http.ResponseWriter, r *http.Request, params httprouter.Params) { - collection, err := getCollection(params.ByName("collection_id")) + collection, err := checkCollectionOwnership(r, params) if err != nil { s.handleErrors(w, err) return @@ -373,7 +373,7 @@ func (s *ShibuyaAPI) collectionFilesUploadHandler(w http.ResponseWriter, r *http } func (s *ShibuyaAPI) collectionFilesDeleteHandler(w http.ResponseWriter, r *http.Request, params httprouter.Params) { - collection, err := getCollection(params.ByName("collection_id")) + collection, err := checkCollectionOwnership(r, params) if err != nil { s.handleErrors(w, err) return From 7bdb3cfc0f228414842ffc3e3bf441ed7f53ccdd Mon Sep 17 00:00:00 2001 From: iandyh Date: Fri, 1 Dec 2023 16:03:14 +0900 Subject: [PATCH 11/12] remove unused code --- shibuya/api/main.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/shibuya/api/main.go b/shibuya/api/main.go index 49b93dbe..280989cd 100644 --- a/shibuya/api/main.go +++ b/shibuya/api/main.go @@ -448,11 +448,6 @@ func (s *ShibuyaAPI) collectionCreateHandler(w http.ResponseWriter, r *http.Requ } func (s *ShibuyaAPI) collectionDeleteHandler(w http.ResponseWriter, r *http.Request, params httprouter.Params) { - account := model.GetAccountBySession(r) - if account == nil { - s.handleErrors(w, makeLoginError()) - return - } collection, err := checkCollectionOwnership(r, params) if err != nil { s.handleErrors(w, err) From 33323887ec10449f30d596fab65d6cb612365f1e Mon Sep 17 00:00:00 2001 From: iandyh Date: Tue, 12 Dec 2023 16:24:19 +0900 Subject: [PATCH 12/12] add ownership to metrics streaming as well --- shibuya/api/main.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/shibuya/api/main.go b/shibuya/api/main.go index 280989cd..ec96691d 100644 --- a/shibuya/api/main.go +++ b/shibuya/api/main.go @@ -733,7 +733,11 @@ func (s *ShibuyaAPI) planLogHandler(w http.ResponseWriter, r *http.Request, para } func (s *ShibuyaAPI) streamCollectionMetrics(w http.ResponseWriter, r *http.Request, params httprouter.Params) { - // Currently we don't do authentication for simplicity. + collection, err := checkCollectionOwnership(r, params) + if err != nil { + s.handleErrors(w, err) + return + } flusher, ok := w.(http.Flusher) if !ok { http.Error(w, "Streaming unsupported!", http.StatusInternalServerError) @@ -744,11 +748,6 @@ func (s *ShibuyaAPI) streamCollectionMetrics(w http.ResponseWriter, r *http.Requ w.Header().Set("Connection", "keep-alive") w.Header().Set("Access-Control-Allow-Origin", "*") - collection, err := getCollection(params.ByName("collection_id")) - if err != nil { - s.handleErrors(w, err) - return - } clientIP := retrieveClientIP(r) item := &controller.ApiMetricStream{ StreamClient: make(chan *controller.ApiMetricStreamEvent),