search

AzureAD / microsoft-authentication-library-for-objc

Microsoft Authentication Library (MSAL) for iOS and macOS
http://aka.ms/aadv2
MIT License
262 stars 142 forks source link

[iOS SDK] Update current user account result with latest account and id token after refresh #2346

Closed spetrescu84 closed 1 week ago

spetrescu84 commented 3 weeks ago

Proposed changes

Update current user account result with latest account and id token after refresh

Type of change

Risk

Additional information

ameyapat commented 3 weeks ago

Cloned PR for spetrescu/update-idtoken-account

AI description (iteration 1)

PR Classification

Bug fix

PR Summary

This pull request addresses a bug related to the handling of ID tokens and account information in the MSALNativeAuthUserAccountResult class.

ameyapat commented 3 weeks ago

"AI code review (iteration 1) There is no AI feedback on this pull request.

AI-generated content may be incorrect "

ameyapat commented 3 weeks ago

"Pull request description has been updated with summary.

AI-generated content may be incorrect

ameyapat commented 2 weeks ago

Cloned PR for spetrescu/update-idtoken-account

AI description (iteration 1)

PR Classification

New feature

PR Summary

This pull request introduces new functionality for handling silent token acquisition in the MSAL library, along with corresponding tests.

ameyapat commented 2 weeks ago

Cloned PR for spetrescu/update-idtoken-account

AI description (iteration 1)

PR Classification

New feature

PR Summary

This pull request introduces new functionality for handling silent token acquisition and related tests.

ameyapat commented 2 weeks ago

"Added 4 Inline Code Review comment(s) at the following location(s):

AI-generated content may be incorrect "

ameyapat commented 2 weeks ago

"AI code review (iteration 1) It might be better to handle the potential error from the MSALNativeAuthPublicClientApplication initialization more explicitly, rather than using try?, to ensure that any initialization issues are properly logged or handled. This can help in debugging and maintaining the code.

Here is the suggested code:

init(
    configuration config: MSALPublicClientApplicationConfig,
    challengeTypes: MSALNativeAuthChallengeTypes) throws {
        do {
            self.application = try MSALNativeAuthPublicClientApplication(configuration: config, challengeTypes: challengeTypes)
        } catch {
            // Handle the error appropriately, e.g., logging the error
            print(\"Failed to initialize MSALNativeAuthPublicClientApplication: \(error)\")
            throw error
        }
    }

This comment refers to line 29 to 33 in the new file. AI-generated content may be incorrect

ameyapat commented 2 weeks ago

"AI code review (iteration 1) It might be better to avoid using try? for error handling in the makeSilentTokenProvider method, as it can silently ignore errors. Instead, you can use try and handle the error explicitly to provide better error reporting and debugging.

Here is the suggested code:

class MSALNativeAuthSilentTokenProviderFactory: MSALNativeAuthSilentTokenProviderBuildable {
    func makeSilentTokenProvider(configuration: MSALPublicClientApplicationConfig,
                                 challengeTypes: MSALNativeAuthChallengeTypes) throws -> MSALNativeAuthSilentTokenProviding? {
        do {
            return try MSALNativeAuthSilentTokenProvider(configuration: configuration, challengeTypes: challengeTypes)
        } catch {
            // Handle the error appropriately, e.g., log it or rethrow
            throw error
        }
    }
}

This comment refers to line 26 to 29 in the new file. AI-generated content may be incorrect

ameyapat commented 2 weeks ago

"AI code review (iteration 1) It might be better to handle the case where result and error are both nil in the completion handler of silentTokenProvider.acquireTokenSilent. This ensures that unexpected states are properly managed and logged.

Here is the suggested code:

silentTokenProvider.acquireTokenSilent(parameters: params) { [weak self] result, error in
    guard let self = self else { return }

    if let error = error as? NSError {
        let accessTokenError = self.createRetrieveAccessTokenError(error: error, context: context)
        Task { await delegate.onAccessTokenRetrieveError(error: accessTokenError) }
        return
    }

    if let result = result {
        let delegateDispatcher = CredentialsDelegateDispatcher(delegate: delegate, telemetryUpdate: nil)
        self.rawIdToken = result.rawIdToken
        self.account = result.account
        Task { await delegateDispatcher.dispatchAccessTokenRetrieveCompleted(result: result.accessTokenResult,
                                                                             correlationId: result.correlationId) }
        return
    }

    // Handle unexpected nil result and error
    MSALLogger.log(
                    level: .error,
                    context: context,
                    format: \"Both result and error are nil in acquireTokenSilent completion handler.\"
                )
    Task {
        await delegate.onAccessTokenRetrieveError(error: RetrieveAccessTokenError(
            type: .generalError,
            correlationId: correlationId ?? context.correlationId())
        )
    }
}

This comment refers to line 58 to 75 in the new file. AI-generated content may be incorrect

ameyapat commented 2 weeks ago

"AI code review (iteration 1) It might be better to handle the case where result.accessTokenResult is nil before calling dispatchAccessTokenRetrieveCompleted. This ensures that the delegate is not called with an invalid result, which could lead to unexpected behavior.

Here is the suggested code:

if let result = result {
    let delegateDispatcher = CredentialsDelegateDispatcher(delegate: delegate, telemetryUpdate: nil)
    self.rawIdToken = result.rawIdToken
    self.account = result.account
    if let accessTokenResult = result.accessTokenResult {
        Task { await delegateDispatcher.dispatchAccessTokenRetrieveCompleted(result: accessTokenResult, correlationId: result.correlationId) }
    } else {
        Task { await delegate.onAccessTokenRetrieveError(error: RetrieveAccessTokenError(type: .generalError, correlationId: correlationId ?? context.correlationId())) }
    }
    return
}

This comment refers to line 67 to 72 in the new file. AI-generated content may be incorrect

ameyapat commented 2 weeks ago

Cloned PR for spetrescu/update-idtoken-account

AI description (iteration 1)

PR Classification

New feature

PR Summary

This pull request introduces new functionality for handling silent token acquisition and related tests.

ameyapat commented 2 weeks ago

"Added 4 Inline Code Review comment(s) at the following location(s):

AI-generated content may be incorrect "

ameyapat commented 2 weeks ago

"AI code review (iteration 1) It might be better to handle the case where the application initialization fails in the initializer. This will ensure that the application property is not nil when acquireTokenSilent is called, which can prevent potential runtime errors.

Here is the suggested code:

class MSALNativeAuthSilentTokenProvider: MSALNativeAuthSilentTokenProviding {

    private let application: MSALNativeAuthPublicClientApplication

    init(
        configuration config: MSALPublicClientApplicationConfig,
        challengeTypes: MSALNativeAuthChallengeTypes) throws {
            guard let app = try? MSALNativeAuthPublicClientApplication(configuration: config, challengeTypes: challengeTypes) else {
                throw NSError(domain: \"MSALNativeAuthSilentTokenProvider\", code: -1, userInfo: [NSLocalizedDescriptionKey: \"Failed to initialize MSALNativeAuthPublicClientApplication\"])
            }
            self.application = app
        }

    func acquireTokenSilent(parameters: MSALSilentTokenParameters,
                            completionBlock: @escaping MSALNativeAuthSilentTokenResponse) {
        application.acquireTokenSilent(with: parameters) { result, error in
            if let result {
                let silentTokenResult = MSALNativeAuthSilentTokenResult(result: result)
                completionBlock(silentTokenResult, error)
            } else {
                completionBlock(nil, error)
            }
        }
    }
}

This comment refers to lines 25 to 46 in the new file. AI-generated content may be incorrect

ameyapat commented 2 weeks ago

"AI code review (iteration 1) It might be better to make the makeSilentTokenProvider method return a non-optional MSALNativeAuthSilentTokenProviding to avoid potential issues with handling nil values. If returning nil is necessary, consider using a Result type to handle success and failure cases more explicitly.

Here is the suggested code:

protocol MSALNativeAuthSilentTokenProviderBuildable {
    func makeSilentTokenProvider(configuration: MSALPublicClientApplicationConfig,
                                 challengeTypes: MSALNativeAuthChallengeTypes) throws -> MSALNativeAuthSilentTokenProviding
}

This comment refers to lines 25 to 28 in the new file. AI-generated content may be incorrect

ameyapat commented 2 weeks ago

"AI code review (iteration 1) It might be better to avoid using try? when calling the MSALNativeAuthSilentTokenProvider initializer, as it can silently ignore errors. Instead, you can use try and handle the error explicitly to provide better error handling and debugging information.

Here is the suggested code:

    func makeSilentTokenProvider(configuration: MSALPublicClientApplicationConfig,
                                 challengeTypes: MSALNativeAuthChallengeTypes) throws -> MSALNativeAuthSilentTokenProviding? {
        do {
            return try MSALNativeAuthSilentTokenProvider(configuration: configuration, challengeTypes: challengeTypes)
        } catch {
            // Handle the error appropriately, e.g., log it or rethrow
            throw error
        }
    }

This comment refers to line 26 to 29 in the new file. AI-generated content may be incorrect

ameyapat commented 2 weeks ago

"AI code review (iteration 1) It might be better to handle the case where silentTokenProviderFactory.makeSilentTokenProvider throws an error more explicitly, rather than using try?. This can help in debugging and understanding the exact reason for failure. You can use a do-catch block for this purpose.

Here is the suggested code:

do {
    let silentTokenProvider = try silentTokenProviderFactory.makeSilentTokenProvider(configuration: config, challengeTypes: challengeTypes)
    silentTokenProvider.acquireTokenSilent(parameters: params) { [weak self] result, error in
        guard let self = self else { return }

        if let error = error as? NSError {
            let accessTokenError = self.createRetrieveAccessTokenError(error: error, context: context)
            Task { await delegate.onAccessTokenRetrieveError(error: accessTokenError) }
            return
        }

        if let result = result {
            let delegateDispatcher = CredentialsDelegateDispatcher(delegate: delegate, telemetryUpdate: nil)
            self.rawIdToken = result.rawIdToken
            self.account = result.account
            Task { await delegateDispatcher.dispatchAccessTokenRetrieveCompleted(result: result.accessTokenResult,
                                                                                 correlationId: result.correlationId) }
            return
        }

        Task {
            await delegate.onAccessTokenRetrieveError(error: RetrieveAccessTokenError(
                type: .generalError,
                correlationId: correlationId ?? context.correlationId())
            )
        }
    }
} catch {
    MSALLogger.log(
                    level: .error,
                    context: context,
                    format: \"Failed to create silentTokenProvider: \(error.localizedDescription)\"
                )
    Task { await delegate.onAccessTokenRetrieveError(
        error: RetrieveAccessTokenError(type: .generalError,
                                        correlationId: correlationId ?? context.correlationId())) }
}

This comment refers to line 43 to 49 in the new file. AI-generated content may be incorrect

ameyapat commented 2 weeks ago

Cloned PR for spetrescu/update-idtoken-account

AI description (iteration 1)

PR Classification

New feature

PR Summary

This pull request introduces new functionality for handling silent token acquisition and related tests.

ameyapat commented 2 weeks ago

"Added 3 Inline Code Review comment(s) at the following location(s):

AI-generated content may be incorrect "

ameyapat commented 2 weeks ago

"AI code review (iteration 1) It might be better to handle the case where the application initialization fails in the initializer. This will ensure that the acquireTokenSilent method does not attempt to use a nil application, which could lead to unexpected behavior or crashes.

Here is the suggested code:

    init(
        configuration config: MSALPublicClientApplicationConfig,
        challengeTypes: MSALNativeAuthChallengeTypes) throws {
            guard let app = try? MSALNativeAuthPublicClientApplication(configuration: config, challengeTypes: challengeTypes) else {
                throw NSError(domain: \"MSALNativeAuthSilentTokenProvider\", code: -1, userInfo: [NSLocalizedDescriptionKey: \"Failed to initialize MSALNativeAuthPublicClientApplication\"])
            }
            self.application = app
        }

This comment refers to lines 29 to 33 in the new file. AI-generated content may be incorrect

ameyapat commented 2 weeks ago

"AI code review (iteration 1) It might be better to avoid using try? for error handling in the makeSilentTokenProvider method, as it can silently ignore errors. Instead, you can use try and handle the error explicitly to provide better error reporting and debugging.

Here is the suggested code:

    func makeSilentTokenProvider(configuration: MSALPublicClientApplicationConfig,
                                 challengeTypes: MSALNativeAuthChallengeTypes) throws -> MSALNativeAuthSilentTokenProviding? {
        do {
            return try MSALNativeAuthSilentTokenProvider(configuration: configuration, challengeTypes: challengeTypes)
        } catch {
            // Handle the error appropriately, e.g., log it or rethrow
            throw error
        }
    }

This comment refers to line 26 to 29 in the new file. AI-generated content may be incorrect

ameyapat commented 2 weeks ago

"AI code review (iteration 1) It might be better to handle the case where result.rawIdToken or result.account is nil before assigning them to self.rawIdToken and self.account. This ensures that the properties are not set to invalid values.

Here is the suggested code:

if let rawIdToken = result.rawIdToken, let account = result.account {
    self.rawIdToken = rawIdToken
    self.account = account
    Task { await delegateDispatcher.dispatchAccessTokenRetrieveCompleted(result: result.accessTokenResult,
                                                                         correlationId: result.correlationId) }
    return
} else {
    Task {
        await delegate.onAccessTokenRetrieveError(error: RetrieveAccessTokenError(
            type: .generalError,
            correlationId: correlationId ?? context.correlationId())
        )
    }
}

This comment refers to lines 69 to 72 in the new file. AI-generated content may be incorrect

ameyapat commented 2 weeks ago

Cloned PR for spetrescu/update-idtoken-account

AI description (iteration 1)

PR Classification

New feature

PR Summary

This pull request introduces new functionality for handling silent token acquisition and related tests.

ameyapat commented 1 week ago

Cloned PR for spetrescu/update-idtoken-account

AI description (iteration 1)

PR Classification

New feature

PR Summary

This pull request introduces new functionality for handling silent token acquisition and updates related tests.

ameyapat commented 1 week ago

"Added 3 Inline Code Review comment(s) at the following location(s):

AI-generated content may be incorrect "

ameyapat commented 1 week ago

"AI code review (iteration 1) In the acquireTokenSilent function, the use of optional chaining (application?.acquireTokenSilent) combined with the try? in the initializer could lead to situations where errors are silently ignored, making debugging more difficult. It might be better to handle potential initialization failures explicitly and propagate errors, allowing for more controlled error handling and better observability of the application state when issues occur. Here is the suggested code:

init(
    configuration config: MSALPublicClientApplicationConfig,
    challengeTypes: MSALNativeAuthChallengeTypes) throws {
        guard let app = try MSALNativeAuthPublicClientApplication(configuration: config, challengeTypes: challengeTypes) else {
            throw NSError(domain: \"MSALInitializationError\", code: 1001, userInfo: nil)
        }
        self.application = app
    }

func acquireTokenSilent(parameters: MSALSilentTokenParameters,
                        completionBlock: @escaping MSALNativeAuthSilentTokenResponse) {
    guard let app = application else {
        completionBlock(nil, NSError(domain: \"MSALApplicationError\", code: 1002, userInfo: nil))
        return
    }
    app.acquireTokenSilent(with: parameters) { result, error in
        if let result = result {
            let silentTokenResult = MSALNativeAuthSilentTokenResult(result: result)
            completionBlock(silentTokenResult, error)
        } else {
            completionBlock(nil, error)
        }
    }
}

This comment refers to line 28 to 46 in the new file. AI-generated content may be incorrect

ameyapat commented 1 week ago

"AI code review (iteration 1) It might be better to ensure that the MSALAccount object in the MSALNativeAuthSilentTokenResult class is safely unwrapped before use, especially since it is being directly assigned from the result.account which could potentially be nil. This would prevent runtime crashes due to unwrapping a nil value. You could use optional binding to safely unwrap this value.

Here is the suggested code:

init(result: MSALResult) {
    self.rawIdToken = result.idToken
    if let account = result.account {
        self.account = account
    } else {
        // Handle the nil case appropriately, perhaps initializing with a default value or throwing an error
    }
    self.accessTokenResult = MSALNativeAuthTokenResult(accessToken: result.accessToken,
                                                       scopes: result.scopes,
                                                       expiresOn: result.expiresOn)
    self.correlationId = result.correlationId
}

This comment refers to line 34 to 39 in the new file. AI-generated content may be incorrect

ameyapat commented 1 week ago

"AI code review (iteration 1) It might be better to handle the potential nil return from the makeSilentTokenProvider method more explicitly. Currently, the method uses try? which could silently ignore errors thrown by the MSALNativeAuthSilentTokenProvider initializer. Instead, you could modify the method to either propagate the error or handle it within the method, depending on the desired error handling strategy. This change would improve error visibility and make the control flow clearer.

Here is the suggested code:

    func makeSilentTokenProvider(configuration: MSALPublicClientApplicationConfig,
                                 challengeTypes: MSALNativeAuthChallengeTypes) throws -> MSALNativeAuthSilentTokenProviding? {
        do {
            return try MSALNativeAuthSilentTokenProvider(configuration: configuration, challengeTypes: challengeTypes)
        } catch {
            // Handle error or propagate it
            throw error
        }
    }

This comment refers to line 26 to 29 in the new file. AI-generated content may be incorrect

ameyapat commented 1 week ago

Cloned PR for spetrescu/update-idtoken-account

AI description (iteration 1)

PR Classification

New feature

PR Summary

This pull request introduces new functionality for handling silent token acquisition and updates related tests.

ameyapat commented 1 week ago

"Added 3 Inline Code Review comment(s) at the following location(s):

AI-generated content may be incorrect "

ameyapat commented 1 week ago

"AI code review (iteration 1) In the acquireTokenSilent function, the use of optional chaining (application?.acquireTokenSilent) combined with the try? in the initializer could lead to situations where errors are silently ignored, making debugging more difficult. It might be better to handle potential initialization failures explicitly and propagate errors, allowing for more controlled error handling and better observability of the application state when issues occur. Here is the suggested code:

init(
    configuration config: MSALPublicClientApplicationConfig,
    challengeTypes: MSALNativeAuthChallengeTypes) throws {
        guard let app = try MSALNativeAuthPublicClientApplication(configuration: config, challengeTypes: challengeTypes) else {
            throw NSError(domain: \"MSALInitializationError\", code: 1001, userInfo: nil)
        }
        self.application = app
    }

func acquireTokenSilent(parameters: MSALSilentTokenParameters,
                        completionBlock: @escaping MSALNativeAuthSilentTokenResponse) {
    guard let app = application else {
        completionBlock(nil, NSError(domain: \"MSALApplicationError\", code: 1002, userInfo: nil))
        return
    }
    app.acquireTokenSilent(with: parameters) { result, error in
        if let result = result {
            let silentTokenResult = MSALNativeAuthSilentTokenResult(result: result)
            completionBlock(silentTokenResult, error)
        } else {
            completionBlock(nil, error)
        }
    }
}

This comment refers to line 28 to 46 in the new file. AI-generated content may be incorrect

ameyapat commented 1 week ago

"AI code review (iteration 1) It might be better to ensure that the MSALAccount object in the MSALNativeAuthSilentTokenResult class is safely unwrapped before use, especially since it is being directly assigned from the result.account which could potentially be nil. This would prevent runtime crashes due to unwrapping a nil value. You could use optional binding to safely unwrap this value.

Here is the suggested code:

init(result: MSALResult) {
    self.rawIdToken = result.idToken
    if let account = result.account {
        self.account = account
    } else {
        // Handle the nil case appropriately, perhaps initializing with a default value or throwing an error
    }
    self.accessTokenResult = MSALNativeAuthTokenResult(accessToken: result.accessToken,
                                                       scopes: result.scopes,
                                                       expiresOn: result.expiresOn)
    self.correlationId = result.correlationId
}

This comment refers to line 34 to 39 in the new file. AI-generated content may be incorrect

ameyapat commented 1 week ago

"AI code review (iteration 1) It might be better to handle the potential nil return from the makeSilentTokenProvider method more explicitly. Currently, the method uses try? which could silently ignore errors thrown by the MSALNativeAuthSilentTokenProvider initializer. Instead, you could modify the method to either propagate the error or handle it within the method, depending on the desired error handling strategy. This change would improve error visibility and make the control flow clearer.

Here is the suggested code:

    func makeSilentTokenProvider(configuration: MSALPublicClientApplicationConfig,
                                 challengeTypes: MSALNativeAuthChallengeTypes) throws -> MSALNativeAuthSilentTokenProviding? {
        do {
            return try MSALNativeAuthSilentTokenProvider(configuration: configuration, challengeTypes: challengeTypes)
        } catch {
            // Handle error or propagate it
            throw error
        }
    }

This comment refers to line 26 to 29 in the new file. AI-generated content may be incorrect

ameyapat commented 1 week ago

"Failed to process request. Error: TF401181: The pull request cannot be edited due to its state. "