Addresses Review Comments

RMI-PACTA · Jan 18, 2024 · f41f88f · f41f88f
1 parent aa9899e
commit f41f88f
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 16 deletions.
diff --git a/frontend/components/analysis/ListView.vue b/frontend/components/analysis/ListView.vue
@@ -125,7 +125,7 @@ const deleteSelected = () => Promise.all([selectedRows.value.map((row) => delete
             icon="pi pi-external-link"
             class="p-button-outlined p-button-xs"
             :label="tt('View')"
-            :to="`${apiServerURL}/report/${slotProps.data.id}`"
+            :to="`${apiServerURL}/report/${slotProps.data.id}/`"
             new-tab
           />
         </template>

diff --git a/reportsrv/reportsrv.go b/reportsrv/reportsrv.go
@@ -79,7 +79,11 @@ func (s *Server) verifyRequest(next http.Handler) http.Handler {
 
 func (s *Server) RegisterHandlers(r chi.Router) {
 	r.Use(s.verifyRequest)
-	r.Get("/report/{analysis_id}", s.serveReport)
+	r.Get("/report/{analysis_id}", func(w http.ResponseWriter, r *http.Request) {
+		analysisID := chi.URLParam(r, "analysis_id")
+		newPath := "/report/" + analysisID + "/"
+		http.Redirect(w, r, newPath, http.StatusTemporaryRedirect)
+	})
 	r.Get("/report/{analysis_id}/*", s.serveReport)
 }
 
@@ -134,18 +138,18 @@ func (s *Server) serveReport(w http.ResponseWriter, r *http.Request) {
 		// Container is just 'reports', we can ignore that.
 		b, ok := blobs[aa.Blob.ID]
 		if !ok {
-			s.logger.Error("no blob loaded for blob ID", zap.String("analysis_artifact_id", string(a.ID)), zap.String("blob_id", string(aa.Blob.ID)))
+			s.logger.Error("no blob loaded for blob ID", zap.String("analysis_artifact_id", string(aa.ID)), zap.String("blob_id", string(aa.Blob.ID)))
 			continue
 		}
 		uri := string(b.BlobURI)
 		_, path, ok := blob.SplitURI(s.blob.Scheme(), uri)
 		if !ok {
-			s.logger.Error("blob had invalid URI", zap.String("analysis_artifact_id", string(a.ID)), zap.String("blob_uri", uri))
+			s.logger.Error("blob had invalid URI", zap.String("analysis_artifact_id", string(aa.ID)), zap.String("blob_uri", uri))
 			continue
 		}
 		_, uriPath, ok := strings.Cut(path, "/")
 		if !ok {
-			s.logger.Error("path had no UUID prefix", zap.String("analysis_artifact_id", string(a.ID)), zap.String("blob_uri", uri), zap.String("blob_path", path))
+			s.logger.Error("path had no UUID prefix", zap.String("analysis_artifact_id", string(aa.ID)), zap.String("blob_uri", uri), zap.String("blob_path", path))
 			continue
 		}
 
@@ -241,7 +245,7 @@ func (s *Server) doAuthzAndAuditLog(a *pacta.Analysis, aa *pacta.AnalysisArtifac
 			return allowIfAuditLogSaves()
 		}
 	}
-	s.logger.Info("unauthorized user attempted to read asset", zap.String("user_id", string(actorID)))
+	s.logger.Info("unauthorized user attempted to read asset", zap.String("user_id", string(actorID)), zap.String("analysis_artifact_id", string(aa.ID)), zap.String("analysis_id", string(a.ID)))
 	http.Error(w, http.StatusText(http.StatusUnauthorized), http.StatusUnauthorized)
 	return false
 }

diff --git a/reportsrv/reportsrv_test.go b/reportsrv/reportsrv_test.go
@@ -20,6 +20,8 @@ import (
 func TestServeReport(t *testing.T) {
 
 	srv, env := setup(t)
+	router := chi.NewRouter()
+	srv.RegisterHandlers(router)
 
 	aIDStr := "analysis.id1"
 	analysisID := pacta.AnalysisID(aIDStr)
@@ -86,20 +88,20 @@ func TestServeReport(t *testing.T) {
 		"test://reports/1111-2222-3333-4444/lib/some/package.js": jsContent,
 	}
 
+	standardPath := "/report/" + aIDStr + "/"
 	cases := []struct {
 		asUser          pacta.UserID
 		path            string
 		wantErr         int
 		wantContentType string
 		wantRespContent string
 	}{{
-		asUser:          userID,
-		path:            "/report/" + aIDStr,
-		wantContentType: "text/html",
-		wantRespContent: htmlContent,
+		asUser:  userID,
+		path:    "/report/" + aIDStr,
+		wantErr: http.StatusTemporaryRedirect,
 	}, {
 		asUser:          userID,
-		path:            "/report/" + aIDStr + "/",
+		path:            standardPath,
 		wantContentType: "text/html",
 		wantRespContent: htmlContent,
 	}, {
@@ -114,22 +116,22 @@ func TestServeReport(t *testing.T) {
 		wantRespContent: jsContent,
 	}, {
 		asUser:          "",
-		path:            "/report/" + aIDStr,
+		path:            standardPath,
 		wantContentType: "text/html",
 		wantErr:         http.StatusUnauthorized,
 	}, {
 		asUser:          otherUserID,
-		path:            "/report/" + aIDStr,
+		path:            standardPath,
 		wantContentType: "text/html",
 		wantErr:         http.StatusUnauthorized,
 	}, {
 		asUser:          adminUserID,
-		path:            "/report/" + aIDStr,
+		path:            standardPath,
 		wantContentType: "text/html",
 		wantRespContent: htmlContent,
 	}, {
 		asUser:          userID,
-		path:            "/report/a-nonsense-report",
+		path:            "/report/a-nonsense-report/",
 		wantContentType: "text/html",
 		wantErr:         http.StatusNotFound,
 	}}
@@ -149,7 +151,7 @@ func TestServeReport(t *testing.T) {
 			r := httptest.NewRequest(http.MethodGet, c.path, nil).WithContext(ctx)
 			w := httptest.NewRecorder()
 
-			srv.serveReport(w, r)
+			router.ServeHTTP(w, r)
 
 			res := w.Result()
 			// Successful response...