Add social authentication options including - Apple, Microsoft, Facebook, and Google #176
Conversation
…eios # Conflicts: # Core/Core.xcodeproj/project.pbxproj # Core/Core/Network/AuthEndpoint.swift # OpenEdX/AppDelegate.swift # Profile/Profile/Presentation/Profile/ProfileViewModel.swift
…eios # Conflicts: # Core/Core.xcodeproj/project.pbxproj
|
| import MSAL | ||
| import Swinject | ||
|
|
||
| enum Socials { |
There was a problem hiding this comment.
how about renaming it to SocialAuthProvider or SocialIdentityProvider or IdentityProvider?
There was a problem hiding this comment.
maybe SocialResult?
There was a problem hiding this comment.
SocialAuthResult would seem more appropriate, what do you think?
There was a problem hiding this comment.
also how about moving this enum out to a separate file?
| private func appleLogin(_ credentials: AppleCredentials) { | ||
| socialLogin( | ||
| externalToken: credentials.token, | ||
| backend: "apple-id", |
There was a problem hiding this comment.
how about moving the backend name to the new Socials enum?
| } | ||
| socialLogin( | ||
| externalToken: currentAccessToken, | ||
| backend: "facebook", |
There was a problem hiding this comment.
how about moving the backend name to the new Socials enum?
| private func googleLogin(_ gIDSignInResult: GIDSignInResult) { | ||
| socialLogin( | ||
| externalToken: gIDSignInResult.user.accessToken.tokenString, | ||
| backend: "google-oauth2", |
There was a problem hiding this comment.
how about moving the backend name to the new Socials enum?
| private func microsoftLogin(_ account: MSALAccount, _ token: String) { | ||
| socialLogin( | ||
| externalToken: token, | ||
| backend: "azuread-oauth2", |
There was a problem hiding this comment.
how about moving the backend name to the new Socials enum?
| LabelButton( | ||
| image: CoreAssets.iconFacebookWhite.swiftUIImage, | ||
| title: "\(title) \(AuthLocalization.facebook)", | ||
| backgroundColor: UIColor(red: 0.09, green: 0.46, blue: 0.95, alpha: 1.00).sui, |
There was a problem hiding this comment.
how about moving the color to Assets folder as we are using in the project?
| LabelButton( | ||
| image: CoreAssets.iconMicrosoftWhite.swiftUIImage, | ||
| title: "\(title) \(AuthLocalization.microsoft)", | ||
| backgroundColor: UIColor(red: 0.18, green: 0.18, blue: 0.18, alpha: 1.00).sui, |
There was a problem hiding this comment.
how about moving the color to Assets folder as we are using in the project?
Core/Core/Network/AuthEndpoint.swift
Outdated
| @@ -10,6 +10,8 @@ import Alamofire | |||
|
|
|||
| enum AuthEndpoint: EndPointType { | |||
| case getAccessToken(username: String, password: String, clientId: String, tokenType: String) | |||
|
|
|||
There was a problem hiding this comment.
please remove this extra line.
Core/Core/Network/AuthEndpoint.swift
Outdated
| "client_id": clientId, | ||
| "token_type": "jwt", | ||
| "access_token": externalToken, | ||
| "asymmetric_jwt": true |
There was a problem hiding this comment.
now that we have a token_type coming from config, i think we should that value.
OpenEdX/AppDelegate.swift
Outdated
| ApplicationDelegate.shared.application( | ||
| app, | ||
| open: url, | ||
| sourceApplication: options[UIApplication.OpenURLOptionsKey.sourceApplication] as? String, | ||
| annotation: options[UIApplication.OpenURLOptionsKey.annotation] | ||
| ) | ||
|
|
||
| if GIDSignIn.sharedInstance.handle(url) { | ||
| return true | ||
| } | ||
|
|
||
| if MSALPublicClientApplication.handleMSALResponse( | ||
| url, | ||
| sourceApplication: options[UIApplication.OpenURLOptionsKey.sourceApplication] as? String | ||
| ) { | ||
| return true | ||
| } |
There was a problem hiding this comment.
i think we should add check with the config, if a social auth is enabled, then handle the url, e.g.
if config.googleConfig.enabled {
GIDSignIn.sharedInstance.handle(url)
}
added SocialSignView_Previews |
|
@eyatsenkoperpetio i see in config we have how about remove Also can you update |
| @@ -145,6 +149,7 @@ | |||
| 071009CC28D1E24000344290 /* Presentation */ = { | |||
| isa = PBXGroup; | |||
| children = ( | |||
| BA8B3A302AD5485100D25EF5 /* SocialSign */, | |||
There was a problem hiding this comment.
How about renaming it to SocialAuth because it's being used for both SignIn and Register? Thoughts?
| import SwiftUI | ||
| import Core | ||
|
|
||
| struct SocialSignView: View { |
There was a problem hiding this comment.
How about renaming it to SocialAuthView because it's being used for both SignIn and Register? Thoughts?
| } | ||
| } | ||
|
|
||
| final public class SocialSignViewModel: ObservableObject { |
There was a problem hiding this comment.
How about renaming it to SocialAuthViewModel because it's being used for both SignIn and Register? Thoughts?
| @@ -8,6 +8,7 @@ | |||
| import Foundation | |||
|
|
|||
| public enum LoginMethod: String { | |||
| case apple = "Apple" | |||
There was a problem hiding this comment.
The enum name is LoginMethod; I guess in this case, the methods are Password and SocialAuth. How about keeping two values and passing the social auth provider(Apple, Google, Facebook, etc) as a parameter to social auth?
| } | ||
|
|
||
| @MainActor | ||
| func sign(with result: Result<SocialResult, Error>) { |
There was a problem hiding this comment.
Can you please give it a descriptive name? it's hard to understand the functionality of the function by looking at the name.
Core/Core/Network/AuthEndpoint.swift
Outdated
| case .socialLogin: | ||
| return .post |
There was a problem hiding this comment.
How about using an OR case condition with getAccessToken?
|
|
||
| import SwiftUI | ||
|
|
||
| public struct LabelButton: View { |
There was a problem hiding this comment.
Can you rename it to SocailAuthButton because we can't say we can generalize this button style for other parts of the app?
| "GOOGLE": [ | ||
| "ENABLED": true, | ||
| "CLIENT_ID": "clientId" | ||
| ], | ||
| "FACEBOOK": [ | ||
| "ENABLED": true, | ||
| "FACEBOOK_APP_ID": "facebookAppId", | ||
| "CLIENT_TOKEN": "client_token" | ||
| ], | ||
| "MICROSOFT": [ | ||
| "ENABLED": true, | ||
| "APP_ID": "appId" | ||
| ], | ||
| "APPLE_SIGNIN": [ | ||
| "ENABLED": true | ||
| ] |
There was a problem hiding this comment.
Can you please add tests for these values as well?
OpenEdX/AppDelegate.swift
Outdated
| FirebaseApp.configure(options: configuration) | ||
| Crashlytics.crashlytics().setCrashlyticsCollectionEnabled(true) | ||
| if let config = Container.shared.resolve(ConfigProtocol.self) { | ||
|
|
| let familyName = credentials.fullName?.familyName ?? storage?.appleSignFamilyName ?? "" | ||
|
|
||
| let email = credentials.email ?? storage?.appleSignEmail ?? "" | ||
| var name: String = "\(givenName) \(familyName)" |
There was a problem hiding this comment.
Can we just save the formatted name instead of saving separately saving family and given name?
|
@eyatsenkoperpetio Tests are also failing. Please take a look into it. And the email is empty in case of Register with Apple on subsequent request if account not registered on first try. |
Authorization/Authorization/Presentation/SocialSign/SocialSignViewModel.swift
Outdated
Show resolved
Hide resolved
Authorization/Authorization/Presentation/SocialSign/SocialSignViewModel.swift
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @MainActor | ||
| func sign(with result: Result<SocialAuthDetails, Error>) { |
There was a problem hiding this comment.
How about renaming it to login(with .....)
| } | ||
|
|
||
| @MainActor | ||
| private func social(result: SocialAuthDetails) { |
There was a problem hiding this comment.
How about renaming it to socialAuth(result....)
| @StateObject var viewModel: SocialAuthViewModel | ||
|
|
||
| init( | ||
| signType: SocialAuthType = .signIn, |
There was a problem hiding this comment.
How about renaming it to authType?
| case apple(AppleCredentials) | ||
| case facebook(LoginManagerLoginResult) | ||
| case google(GIDSignInResult) | ||
| case microsoft(MSALAccount, String) |
There was a problem hiding this comment.
Can you please create a new struct like below and use it for all social auth providers? It will generalize the code for all social auth providers.
struct SocialAuthResponse {
var name: String
var email: String
var token: String
}
like
case apple(SocialAuthResponse)
case facebook(SocialAuthResponse)
..
|
|
||
| "FORGOT.TITLE"= "Forgot password"; | ||
| "FORGOT.DESCRIPTION" = "Please enter your log-in or recovery email address below and we will send you an email with instructions."; | ||
| "FORGOT.REQUEST" = "Reset password"; | ||
| "FORGOT.CHECK_TITLE" = "Check your email"; | ||
| "FORGOT.CHECK_Description" = "We have sent a password recover instructions to your email "; | ||
|
|
||
|
|
||
| "SIGN_IN_WITH" = "Sign in with"; | ||
| "SIGN_IN_REGISTER" = "Register with"; |
There was a problem hiding this comment.
How about renaming it to REGISTER_WITH?
| "ERROR.INVALID_EMAIL_ADDRESS_OR_USERNAME" = "Invalid email or username"; | ||
| "ERROR.INVALID_PASSWORD_LENGTH" = "Invalid password length"; | ||
| "ERROR.ACCOUNT_DISABLED" = "Your account is disabled. Please contact customer support for assistance."; |
There was a problem hiding this comment.
Although it's fine but ERROR.DISABLED_ACCOUNT sounds good.
Core/Core/Network/AuthEndpoint.swift
Outdated
| @@ -10,6 +10,7 @@ import Alamofire | |||
|
|
|||
| enum AuthEndpoint: EndPointType { | |||
| case getAccessToken(username: String, password: String, clientId: String, tokenType: String) | |||
| case getEchangeAccessToken(externalToken: String, backend: String, clientId: String, tokenType: String) | |||
There was a problem hiding this comment.
How about renaming it to exchangeAccessToken?
| storage?.appleSignFullName = name | ||
| } | ||
|
|
||
| if storage?.appleSignEmail == nil { |
There was a problem hiding this comment.
The value of storage?.appleSignEmail is an empty string so the email is not being saved into storage for later ussage.
| @@ -0,0 +1,63 @@ | |||
| // | |||
| // FacebookSingInProvider.swift | |||
There was a problem hiding this comment.
Can you please change these names as well after renaming the files? You might need to make this change in all the relevant files like SocialAuthView, SocialAuthViewModel etc.
| @@ -0,0 +1,74 @@ | |||
| // | |||
| // LabelButton.swift | |||
There was a problem hiding this comment.
Please change this as well
Add social authentication options including - Apple, Microsoft, Facebook, and Google
in config you need add:
SOCIAL_LOGINS:
ENABLED: true
APPLE_SIGNIN_ENABLED: true
MICROSOFT:
ENABLED: true
APP_ID: 'id'
GOOGLE:
ENABLED: true
GOOGLE_PLUS_KEY: 'id.apps.googleusercontent.com'
CLIENT_ID: 'id.apps.googleusercontent.com'
FACEBOOK:
ENABLED: true
FACEBOOK_APP_ID: 'id'
CLIENT_TOKEN: 'id'