feat: add nonce support for OpenID Connect in login #429
Conversation
WalkthroughAdds optional nonce support for OpenID Connect to the login flows across JS, web, Android, and iOS. Login APIs now accept an optional nonce and native layers choose KakaoTalk or KakaoAccount based on nonce presence; Android build adds the Kakao auth module dependency. Changes
sequenceDiagram
participant JS as React Native JS
participant Bridge as Native Bridge (iOS/Android)
participant SDK as Kakao SDK
JS->>Bridge: login(nonce?)
alt nonce is nil/empty AND KakaoTalk available
Bridge->>SDK: loginWithKakaoTalk()
else nonce provided OR KakaoTalk unavailable
Bridge->>SDK: loginWithKakaoAccount(nonce)
end
SDK-->>Bridge: token (accessToken, refreshToken, idToken?, scopes, expires)
Bridge-->>JS: resolve(tokenMap)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/index.web.ts (1)
🔇 Additional comments (1)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
android/src/main/java/com/dooboolab/kakaologins/RNKakaoLoginsModule.kt (2)
143-148: Redundant null check after early return.The
if (token != null)check on line 148 is unnecessary. Line 143-145 already returns early whentoken == null, so at line 148,tokenis guaranteed to be non-null.Apply this diff to remove redundant check:
if (token == null) { promise.reject("RNKakaoLogins", "Token is null") return@loginWithKakaoAccount } - if (token != null) { - val (accessToken, accessTokenExpiresAt, refreshToken, refreshTokenExpiresAt, idToken, scopes) = token + val (accessToken, accessTokenExpiresAt, refreshToken, refreshTokenExpiresAt, idToken, scopes) = token // ... rest of token mapping - }
37-52: Consider extracting duplicated token-mapping logic into a helper function.The token-to-map conversion logic is repeated four times across the file with identical structure. This violates DRY and increases maintenance burden.
Extract a helper function:
private fun createTokenMap(token: OAuthToken): WritableMap { val (accessToken, accessTokenExpiresAt, refreshToken, refreshTokenExpiresAt, idToken, scopes) = token val map = Arguments.createMap() map.putString("accessToken", accessToken) map.putString("refreshToken", refreshToken) map.putString("idToken", idToken) map.putString("accessTokenExpiresAt", dateFormat(accessTokenExpiresAt)) map.putString("refreshTokenExpiresAt", dateFormat(refreshTokenExpiresAt)) val scopeArray = Arguments.createArray() scopes?.forEach { scopeArray.pushString(it) } map.putArray("scopes", scopeArray) return map }Then replace each occurrence with
promise.resolve(createTokenMap(token)).Also applies to: 70-85, 117-131, 148-164
ios/RNKakaoLogins/RNKakaoLogins.swift (2)
49-92: Nonce-driven behavior inlogindiffers from Android and could use small robustness/style tweaks
With this change, on iOS
login(nonce: ...)will skip KakaoTalk and always go throughloginWithKakaoAccount, even if KakaoTalk is available. The Android implementation described in the PR context still prefers KakaoTalk when available and only uses the account path (and thus the nonce) in specific cases. Please confirm whether this cross‑platform divergence is intentional; if the goal is “OIDC with nonce → always Kakao Account”, Android’sloginpath should likely be aligned, or the behavior clearly documented as iOS‑only.Both branches force‑unwrap
oauthToken!forexpiredAt/refreshTokenExpiredAt. This relies on the Kakao SDK invariant thatoauthTokenis never nil whenerroris nil. To avoid a hard crash if that ever changes (or if the SDK returns an unexpected state), consider a defensive pattern like:guard let token = oauthToken else { reject("RNKakaoLogins", "OAuthToken was nil on successful login", nil) return }and then use
tokeninstead ofoauthToken!in the mapping.Minor SwiftLint/style nits (non‑functional but easy wins):
- The explicit
-> Voidreturn type is redundant in Swift for this signature.- The
dateFormatassignment line still has a trailing semicolon while the newloginWithKakaoAccountimplementation below omits it; you may want to normalize these for consistency.
96-121: Factor out shared Kakao Account login logic and harden token handling
- This method correctly exposes a “login with Kakao Account (with optional nonce)” path, but its body is effectively the same as the
loginmethod’s Kakao Account branch. To avoid divergence and make future changes (e.g., adding fields, changing date format) safer, consider extracting the commonloginWithKakaoAccount+ token‑mapping logic into a private helper that bothloginandloginWithKakaoAccountcall.- As with
login, you’re force‑unwrappingoauthToken!to access date fields. Using aguard let token = oauthToken else { ... }pattern would make this resilient to any future SDK behavior changes whereerroris nil butoauthTokenhappens to be nil.- Minor style cleanups:
- The explicit
-> Voidis redundant in Swift and flagged by SwiftLint.- Once you centralize token mapping, you can also centralize
DateFormattercreation (or cache a formatter) to avoid repeated instantiation and keep formatting consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
android/build.gradle(1 hunks)android/src/main/java/com/dooboolab/kakaologins/RNKakaoLoginsModule.kt(2 hunks)ios/RNKakaoLogins/RNKakaoLogins.m(1 hunks)ios/RNKakaoLogins/RNKakaoLogins.swift(3 hunks)src/index.ts(1 hunks)src/types/index.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/index.ts (1)
src/types/index.ts (3)
KakaoLoginModuleInterface(1-23)KakaoLoginOptions(25-27)KaKaoLoginWebType(81-85)
ios/RNKakaoLogins/RNKakaoLogins.swift (3)
android/src/main/java/com/dooboolab/kakaologins/RNKakaoLoginsModule.kt (2)
login(23-93)loginWithKakaoAccount(95-167)src/index.ts (4)
login(7-15)login(43-43)loginWithKakaoAccount(16-22)loginWithKakaoAccount(44-44)src/index.web.ts (4)
login(4-35)login(83-83)loginWithKakaoAccount(78-80)loginWithKakaoAccount(84-84)
🪛 GitHub Actions: CI
src/types/index.ts
[error] 25-25: prettier/prettier: Insert ·
[error] 27-27: prettier/prettier: Insert ;
src/index.ts
[error] 2-2: prettier/prettier: Replace KakaoLoginModuleInterface, KakaoLoginOptions, KaKaoLoginWebType with KakaoLoginModuleInterface, KakaoLoginOptions, KaKaoLoginWebType
[error] 11-11: prettier/prettier: Delete ····
[error] 13-13: prettier/prettier: Delete ····
[error] 20-20: prettier/prettier: Delete ····
🪛 SwiftLint (0.57.0)
ios/RNKakaoLogins/RNKakaoLogins.swift
[Warning] 52-52: Returning Void in a function declaration is redundant
(redundant_void_return)
[Warning] 55-55: Lines should not have trailing semicolons
(trailing_semicolon)
[Warning] 99-99: Returning Void in a function declaration is redundant
(redundant_void_return)
🔇 Additional comments (3)
android/build.gradle (1)
56-56: LGTM!The new
v2-authdependency is appropriately added with the same version as other Kakao SDK modules, ensuring version consistency. The comment documenting the purpose (OAuthTokenRequest for OIDC) is helpful.src/types/index.ts (1)
1-4: LGTM!The new overloads for
loginandloginWithKakaoAccountacceptingKakaoLoginOptionscorrectly extend the interface while preserving backward compatibility with existing signatures.Also applies to: 17-18
ios/RNKakaoLogins/RNKakaoLogins.m (1)
7-8: LGTM!The Objective-C bridge method declarations correctly add the nullable
nonceparameter to bothloginandloginWithKakaoAccountmethods, maintaining consistency with the Swift implementation.
| if (error is AuthError && error.statusCode == 302) { | ||
| this.loginWithKakaoAccount(promise) | ||
| this.loginWithKakaoAccount(null, promise) | ||
| return@loginWithKakaoTalk |
There was a problem hiding this comment.
Nonce is discarded on 302 fallback - user's nonce parameter lost.
When KakaoTalk login fails with status 302, the code falls back to loginWithKakaoAccount(null, promise), discarding the user-provided nonce. This breaks OIDC nonce support when KakaoTalk is installed but returns 302.
Apply this diff to preserve the nonce:
if (error is AuthError && error.statusCode == 302) {
- this.loginWithKakaoAccount(null, promise)
+ this.loginWithKakaoAccount(nonce, promise)
return@loginWithKakaoTalk
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (error is AuthError && error.statusCode == 302) { | |
| this.loginWithKakaoAccount(promise) | |
| this.loginWithKakaoAccount(null, promise) | |
| return@loginWithKakaoTalk | |
| if (error is AuthError && error.statusCode == 302) { | |
| this.loginWithKakaoAccount(nonce, promise) | |
| return@loginWithKakaoTalk |
🤖 Prompt for AI Agents
In android/src/main/java/com/dooboolab/kakaologins/RNKakaoLoginsModule.kt around
lines 29 to 31, the fallback call on AuthError with statusCode 302 discards the
user-provided nonce by calling loginWithKakaoAccount(null, promise); change the
call to forward the original nonce instead of null so the OIDC nonce is
preserved during the KakaoTalk→account fallback (i.e., capture the nonce
parameter in the outer scope and pass it into loginWithKakaoAccount when falling
back).
| login(props?: KakaoLoginOptions | KaKaoLoginWebType) { | ||
| if (!props) { | ||
| return RNKakaoLogins.login(null); | ||
| } | ||
|
|
||
| const kakaoLoginOptions = props as KakaoLoginOptions; | ||
|
|
||
| return RNKakaoLogins.login(kakaoLoginOptions.nonce || null); | ||
| }, |
There was a problem hiding this comment.
Web login flow appears broken - props type discrimination missing.
When props is of type KaKaoLoginWebType (for web login), the code casts it to KakaoLoginOptions and passes nonce || null to the native module. This breaks the web login flow since KaKaoLoginWebType properties (restApiKeyWeb, redirectUrlWeb, codeWeb) are never used.
The function needs to discriminate between the two prop types and handle each appropriately.
Consider implementing type discrimination:
login(props?: KakaoLoginOptions | KaKaoLoginWebType) {
if (!props) {
return RNKakaoLogins.login(null);
}
-
- const kakaoLoginOptions = props as KakaoLoginOptions;
-
- return RNKakaoLogins.login(kakaoLoginOptions.nonce || null);
+
+ // Discriminate between web login and native login with nonce
+ if ('restApiKeyWeb' in props || 'redirectUrlWeb' in props || 'codeWeb' in props) {
+ // Handle web login flow (existing behavior)
+ // TODO: Implement web login handling
+ throw new Error('Web login not yet implemented in this refactor');
+ }
+
+ return RNKakaoLogins.login(props.nonce || null);
},Alternatively, verify how web login was handled before this change and ensure that flow is preserved.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| login(props?: KakaoLoginOptions | KaKaoLoginWebType) { | |
| if (!props) { | |
| return RNKakaoLogins.login(null); | |
| } | |
| const kakaoLoginOptions = props as KakaoLoginOptions; | |
| return RNKakaoLogins.login(kakaoLoginOptions.nonce || null); | |
| }, | |
| login(props?: KakaoLoginOptions | KaKaoLoginWebType) { | |
| if (!props) { | |
| return RNKakaoLogins.login(null); | |
| } | |
| // Discriminate between web login and native login with nonce | |
| if ('restApiKeyWeb' in props || 'redirectUrlWeb' in props || 'codeWeb' in props) { | |
| // Handle web login flow (existing behavior) | |
| // TODO: Implement web login handling | |
| throw new Error('Web login not yet implemented in this refactor'); | |
| } | |
| return RNKakaoLogins.login(props.nonce || null); | |
| }, |
🧰 Tools
🪛 GitHub Actions: CI
[error] 11-11: prettier/prettier: Delete ····
[error] 13-13: prettier/prettier: Delete ····
🤖 Prompt for AI Agents
In src/index.ts around lines 7-15, the login function incorrectly casts props to
KakaoLoginOptions and always passes nonce to RNKakaoLogins.login, breaking the
web flow; change the implementation to discriminate the union type (e.g., check
for a web-specific property like codeWeb or restApiKeyWeb) and when those exist
call RNKakaoLogins.login with the web payload (restApiKeyWeb, redirectUrlWeb,
codeWeb), otherwise treat props as KakaoLoginOptions and pass props.nonce ||
null; ensure you still handle the no-props case by calling
RNKakaoLogins.login(null).
1 participant
#428 이슈
ios 시뮬레이터, 안드로이드 실제 기기와 연동하여 테스트 완료했습니다 !
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.