Fix 500 return code for several endpoints. (#14859)

Fixed 500 return code from several endpoints.

/api/v1/fleet/perform_required_password_reset
- Now returns 403 when Authorization token is missing

/api/v1/fleet/hosts_summary
- Now returns 400 when low_disk_space parameter is invalid

/api/v1/fleet/demologin
- Now returns 403

/api/v1/fleet/sessions/*
- Now returns 400 on invalid input

#12274

# Checklist for submitter

If some of the following don't apply, delete the relevant line.

- [x] Changes file added for user-visible changes in `changes/` or
`orbit/changes/`.
See [Changes
files](https://fleetdm.com/docs/contributing/committing-changes#changes-files)
for more information.
- [x] Added/updated tests
- [x] Manual QA for all new/changed functionality

changes/12274-return-code-for-password-reset

@@ -0,0 +1,11 @@
+Fixed 500 return code from several endpoints.
+
+/api/v1/fleet/perform_required_password_reset
+- Now returns 403 when Authorization token is missing
+
+/api/v1/fleet/hosts_summary
+- Now returns 400 when low_disk_space parameter is invalid
+
+/api/v1/fleet/sessions/*
+- Now returns 404 when session cannot be found
+

server/service/hosts.go

@@ -456,13 +456,6 @@ func (r getHostSummaryResponse) error() error { return r.Err }
 
 func getHostSummaryEndpoint(ctx context.Context, request interface{}, svc fleet.Service) (errorer, error) {
 	req := request.(*getHostSummaryRequest)
-	if req.LowDiskSpace != nil {
-		if *req.LowDiskSpace < 1 || *req.LowDiskSpace > 100 {
-			err := ctxerr.Errorf(ctx, "invalid low_disk_space threshold, must be between 1 and 100: %d", *req.LowDiskSpace)
-			return getHostSummaryResponse{Err: err}, nil
-		}
-	}
-
 	summary, err := svc.GetHostSummary(ctx, req.TeamID, req.Platform, req.LowDiskSpace)
 	if err != nil {
 		return getHostSummaryResponse{Err: err}, nil
@@ -475,6 +468,14 @@ func getHostSummaryEndpoint(ctx context.Context, request interface{}, svc fleet.
 }
 
 func (svc *Service) GetHostSummary(ctx context.Context, teamID *uint, platform *string, lowDiskSpace *int) (*fleet.HostSummary, error) {
+	if lowDiskSpace != nil {
+		if *lowDiskSpace < 1 || *lowDiskSpace > 100 {
+			svc.authz.SkipAuthorization(ctx)
+			return nil, ctxerr.Wrap(
+				ctx, badRequest(fmt.Sprintf("invalid low_disk_space threshold, must be between 1 and 100: %d", *lowDiskSpace)),
+			)
+		}
+	}
 	if err := svc.authz.Authorize(ctx, &fleet.Host{TeamID: teamID}, fleet.ActionList); err != nil {
 		return nil, err
 	}

server/service/integration_core_test.go

@@ -1774,7 +1774,7 @@ func (s *integrationTestSuite) TestGetHostSummary() {
 	require.Len(t, resp.Platforms, 0)
 
 	// invalid low_disk_space value is still validated and results in error
-	s.DoJSON("GET", "/api/latest/fleet/host_summary", nil, http.StatusInternalServerError, &resp, "low_disk_space", "1234") // TODO: should be 400, see #4406
+	s.DoJSON("GET", "/api/latest/fleet/host_summary", nil, http.StatusBadRequest, &resp, "low_disk_space", "1234")
 }
 
 func (s *integrationTestSuite) TestGlobalPoliciesProprietary() {
@@ -3413,10 +3413,18 @@ func (s *integrationTestSuite) TestUsers() {
 	}
 	s.DoJSON("PATCH", fmt.Sprintf("/api/latest/fleet/users/%d", u.ID+1), params, http.StatusNotFound, &modResp)
 
-	// perform a required password change as the user themselves
-	s.token = s.getTestToken(u.Email, userRawPwd)
 	var perfPwdResetResp performRequiredPasswordResetResponse
 	newRawPwd := test.GoodPassword2
+	// Try a required password change without authentication
+	s.DoJSON(
+		"POST", "/api/latest/fleet/perform_required_password_reset", performRequiredPasswordResetRequest{
+			Password: newRawPwd,
+			ID:       u.ID,
+		}, http.StatusForbidden, &perfPwdResetResp,
+	)
+
+	// perform a required password change as the user themselves
+	s.token = s.getTestToken(u.Email, userRawPwd)
 	s.DoJSON("POST", "/api/latest/fleet/perform_required_password_reset", performRequiredPasswordResetRequest{
 		Password: newRawPwd,
 		ID:       u.ID,
@@ -4836,17 +4844,15 @@ func (s *integrationTestSuite) TestSessionInfo() {
 	assert.Equal(t, ssn.ID, getResp.SessionID)
 	assert.Equal(t, uint(1), getResp.UserID)
 
-	// get info about session - non-existing: appears to deliberately return 500 due to forbidden,
-	// which takes precedence vs the not found returned by the datastore (it still shouldn't be a
-	// 500 though).
-	s.DoJSON("GET", fmt.Sprintf("/api/latest/fleet/sessions/%d", ssn.ID+1), nil, http.StatusInternalServerError, &getResp)
+	// get info about session
+	s.DoJSON("GET", fmt.Sprintf("/api/latest/fleet/sessions/%d", ssn.ID+1), nil, http.StatusNotFound, &getResp)
 
 	// delete session
 	var delResp deleteSessionResponse
 	s.DoJSON("DELETE", fmt.Sprintf("/api/latest/fleet/sessions/%d", ssn.ID), nil, http.StatusOK, &delResp)
 
-	// delete session - non-existing: again, 500 due to forbidden instead of 404.
-	s.DoJSON("DELETE", fmt.Sprintf("/api/latest/fleet/sessions/%d", ssn.ID), nil, http.StatusInternalServerError, &delResp)
+	// delete session - non-existing
+	s.DoJSON("DELETE", fmt.Sprintf("/api/latest/fleet/sessions/%d", ssn.ID), nil, http.StatusNotFound, &delResp)
 }
 
 func (s *integrationTestSuite) TestAppConfig() {

server/service/middleware/authzcheck/authzcheck.go

@@ -51,6 +51,7 @@ func (m *Middleware) AuthzCheck() endpoint.Middleware {
 			// If authorization was not checked, return a response that will
 			// marshal to a generic error and log that the check was missed.
 			if !authzctx.Checked() {
+				// Getting to here means there is an authorization-related bug in our code.
 				return nil, authz.CheckMissingWithResponse(response)
 			}
 

server/service/sessions.go

@@ -55,6 +55,7 @@ func getInfoAboutSessionEndpoint(ctx context.Context, request interface{}, svc f
 func (svc *Service) GetInfoAboutSession(ctx context.Context, id uint) (*fleet.Session, error) {
 	session, err := svc.ds.SessionByID(ctx, id)
 	if err != nil {
+		svc.authz.SkipAuthorization(ctx)
 		return nil, err
 	}
 
@@ -96,6 +97,7 @@ func deleteSessionEndpoint(ctx context.Context, request interface{}, svc fleet.S
 func (svc *Service) DeleteSession(ctx context.Context, id uint) error {
 	session, err := svc.ds.SessionByID(ctx, id)
 	if err != nil {
+		svc.authz.SkipAuthorization(ctx)
 		return err
 	}
 

server/service/users.go

@@ -832,7 +832,9 @@ func performRequiredPasswordResetEndpoint(ctx context.Context, request interface
 func (svc *Service) PerformRequiredPasswordReset(ctx context.Context, password string) (*fleet.User, error) {
 	vc, ok := viewer.FromContext(ctx)
 	if !ok {
-		return nil, fleet.ErrNoContext
+		// No user in the context -- authentication issue
+		svc.authz.SkipAuthorization(ctx)
+		return nil, authz.ForbiddenWithInternal("No user in the context", nil, nil, nil)
 	}
 	if !vc.CanPerformPasswordReset() {
 		svc.authz.SkipAuthorization(ctx)