diff --git a/changes/12274-return-code-for-password-reset b/changes/12274-return-code-for-password-reset new file mode 100644 index 000000000..0b4b0667c --- /dev/null +++ b/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 + diff --git a/server/service/hosts.go b/server/service/hosts.go index 0cd88c8b5..9be38b2bc 100644 --- a/server/service/hosts.go +++ b/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 } diff --git a/server/service/integration_core_test.go b/server/service/integration_core_test.go index 0013875c2..9b9bf8c2e 100644 --- a/server/service/integration_core_test.go +++ b/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() { diff --git a/server/service/middleware/authzcheck/authzcheck.go b/server/service/middleware/authzcheck/authzcheck.go index 965748efb..d2baa022b 100644 --- a/server/service/middleware/authzcheck/authzcheck.go +++ b/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) } diff --git a/server/service/sessions.go b/server/service/sessions.go index a37ad8391..84f6308d2 100644 --- a/server/service/sessions.go +++ b/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 } diff --git a/server/service/users.go b/server/service/users.go index c73504577..797158e10 100644 --- a/server/service/users.go +++ b/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)