Skip to content

fix(auth): decode MFA unenroll responses with id key#905

Merged
grdsdev merged 1 commit intosupabase:mainfrom
Tidex-Payroll-Shifts:fix-mfa-unenroll-response-id
Feb 6, 2026
Merged

fix(auth): decode MFA unenroll responses with id key#905
grdsdev merged 1 commit intosupabase:mainfrom
Tidex-Payroll-Shifts:fix-mfa-unenroll-response-id

Conversation

@kkarlsen06
Copy link
Contributor

@kkarlsen06 kkarlsen06 commented Feb 6, 2026

Summary

  • Rename factorId to id in AuthMFAUnenrollResponse to match the GoTrue API response shape ({"id":"..."})
  • Update test mock and assertion accordingly

Why

The GoTrue UnenrollFactorResponse struct serializes as {"id":"..."} (source), but the Swift SDK was decoding it as factor_id, causing a decoding mismatch.

Test plan

  • swift test --filter AuthClientTests/testMFAUnenroll

Closes #904

Summary by CodeRabbit

  • Refactor

    • Renamed the MFA unenroll response property from factorId to id. This is a breaking change for integrations that read the unenroll response.
  • Tests

    • Updated unenroll tests and assertions to expect the new response shape (id).

@kkarlsen06 kkarlsen06 requested a review from grdsdev as a code owner February 6, 2026 04:27
@coderabbitai
Copy link

coderabbitai bot commented Feb 6, 2026

📝 Walkthrough

Walkthrough

AuthMFAUnenrollResponse's public property was renamed from factorId to id to match the GoTrue API response; tests updated to mock {"id":"..."} and to read .id from the unenroll result.

Changes

Cohort / File(s) Summary
Response Type Definition
Sources/Auth/Types.swift
Renamed public property factorIdid on AuthMFAUnenrollResponse.
Test Updates
Tests/AuthTests/AuthClientTests.swift
Mock DELETE response body changed from {"factor_id":"123"} to {"id":"123"} and test assertion updated from .factorId to .id.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly matches the main change: renaming the property from factorId to id in AuthMFAUnenrollResponse to decode responses with the 'id' key from the GoTrue API.
Linked Issues check ✅ Passed The pull request fully addresses all coding requirements from issue #904: renaming the property to id, updating test mocks to match API response format, and updating test assertions.
Out of Scope Changes check ✅ Passed All changes are directly scoped to addressing the mismatch between the GoTrue API response format and SDK decoding; no unrelated modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@kkarlsen06 kkarlsen06 marked this pull request as draft February 6, 2026 04:29
@kkarlsen06 kkarlsen06 marked this pull request as ready for review February 6, 2026 04:42
@grdsdev
Copy link
Contributor

grdsdev commented Feb 6, 2026

Hey @kkarlsen06, thanks for the PR, but let's go with only the id field and remove the old factorId. I know this is theoretically a breaking change, but since factorId is simply wrong, I think that is an acceptable breaking change to have.

Can you update your PR to accept only the id field and also remove the 204 check? I don't see that check in the JS SDK, so let's try having the same behavior across libraries.

@kkarlsen06 kkarlsen06 force-pushed the fix-mfa-unenroll-response-id branch from d92f0f2 to 746a3d4 Compare February 6, 2026 12:01
@kkarlsen06
Copy link
Contributor Author

kkarlsen06 commented Feb 6, 2026

Done, @grdsdev. Simple, minimal diff, with breaking changes as per your request.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@Tests/AuthTests/AuthClientTests.swift`:
- Around line 1807-1808: The mock response in the failing test uses statusCode
204 but provides a JSON body, which contradicts HTTP semantics and breaks the
SDK's unenroll decoding (AuthMFA.unenroll calls .decoded(decoder:)); update the
mock in AuthClientTests.swift to use statusCode 200 so the mock returns a
successful JSON payload that the unenroll call can decode correctly and aligns
with the SDK contract.

@kkarlsen06 kkarlsen06 force-pushed the fix-mfa-unenroll-response-id branch from 746a3d4 to 00f1a75 Compare February 6, 2026 12:05
@grdsdev
Copy link
Contributor

grdsdev commented Feb 6, 2026

@kkarlsen06 please run swift format on the files you touched to make sure they follow formatting pattern.

@kkarlsen06 kkarlsen06 force-pushed the fix-mfa-unenroll-response-id branch from 00f1a75 to 7c555a0 Compare February 6, 2026 13:04
@grdsdev grdsdev merged commit 7f42d92 into supabase:main Feb 6, 2026
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: mfa.unenroll() throws DecodingError on successful delete due to id vs factor_id mismatch

2 participants