-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat [#143] 로그아웃 및 애플로그인 기능 구현 #145
Conversation
@@ -1524,7 +1523,6 @@ | |||
PRODUCT_BUNDLE_IDENTIFIER = "com.SOPT33.DontBe-iOS"; | |||
PRODUCT_NAME = "$(TARGET_NAME)"; | |||
PROVISIONING_PROFILE_SPECIFIER = ""; | |||
"PROVISIONING_PROFILE_SPECIFIER[sdk=iphoneos*]" = "DontBe-Release"; | |||
SUPPORTED_PLATFORMS = "iphoneos iphonesimulator"; | |||
SUPPORTS_MACCATALYST = NO; | |||
SUPPORTS_MAC_DESIGNED_FOR_IPHONE_IPAD = NO; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DontBe-iOS.entitlements 파일이 프로젝트에 추가되었고, 코드 사인 관련 설정이 변경되어 있습니다. 자동 코드 사인 스타일로 변경된 점과 개발 팀 식별자(DEVELOPMENT_TEAM)가 명시적으로 설정되었으며 수동 코드 사인 스타일 설정이 제거되었습니다. Provisioning profile specifier 관련 설정이 삭제되었습니다.
개선 제안:
- 보안상의 이유로 CODE_SIGN_ENTITLEMENTS 값을 "DontBe-iOS/DontBe-iOS.entitlements"로 설정하는 것은 적절합니다.
- DEVELOPMENT_TEAM을 비워두지 말고 명시적으로 설정해야 합니다.
- 프로비저닝 관련 설정(PROVISIONING_PROFILE_SPECIFIER)이 삭제되어 있어, 필요한 경우 해당 설정을 유효한 값으로 다시 설정해야 할 수 있습니다.
let refreshToken = KeychainWrapper.loadToken(forKey: "refreshToken") ?? "" | ||
KeychainWrapper.saveToken(refreshToken, forKey: "refreshToken") | ||
} | ||
|
||
return true | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제안 사항 및 개선점:
-
코드 중간에 있는 ASAuthorizationAppleIDProvider.credentialRevokedNotification을 처리하는 블록 내부에서 스레드 관련 문제가 발생할 수 있습니다. UI 업데이트는 메인 스레드에서 실행되어야 하며, 필요한 경우 DispatchQueue.main.async를 사용하여 해당 스레드로 이동해야 합니다.
-
AppDelegate 클래스가 담당하는 책임이 많아서 단일 책임 원칙(Single Responsibility Principle)을 준수하기 어렵습니다. 로그인 기능과 토큰 관리 등을 분리하고 적절한 모듈로 나누는 것이 좋습니다.
-
KeychainWrapper에 토큰을 저장하는 방식은 보안 취약점을 유발할 수 있습니다. 안전한 보호 도구를 사용하여 민감한 정보를 저장하고 관리해야 합니다.
-
NotificationCenter의 addObserver 부분에서의 주석 설명이 조금 더 구체적일 수 있습니다. 연결이 취소될 때 앱이 예상대로 작동하도록 설명을 명확히 하는 것이 좋습니다.
-
코드 일관성을 위해 import 문장들을 정렬하고, 코드 스타일 가이드라인을 따르도록 수정하는 것이 바람직합니다.
-
'saveUserData()'와 'loadUserData()' 함수가 어디에서 온지 명확하지 않습니다. 이러한 함수들이 어떻게 동작하고 데이터를 처리하는지 추가적인 설명이 필요합니다.
-
앱 초기화 및 로직 점검을 위한 명확한 로깅 기능을 추가하면 디버깅을 용이하게 할 수 있습니다.
위에서 언급된 지적 사항들을 고려하여 코드의 가독성과 유지보수성을 향상시킬 수 있습니다.
<string>Default</string> | ||
</array> | ||
</dict> | ||
</plist> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 코드 패치는 XML 형식으로 작성된 Plist 파일을 만드는 것 같습니다. 여러 가지 개선 제안과 버그 리스크를 살펴보겠습니다:
- 보안 측면: XML Plist 파일은 일반적으로 문자열 escape 처리가 필요하며, 사용자 입력값에 영향을 받을 수 있으므로 주의가 필요합니다.
- URL 하드코딩: Doctype 선언 부분에서
http://www.apple.com
을 직접 참조하고 있습니다. 이 부분은 HTTPS로 업데이트 하는 것이 좋습니다. - Key 값들의 통일성: 들여쓰기와 공백 등의 포매팅이 일관되지 않을 수 있습니다. 코드 포맷을 일관되게 유지하는 것이 가독성을 향상시킬 수 있습니다.
- Error Handling: 파일이나 태그 구조 오류에 대한 애플리케이션 내에서의 처리 루틴이 없는지 확인할 필요가 있습니다.
개선 제안:
- 사용자 입력값을 처리할 때 XML Escape 함수를 사용하여 데이터의 무결성 및 보안을 강화합니다.
- HTTPS를 통한 URL 참조를 사용하여 보안 문제를 방지합니다.
- 일관된 들여쓰기와 코드 스타일을 적용하여 가독성을 관리합니다.
@@ -11,4 +11,5 @@ import Foundation | |||
|
|||
struct SocialLoginRequestDTO: Encodable { | |||
let socialPlatform: String | |||
let userName: String? | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 코드의 주요 문제점은 SocialLoginRequestDTO
구조체에 새로운 프로퍼티 userName
이 옵셔널 타입으로 추가되었지만 해당 프로퍼티가 초기화될 때 값을 전달받지 않는다는 점입니다. 이 경우, userName
이 옵셔널인 이유와 언제 왜 nil이 될 수 있는지 확실하게 결정해야 합니다. 또한, 새로운 변수가 추가되었으므로 관련된 변경사항이 있다면 다른 코드에서도 업데이트되어야 할 수 있습니다.
개선 제안:
userName
프로퍼티를 초기화하는 초기화자를 추가하여 값이 필요할 때 옵셔널 특성을 명확히합니다.- 도움이 될 것으로 예상하는 주석을 추가하여 초안이나 코드 발전 방향을 설명합니다.
- 코드베이스에서
SocialLoginRequestDTO
구조체를 사용하는 모든 곳에서userName
프로퍼티의 추가를 알립니다. - 필요시 기존 코드베이스와 일관성을 유지하며 코딩 스타일을 조정합니다.
이상 입니다. 부가적인 정보가 필요하거나 질문이 있으면 언제든지 물어봐주세요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3
애플로그인 짱짱 고생하셨습니다!! 주주 폼 미쳐따-!! 로그아웃 로직도 짱입니당!!!
controller.delegate = self | ||
controller.performRequests() | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3
미쳤다..!! 애플 로그인은 이렇게 연결하는거군여..!!!! 짱이당!
let identifyToken = credential.identityToken { | ||
let userName = (fullName.familyName ?? "") + (fullName.givenName ?? "") | ||
print(userName) | ||
let accessToken = String(data: identifyToken, encoding: .utf8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3
옹.. 이게 그 애플에서 발급해준다는 토큰..?! 위 아래로 결과 출력용 print 이제 없애도 될거같아여!!
let rootViewController = LoginViewController(viewModel: LoginViewModel(networkProvider: NetworkService())) | ||
sceneDelegate.window?.rootViewController = UINavigationController(rootViewController: rootViewController) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P3
로그아웃 처리 로직 이해했습니다!!! 팝업 창 클릭 시 실행되도록 옮겨놓겠습니당!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
와우!!! 고생했습니다 👻👍
@@ -125,3 +145,39 @@ extension LoginViewModel { | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2
피드백 반영해서 요기 catch문에 nil 대신 다른 처리를 해주는 건 어떨까요 ?!
func authorizationController(controller: ASAuthorizationController, didCompleteWithError error: Error) { | ||
print(error) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 코드 패치의 리뷰를 제공해 드리겠습니다:
import AuthenticationServices
가 추가되었습니다.LoginViewModel
클래스가NSObject
를 상속받도록 변경되었습니다.Input
구조체에appleButtonTapped
가 추가되었습니다.performAppleLogin()
메서드가 추가되어 Apple 로그인을 처리합니다.handleKakaoLoginResult
메서드에서 소셜 플랫폼 종류와 사용자 이름이 추가 전달됩니다.postSocialLoginAPI
메서드 시그니처가 변경되어 Apple 로그인에 대한 처리도 가능합니다.- ASAuthorizationControllerDelegate를 확장하여 Apple 로그인이 처리됩니다.
- 에러처리 및 로그 출력이 추가되었습니다.
개선 제안:
Input
구조체에서 옵셔널 타입 (let kakaoButtonTapped: AnyPublisher<Void, Never>?
)을 사용하는 것이 좋습니다.handleKakaoLoginResult
메서드 내에 있는 서버 통신 부분은 별도의 함수로 분리하는 것이 도움이 될 수 있습니다.- 네트워크 요청 중 발생할 수 있는 다양한 오류에 대한 처리 로직을 더 강화해야 합니다.
- 코드의 가독성을 높이기 위해 주석을 보강하고, Swift 네이밍 규칙을 준수하도록 개선할 필요가 있습니다.
- 애플 로그인 후 결과를 처리하는 섹션과 관련된 로직을 더 명확하게 구조화할 수 있습니다.
👻 PULL REQUEST
💻 작업한 내용
💡 참고사항
Don-tBe-iOS/DontBe-iOS/DontBe-iOS/Presentation/Login/ViewModels/LoginViewModel.swift
Lines 149 to 183 in d74ab45
Don-tBe-iOS/DontBe-iOS/DontBe-iOS/Presentation/Login/ViewModels/LoginViewModel.swift
Lines 104 to 147 in d74ab45
📸 스크린샷
KakaoTalk_Video_2024-02-18-23-32-10.mp4
📟 관련 이슈