Merge pull request #142 from uc-cdis/feat/delete_policy_error

arborist/server.go

@@ -209,7 +209,7 @@ func (server *Server) handleHealth(w http.ResponseWriter, r *http.Request) {
 		_ = response.write(w, r)
 		return
 	}
-	w.WriteHeader(http.StatusOK)
+	_ = jsonResponseFrom("Healthy", http.StatusOK).write(w, r)
 }
 
 func handleNotFound(w http.ResponseWriter, r *http.Request) {
@@ -1304,14 +1304,50 @@ func (server *Server) handleUserRevokeAll(w http.ResponseWriter, r *http.Request
 func (server *Server) handleUserRevokePolicy(w http.ResponseWriter, r *http.Request) {
 	username := mux.Vars(r)["username"]
 	policyName := mux.Vars(r)["policyName"]
-	errResponse := revokeUserPolicy(server.db, username, policyName, getAuthZProvider(r))
-	if errResponse != nil {
-		errResponse.log.write(server.logger)
-		_ = errResponse.write(w, r)
+	authzProvider := getAuthZProvider(r)
+	policyInfo, err := fetchUserPolicyInfo(server.db, username, policyName)
+
+	if err != nil {
+		server.logger.Info("Error Fetching policy Info: %s", err.Error())
+		msg := fmt.Sprintf("Error Fetching policy Info: %s", err.Error())
+		response := newErrorResponse(msg, http.StatusInternalServerError, nil)
+		_ = response.write(w, r)
 		return
 	}
-	server.logger.Info("revoked policy %s for user %s", policyName, username)
-	_ = jsonResponseFrom(nil, http.StatusNoContent).write(w, r)
+
+	if policyInfo != nil {
+
+		dbAuthzProvider := ""
+		providerExists := policyInfo.AuthzProvider.Valid
+		if providerExists {
+			dbAuthzProvider = policyInfo.AuthzProvider.String
+		}
+		server.logger.Debug("Policy - {name: %s, authz_provider: %s, expires_at: %s} assigned to user %s",
+			policyInfo.PolicyName, dbAuthzProvider, policyInfo.ExpiresAt, policyInfo.Username)
+
+		if !authzProvider.Valid || (providerExists && dbAuthzProvider == authzProvider.String) {
+			errResponse := revokeUserPolicy(
+				server.db, username, policyName, authzProvider)
+			if errResponse != nil {
+				errResponse.log.write(server.logger)
+				_ = errResponse.write(w, r)
+				return
+			}
+			server.logger.Info("revoked policy %s for user %s", policyName, username)
+			_ = jsonResponseFrom(nil, http.StatusNoContent).write(w, r)
+		} else {
+			server.logger.Info("Cannot revoke policy `%s`. Policy authz_provider `%s` and request authz_provider `%s` mismatch",
+				policyName, policyInfo.AuthzProvider.String, authzProvider.String)
+			msg := fmt.Sprintf("Cannot revoke policy `%s`. Authz_provider Mismatch", policyName)
+			errResponse := newErrorResponse(msg, http.StatusUnauthorized, nil)
+			errResponse.log.write(server.logger)
+			_ = errResponse.write(w, r)
+		}
+	} else {
+		server.logger.Info("Policy `%s` does not exist for user `%s`: not revoking. Check if it is assigned through a group.",
+			policyName, username)
+		_ = jsonResponseFrom(nil, http.StatusBadRequest).write(w, r)
+	}
 }
 
 func (server *Server) handleUserListResources(w http.ResponseWriter, r *http.Request) {

arborist/server_test.go

@@ -1965,14 +1965,16 @@ func TestServer(t *testing.T) {
 		})
 
 		t.Run("RevokePolicy", func(t *testing.T) {
-			test := func(authzProvider string, expected bool, msg string) {
+			test := func(authzProvider string, httpStatusCode int, expected bool, msg string) {
 				w := httptest.NewRecorder()
 				url := fmt.Sprintf("/user/%s/policy/%s", username, policyName)
 				req := newRequest("DELETE", url, nil)
-				req.Header.Add("X-AuthZ-Provider", authzProvider)
+				if authzProvider != "" {
+					req.Header.Add("X-AuthZ-Provider", authzProvider)
+				}
 				handler.ServeHTTP(w, req)
-				if w.Code != http.StatusNoContent {
-					httpError(t, w, "couldn't revoke policy")
+				if w.Code != httpStatusCode {
+					httpError(t, w, fmt.Sprintf("revoke policy: expected status code `%d`, got status code `%d`", httpStatusCode, w.Code))
 				}
 				// look up user again and check if policy is still there
 				w = httptest.NewRecorder()
@@ -2006,8 +2008,12 @@ func TestServer(t *testing.T) {
 					assert.Fail(t, fmt.Sprintf(msg, w.Body.String()))
 				}
 			}
-			test("yyy", true, "shouldn't revoke policy; got response body: %s")
-			test("xxx", false, "didn't revoke policy correctly; got response body: %s")
+			test("yyy", http.StatusUnauthorized, true, "shouldn't revoke policy; got response body: %s")
+			test("xxx", http.StatusNoContent, false, "didn't revoke policy correctly; got response body: %s")
+
+			// If no authz header is provided, the policy should still be revoked.
+			grantUserPolicy(t, username, policyName, "") // Granting policy again to revoke
+			test("", http.StatusNoContent, false, "didn't revoke policy correctly; got response body: %s")
 		})
 
 		timestamp := time.Now().Add(time.Hour).Format(time.RFC3339)
@@ -2642,6 +2648,42 @@ func TestServer(t *testing.T) {
 			t.Run("InvalidJSON", func(t *testing.T) {
 			})
 		})
+		userToAttemptRevoke := testGroupUser2
+		t.Run("RevokePolicyThroughUser", func(t *testing.T) {
+			w := httptest.NewRecorder()
+			url := fmt.Sprintf("/user/%s/policy/%s", userToAttemptRevoke, policyName)
+			req := newRequest("DELETE", url, nil)
+			req.Header.Add("X-AuthZ-Provider", "xxx")
+			handler.ServeHTTP(w, req)
+			assert.Equal(t, w.Code, http.StatusBadRequest, " Group policy must not be revoked directly through user")
+			w = httptest.NewRecorder()
+			url = fmt.Sprintf("/user/%s", userToAttemptRevoke)
+			req = newRequest("GET", url, nil)
+			handler.ServeHTTP(w, req)
+			if w.Code != http.StatusOK {
+				httpError(t, w, "couldn't read user policies")
+			}
+			result := struct {
+				Name     string `json:"name"`
+				Email    string `json:"email"`
+				Policies []struct {
+					Name string `json:"policy"`
+				} `json:"policies"`
+				Groups []string `json:"groups"`
+			}{}
+			err = json.Unmarshal(w.Body.Bytes(), &result)
+			if err != nil {
+				httpError(t, w, "couldn't read response from user read")
+			}
+			found := false
+			for _, policy := range result.Policies {
+				if policy.Name == policyName {
+					found = true
+					break
+				}
+			}
+			assert.True(t, found, fmt.Sprintf("shouldn't revoke group policy through user; got response body: %s", result))
+		})
 
 		t.Run("RevokePolicy", func(t *testing.T) {
 			test := func(authzProvider string, expected bool, msg string) {

arborist/user.go

@@ -150,6 +150,44 @@ func userWithName(db *sqlx.DB, name string) (*UserFromQuery, error) {
 	return &user, nil
 }
 
+type UserPolicyInfoFromQuery struct {
+	Username      string         `db:"username"`
+	PolicyName    string         `db:"policy_name"`
+	ExpiresAt     *time.Time     `db:"expires_at"`
+	AuthzProvider sql.NullString `db:"authz_provider"`
+}
+
+func fetchUserPolicyInfo(db *sqlx.DB, user_name string, policy_name string) (*UserPolicyInfoFromQuery, error) {
+
+	stmt := `
+		SELECT usr.name as username, policy.name AS policy_name, usr_policy.expires_at as expires_at, usr_policy.authz_provider as authz_provider
+			FROM usr_policy
+			INNER JOIN policy ON policy.id = usr_policy.policy_id
+			INNER JOIN usr on usr_policy.usr_id = usr.id 
+			WHERE usr.name like $1 and policy.name like $2;
+	`
+	policyInfoList := []UserPolicyInfoFromQuery{}
+	err := db.Select(
+		&policyInfoList,
+		stmt,
+		user_name,   // $1
+		policy_name, // $2
+	)
+	if err != nil {
+		return nil, err
+	}
+	if len(policyInfoList) == 0 {
+		return nil, nil
+	}
+	if len(policyInfoList) > 1 {
+		fmt.Printf("WARN: More than one record of the policy `%s` exists for the user `%s`. Returning the first matching record",
+			policy_name, user_name)
+	}
+
+	policyInfo := policyInfoList[0]
+	return &policyInfo, nil
+}
+
 func listUsersFromDb(db *sqlx.DB) ([]UserFromQuery, error) {
 	stmt := `
 		SELECT