Merge pull request #4147 from Joshua-Anderson/admin-user-cancelation

feat(controller): allow admins to delete users

Author: Matthew Fisher

client-go/cmd/auth.go

@@ -199,10 +199,12 @@ func Cancel(username string, password string, yes bool) error {
 		return err
 	}
 
-	fmt.Println("Please log in again in order to cancel this account")
+	if username == "" || password != "" {
+		fmt.Println("Please log in again in order to cancel this account")
 
-	if err = Login(c.ControllerURL.String(), username, password, c.SSLVerify); err != nil {
-		return err
+		if err = Login(c.ControllerURL.String(), username, password, c.SSLVerify); err != nil {
+			return err
+		}
 	}
 
 	if yes == false {
@@ -214,7 +216,13 @@ func Cancel(username string, password string, yes bool) error {
 			return err
 		}
 
-		fmt.Printf("cancel account %s at %s? (y/N): ", c.Username, c.ControllerURL.String())
+		deletedUser := username
+
+		if deletedUser == "" {
+			deletedUser = c.Username
+		}
+
+		fmt.Printf("cancel account %s at %s? (y/N): ", deletedUser, c.ControllerURL.String())
 		fmt.Scanln(&confirm)
 
 		if strings.ToLower(confirm) == "y" {
@@ -227,14 +235,17 @@ func Cancel(username string, password string, yes bool) error {
 		return nil
 	}
 
-	err = auth.Delete(c)
+	err = auth.Delete(c, username)
 
 	if err != nil {
 		return err
 	}
 
-	if err := client.Delete(); err != nil {
-		return err
+	// If user targets themselves, logout.
+	if username != "" || c.Username == username {
+		if err := client.Delete(); err != nil {
+			return err
+		}
 	}
 
 	fmt.Println("Account cancelled")

client-go/controller/api/auth.go

@@ -31,6 +31,11 @@ type AuthRegenerateRequest struct {
 	All  bool   `json:"all,omitempty"`
 }
 
+// AuthCancelRequest is the definition of POST /v1/auth/cancel/.
+type AuthCancelRequest struct {
+	Username string `json:"username"`
+}
+
 // AuthRegenerateResponse is the definition of /v1/auth/tokens/.
 type AuthRegenerateResponse tokenResponse
 

client-go/controller/models/auth/auth.go

@@ -44,8 +44,20 @@ func Login(c *client.Client, username, password string) (string, error) {
 }
 
 // Delete deletes a user.
-func Delete(c *client.Client) error {
-	_, err := c.BasicRequest("DELETE", "/v1/auth/cancel/", nil)
+func Delete(c *client.Client, username string) error {
+	var body []byte
+	var err error
+
+	if username != "" {
+		req := api.AuthCancelRequest{Username: username}
+		body, err = json.Marshal(req)
+
+		if err != nil {
+			return err
+		}
+	}
+
+	_, err = c.BasicRequest("DELETE", "/v1/auth/cancel/", body)
 	return err
 }
 

client-go/controller/models/auth/auth_test.go

@@ -17,14 +17,17 @@ const loginExpected string = `{"username":"test","password":"opensesame"}`
 const passwdExpected string = `{"username":"test","password":"old","new_password":"new"}`
 const regenAllExpected string = `{"all":true}`
 const regenUserExpected string = `{"username":"test"}`
+const cancelUserExpected string = `{"username":"foo"}`
 
 type fakeHTTPServer struct {
 	regenBodyEmpty    bool
 	regenBodyAll      bool
 	regenBodyUsername bool
+	cancelEmpty       bool
+	cancelUsername    bool
 }
 
-func (f fakeHTTPServer) ServeHTTP(res http.ResponseWriter, req *http.Request) {
+func (f *fakeHTTPServer) ServeHTTP(res http.ResponseWriter, req *http.Request) {
 	res.Header().Add("DEIS_API_VERSION", version.APIVersion)
 
 	if req.URL.Path == "/v1/auth/register/" && req.Method == "POST" {
@@ -55,6 +58,7 @@ func (f fakeHTTPServer) ServeHTTP(res http.ResponseWriter, req *http.Request) {
 			fmt.Println(err)
 			res.WriteHeader(http.StatusInternalServerError)
 			res.Write(nil)
+			return
 		}
 
 		if string(body) != loginExpected {
@@ -119,7 +123,28 @@ func (f fakeHTTPServer) ServeHTTP(res http.ResponseWriter, req *http.Request) {
 	}
 
 	if req.URL.Path == "/v1/auth/cancel/" && req.Method == "DELETE" {
-		res.WriteHeader(http.StatusNoContent)
+		body, err := ioutil.ReadAll(req.Body)
+
+		if err != nil {
+			fmt.Println(err)
+			res.WriteHeader(http.StatusInternalServerError)
+			res.Write(nil)
+		}
+
+		if string(body) == cancelUserExpected && !f.cancelUsername {
+			f.cancelUsername = true
+			res.WriteHeader(http.StatusNoContent)
+			res.Write(nil)
+			return
+		} else if string(body) == "" && !f.cancelEmpty {
+			f.cancelEmpty = true
+			res.WriteHeader(http.StatusNoContent)
+			res.Write(nil)
+			return
+		}
+
+		fmt.Printf("%s is not a valid body.", body)
+		res.WriteHeader(http.StatusInternalServerError)
 		res.Write(nil)
 		return
 	}
@@ -204,7 +229,7 @@ func TestPasswd(t *testing.T) {
 func TestDelete(t *testing.T) {
 	t.Parallel()
 
-	handler := fakeHTTPServer{}
+	handler := fakeHTTPServer{cancelUsername: false, cancelEmpty: false}
 	server := httptest.NewServer(&handler)
 	defer server.Close()
 
@@ -217,7 +242,11 @@ func TestDelete(t *testing.T) {
 	httpClient := client.CreateHTTPClient(false)
 	client := client.Client{HTTPClient: httpClient, ControllerURL: *u}
 
-	if err := Delete(&client); err != nil {
+	if err := Delete(&client, "foo"); err != nil {
+		t.Error(err)
+	}
+
+	if err := Delete(&client, ""); err != nil {
 		t.Error(err)
 	}
 }

client/deis.py

@@ -827,20 +827,26 @@ def auth_cancel(self, args):
         if not controller:
             self._logger.error('Not logged in to a Deis controller')
             sys.exit(1)
-        self._logger.info('Please log in again in order to cancel this account')
-        args['<controller>'] = controller
-        username = self.auth_login(args)
+        username = args.get('--username')
+
+        if not username and not args.get('--password'):
+            self._logger.info('Please log in again in order to cancel this account')
+            args['<controller>'] = controller
+            username = self.auth_login(args)
+
         if username:
             confirm = args.get('--yes')
             if not confirm:
                 confirm = raw_input(
                     "Cancel account \"{}\" at {}? (y/N) ".format(username, controller))
             if confirm in ['y', True]:
-                response = self._dispatch('delete', '/v1/auth/cancel')
+                response = self._dispatch('delete', '/v1/auth/cancel',
+                                          json.dumps({'username': username}))
                 if response.status_code == requests.codes.no_content:
-                    self._settings['controller'] = None
-                    self._settings['token'] = None
-                    self._settings.save()
+                    if not args.get('--username'):
+                        self._settings['controller'] = None
+                        self._settings['token'] = None
+                        self._settings.save()
                     self._logger.info('Account cancelled')
                 else:
                     self._logger.info('Account not changed')

controller/api/tests/test_auth.py

@@ -170,21 +170,31 @@ def test_cancel(self):
         """Test that a registered user can cancel her account."""
         # test registration workflow
         username, password = 'newuser', 'password'
-        first_name, last_name = 'Otto', 'Test'
-        email = 'autotest@deis.io'
         submit = {
             'username': username,
             'password': password,
-            'first_name': first_name,
-            'last_name': last_name,
-            'email': email,
+            'first_name': 'Otto',
+            'last_name': 'Test',
+            'email': 'autotest@deis.io',
             # try to abuse superuser/staff level perms
             'is_superuser': True,
             'is_staff': True,
         }
+
+        other_username, other_password = 'newuser2', 'password'
+        other_submit = {
+            'username': other_username,
+            'password': other_password,
+            'first_name': 'Test',
+            'last_name': 'Tester',
+            'email': 'autotest-2@deis.io',
+            'is_superuser': False,
+            'is_staff': False,
+        }
         url = '/v1/auth/register'
         response = self.client.post(url, json.dumps(submit), content_type='application/json')
         self.assertEqual(response.status_code, 201)
+
         # cancel the account
         url = '/v1/auth/cancel'
         user = User.objects.get(username=username)
@@ -193,6 +203,25 @@ def test_cancel(self):
                                       HTTP_AUTHORIZATION='token {}'.format(token))
         self.assertEqual(response.status_code, 204)
 
+        url = '/v1/auth/register'
+        response = self.client.post(url, json.dumps(other_submit), content_type='application/json')
+        self.assertEqual(response.status_code, 201)
+
+        # normal user can't delete another user
+        url = '/v1/auth/cancel'
+        other_user = User.objects.get(username=other_username)
+        other_token = Token.objects.get(user=other_user).key
+        response = self.client.delete(url, json.dumps({'username': self.admin.username}),
+                                      content_type='application/json',
+                                      HTTP_AUTHORIZATION='token {}'.format(other_token))
+        self.assertEqual(response.status_code, 403)
+
+        # admin can delete another user
+        response = self.client.delete(url, json.dumps({'username': other_username}),
+                                      content_type='application/json',
+                                      HTTP_AUTHORIZATION='token {}'.format(self.admin_token))
+        self.assertEqual(response.status_code, 204)
+
     def test_passwd(self):
         """Test that a registered user can change the password."""
         # test registration workflow

controller/api/views.py

@@ -26,8 +26,7 @@ class UserRegistrationViewSet(GenericViewSet,
     serializer_class = serializers.UserSerializer
 
 
-class UserManagementViewSet(GenericViewSet,
-                            mixins.DestroyModelMixin):
+class UserManagementViewSet(GenericViewSet):
     serializer_class = serializers.UserSerializer
 
     def get_queryset(self):
@@ -36,6 +35,20 @@ def get_queryset(self):
     def get_object(self):
         return self.get_queryset()[0]
 
+    def destroy(self, request, **kwargs):
+        calling_obj = self.get_object()
+        target_obj = calling_obj
+
+        if request.data.get('username'):
+            # if you "accidentally" target yourself, that should be fine
+            if calling_obj.username == request.data['username'] or calling_obj.is_superuser:
+                target_obj = get_object_or_404(User, username=request.data['username'])
+            else:
+                raise PermissionDenied()
+
+        target_obj.delete()
+        return Response(status=status.HTTP_204_NO_CONTENT)
+
     def passwd(self, request, **kwargs):
         caller_obj = self.get_object()
         target_obj = self.get_object()

docs/reference/api-v1.6.rst

@@ -14,6 +14,8 @@ What's New
 
 **New!** administrators no longer have to supply a password when changing another user's password.
 
+**New!** administrators no longer have to supply a password when deleting another user.
+
 Authentication
 --------------
 

tests/auth_test.go

@@ -13,11 +13,13 @@ var (
 	authLogoutCmd        = "auth:logout"
 	authRegisterCmd      = "auth:register http://deis.{{.Domain}} --username={{.UserName}} --password={{.Password}} --email={{.Email}}"
 	authCancelCmd        = "auth:cancel --username={{.UserName}} --password={{.Password}} --yes"
+	authCancelAdminCmd   = "auth:cancel --username={{.UserName}} --yes"
 	authRegenerateCmd    = "auth:regenerate"
 	authRegenerateUsrCmd = "auth:regenerate -u {{.UserName}}"
 	authRegenerateAllCmd = "auth:regenerate --all"
 	checkTokenCmd        = "apps:list"
 	authPasswdCmd        = "auth:passwd --username={{.UserName}} --password={{.Password}} --new-password={{.NewPassword}}"
+	authWhoamiCmd        = "auth:whoami"
 )
 
 func TestAuth(t *testing.T) {
@@ -39,6 +41,16 @@ func authSetup(t *testing.T) *utils.DeisTestConfig {
 
 func authCancel(t *testing.T, params *utils.DeisTestConfig) {
 	utils.Execute(t, authCancelCmd, params, false, "Account cancelled")
+	user := utils.GetGlobalConfig()
+
+	// Admins can delete other users.
+	user.UserName, user.Password = "cancel-test", "test"
+	utils.Execute(t, authRegisterCmd, user, false, "")
+	admin := utils.GetGlobalConfig()
+	utils.Execute(t, authLoginCmd, admin, false, "")
+	utils.Execute(t, authCancelAdminCmd, user, false, "Account cancelled")
+	// Make sure the admin is still logged in
+	utils.CheckList(t, authWhoamiCmd, admin, admin.UserName, false)
 }
 
 func authLoginTest(t *testing.T, params *utils.DeisTestConfig) {