Auth - use GULAppDelegateSwizzler #2710
Conversation
…DelegateSwizzler [WIP]" This reverts commit b2eabf2.
…public interface with no assumptions on the implementation details
…lementation details assumptions
Conflicts: Example/Messaging/Tests/FIRMessagingRemoteNotificationsProxyTest.m
…s applied to all targets. Move the hook to the top level to avoid confusion.
…etProxyOriginalDelegateOnceToken] at the beginning of each test.
…sts added. Cleanup
…essage:] before calling delegate.
| #import <FirebaseCore/FIRLogger.h> | ||
| #import <FirebaseCore/FIROptions.h> | ||
| #import <GoogleUtilities/GULAppEnvironmentUtil.h> | ||
| #import <GoogleUtilities/GULAppDelegateSwizzler.h> |
There was a problem hiding this comment.
Move this line above #import <GoogleUtilities/GULAppEnvironmentUtil.h>.
| didReceiveRemoteNotification:(NSDictionary *)userInfo | ||
| fetchCompletionHandler:(void (^)(UIBackgroundFetchResult))completionHandler { | ||
| [self canHandleNotification:userInfo]; | ||
| } |
There was a problem hiding this comment.
If Auth can handle the notification, we want to stop passing the notification to other UIApplicationDelegate interceptors. Is it possible with GULAppDelegateSwizzler?
There was a problem hiding this comment.
GULAppDelegateSwizzler is a generic class that makes sure the methods are called for each interceptor as well as for the original implementation. AFAIK it would be a violation of the class design if one interceptor could affect other interceptors (e.g. prevent calling a method). @tejasd @ryanwilson please let us know if it is not true.
@renkelvin If it is an essential to call the methods on only one arbitrary instance of FIRAuth I can recover FIRAuthAppDelegateProxy and put this logic there. What do you think?
There was a problem hiding this comment.
GULAppDelegateSwizzler cannot currently handle this scenario.
@renkelvin Would you be able to elaborate on why that's necessary? At first glance, that seems like a dangerous thing to do.
There was a problem hiding this comment.
@tejasd Yeah, I agree it seems dangerous. Auth uses remote notification and redirected URL to pass some sensitive information. These information are necessary but not sufficient to hack a user's account. It would be better if we can hide them if possible.
@maksymmalyhin Yeah, it's not what GULAppDelegateSwizzler should handle. I agree.
Let me double check with my team to see if we can find a better solution and get back to you guys. Thanks!
There was a problem hiding this comment.
Hi all, after further discussions, we think Auth should follow the pattern of GULAppDelegateSwizzler and we'll be more restricted to the information passed in notifications and urls.
@maksymmalyhin FIRAuthAppDelegateProxy can't fully protect the sensitive information, so let's still discard it. Thank you!
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the ℹ️ Googlers: Go here for more info. |
|
It looks like GitHub could not handle retargeting the PR to |
|
GitHub seems to do better with rebases than merges :( |
|
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) ℹ️ Googlers: Go here for more info. |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
5 participants
This is a part of #2591
I target it to
mm/app-swizzler-apnsto simplify the review. The PR should be merged tomasterafter #2698