Loading
Return forbidden from the admin-or-self check on crowdin_username
What does this MR do and why?
- Two "admin or self" authorization checks disagreed on their failure response.
Api::V1::UsersController#crowdin_usernamereturned401 Unauthorized, whileApi::V1::Users::MentorsController#ensure_admin_or_owner!returned403 Forbiddenfor the equivalent condition. This MR aligns the first onto403. 403is the correct status here. Both checks run afterensure_logged_in, so the caller is always authenticated by the time either is reached.401means the request lacked valid credentials, which is never the case at that point the caller is known, they simply are not the owner or an admin.- This is also the convention the codebase already sets.
ApiApplicationControllerdraws exactly this line:ensure_logged_inreturns401, whileensure_approved_community_memberandensure_adminreturn403. Thecrowdin_usernamecheck was the only place that crossed it. - On the security question raised in the issue: neither status discloses more than the other. Both sit behind authentication, and
ensure_user_existsalready returns404for an unknown user before this check is reached, so the change reveals nothing new about which accounts exist. - I audited every
401and403incontributors/app/to confirm this does not simply relocate the inconsistency. After this change the rule holds without exception. Every401is an authentication failure: the twologged_in?guards inApiApplicationController, and the onboarding trigger token comparison. Every403is an authorization failure on an authenticated caller:approved_community_member?,admin?,owner?, admin-or-owner, and now admin-or-self.
The 'an endpoint requiring login' shared example is deliberately untouched and still asserts 401. That covers the genuinely unauthenticated request, which is a different case and keeps the correct status.
Differences :
This changes an API response status and has no UI surface, so there is nothing to screenshot.
- Before:
PATCH /api/v1/users/:id/crowdin_usernamereturned401 Unauthorizedwhen an authenticated user who was neither the owner nor an admin made the request. - After: the same request returns
403 Forbidden. The unauthenticated case is unaffected and still returns401 Unauthorized.
Validation steps
Automated:
bundle exec rspec spec/requests/api/v1/users_controller_spec.rbManual, against a local environment:
- Sign in as a non-admin user.
- Send
PATCH /api/v1/users/:id/crowdin_usernameusing a different user's ID. - The response is now
403 Forbidden; it was previously401 Unauthorized. - Sign out and repeat the same request. It still returns
401 Unauthorized, confirming the authentication and authorization cases stay distinct. - Repeat step 2 as an admin, and as the owning user. Both still return
200 OK.
Related to
- Closes #648 (closed)
- Original observation: !2556 (closed)
Edited by Jeston Singh