Merge pull request #391 from helgi/fix_213

fix(auth): return a 409 if a user is cancelled that has apps

Author: helgi

client/cmd/auth.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"net/url"
 	"os"
+	"reflect"
 	"strings"
 	"syscall"
 
@@ -241,8 +242,11 @@ func Cancel(username string, password string, yes bool) error {
 	}
 
 	err = auth.Delete(c, username)
-
-	if err != nil {
+	cleanup := fmt.Errorf("\n%s %s\n\n", "409", "Conflict")
+	if reflect.DeepEqual(err, cleanup) {
+		fmt.Printf("%s still has application associated with it. Transfer ownership or delete them first\n", username)
+		return nil
+	} else if err != nil {
 		return err
 	}
 

client/controller/models/auth/auth_test.go

@@ -6,6 +6,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"reflect"
 	"testing"
 
 	"github.com/deis/workflow/client/controller/client"
@@ -18,6 +19,7 @@ const passwdExpected string = `{"username":"test","password":"old","new_password
 const regenAllExpected string = `{"all":true}`
 const regenUserExpected string = `{"username":"test"}`
 const cancelUserExpected string = `{"username":"foo"}`
+const cancelAdminExpected string = `{"username":"admin"}`
 
 type fakeHTTPServer struct {
 	regenBodyEmpty    bool
@@ -131,7 +133,12 @@ func (f *fakeHTTPServer) ServeHTTP(res http.ResponseWriter, req *http.Request) {
 			res.Write(nil)
 		}
 
-		if string(body) == cancelUserExpected && !f.cancelUsername {
+		if string(body) == cancelAdminExpected && !f.cancelUsername {
+			f.cancelUsername = true
+			res.WriteHeader(http.StatusConflict)
+			res.Write(nil)
+			return
+		} else if string(body) == cancelUserExpected && !f.cancelUsername {
 			f.cancelUsername = true
 			res.WriteHeader(http.StatusNoContent)
 			res.Write(nil)
@@ -251,6 +258,31 @@ func TestDelete(t *testing.T) {
 	}
 }
 
+func TestDeleteUserApp(t *testing.T) {
+	t.Parallel()
+
+	handler := fakeHTTPServer{cancelUsername: false, cancelEmpty: false}
+	server := httptest.NewServer(&handler)
+	defer server.Close()
+
+	u, err := url.Parse(server.URL)
+
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	httpClient := client.CreateHTTPClient(false)
+	client := client.Client{HTTPClient: httpClient, ControllerURL: *u}
+
+	err = Delete(&client, "admin")
+	// should be a 409 Conflict
+
+	expected := fmt.Errorf("\n%s %s\n\n", "409", "Conflict")
+	if reflect.DeepEqual(err, expected) == false {
+		t.Errorf("got '%s' but expected '%s'", err, expected)
+	}
+}
+
 func TestRegenerate(t *testing.T) {
 	t.Parallel()
 

rootfs/api/tests/test_auth.py

@@ -221,6 +221,20 @@ def test_cancel(self):
                                       HTTP_AUTHORIZATION='token {}'.format(self.admin_token))
         self.assertEqual(response.status_code, 204)
 
+        # user can not be deleted if it has an app attached to it
+        response = self.client.post(
+            '/v2/apps',
+            HTTP_AUTHORIZATION='token {}'.format(self.admin_token)
+        )
+        self.assertEqual(response.status_code, 201)
+        app_id = response.data['id']  # noqa
+        self.assertIn('id', response.data)
+
+        response = self.client.delete(url, json.dumps({'username': str(self.admin)}),
+                                      content_type='application/json',
+                                      HTTP_AUTHORIZATION='token {}'.format(self.admin_token))
+        self.assertEqual(response.status_code, 409)
+
     def test_passwd(self):
         """Test that a registered user can change the password."""
         # test registration workflow

rootfs/api/views.py

@@ -77,6 +77,11 @@ def destroy(self, request, **kwargs):
             else:
                 raise PermissionDenied()
 
+        # A user can not be removed without apps changing ownership first
+        if len(models.App.objects.filter(owner=target_obj)) > 0:
+            msg = '{} still has applications assigned. Delete or transfer ownership'.format(str(target_obj))  # noqa
+            return Response({'detail': msg}, status=status.HTTP_409_CONFLICT)
+
         target_obj.delete()
         return Response(status=status.HTTP_204_NO_CONTENT)