googleads / googleads-mobile-unity

Official Unity Plugin for the Google Mobile Ads SDK
https://developers.google.com/admob/unity
Apache License 2.0
1.36k stars 1.08k forks source link

Risk of hardcoded app ID and PListProcessor #887

Closed TheEdgeWalker closed 4 years ago

TheEdgeWalker commented 5 years ago

The Initialize method requires the app ID as a parameter. This approach is acceptable for native Android or iOS, because normally only one appID is required.

Most Unity developers like myself have to deal with both Android and iOS, so we have to do something like this:

        #if UNITY_ANDROID
            string appId = "ca-app-pub-3940256099942544~3347511713";
        #elif UNITY_IPHONE
            string appId = "ca-app-pub-3940256099942544~1458002511";
        #else
            string appId = "unexpected_platform";
        #endif

        // Initialize the Google Mobile Ads SDK.
        MobileAds.Initialize(appId);

It is not pretty, but still acceptable.

But with the introduction of the PListProcessor, now we have TWO places where we have to add the app ID. Even worse, the PListProcessor may be overridden whenever we do a AdMob update. I feel that this is potentially a huge risk, especially whenever we have to update AdMob.

I think that a possible solution to this is to add a ScriptableObject (.asset), that contains the app ID of all the required platforms. Also, the Initialize method and the PListProcessor class should be modified to read the app IDs from the ScriptableObject.

rchallenger commented 5 years ago

Voting for this feature too. Android manifest also is changed to have a hard-coded value for this parameter. It is ok if you manage only one project. But if you have to deal with 10+ projects, advertising system becomes an external part, that is managed and updated independently. But this AppID makes automatisation of update process incomplete. Because you have to check some files for valid values all the time. Scriptable objects are called to solve these issues.

stowy commented 5 years ago

Hi @TheEdgeWalker, thanks for your suggestions.

The initialize method will be modified to not take an App ID in an upcoming release of the plugin. We also have the android manifest where the App ID for android is required to be entered.

We have already started work on adding a user interface to specify the app ID per platform which will then be ported into the iOS plist and the Android manifest as required.

We hope to include this in the upcoming release also. However, the app ID will only be required in one place per platform.

TheEdgeWalker commented 5 years ago

Hello @stowy I am glad that something similar is already being worked on. Can you describe in detail what this interface will look like? Do you have an ETA for this?

stowy commented 5 years ago

Sorry for the delay. We're waiting on the launch of the next version of the Android SDK. I believe it should be mid-June. The initialization call will not require an app id, and will take a completion handler.

TheEdgeWalker commented 5 years ago

@stowy the PListProcessor that I mentioned is for iOS. Please tell me you are also making a iOS version.

stowy commented 5 years ago

Yes, we are working on an editor plugin which enables this for both Android and iOS. What I mean is that the API that removes the need for the App ID in code will be available in the plugin once the next version of the Android SDK is released. This API will obviously apply to both platforms.

stowy commented 5 years ago

Hi @TheEdgeWalker, just an update that the settings menu is available in the code now but not yet released. See https://github.com/googleads/googleads-mobile-unity/tree/master/source/plugin/Assets/GoogleMobileAds/Editor. If you copy these files into your project, you should see the settings menu.

Thaina commented 5 years ago

@stowy

settings menu is available in the code now

That's great, thank you very much

I think we should also have an alert when the appID is missing or not in correct format. Because it take too times to build unity and just because a little config the build just crash should not be acceptable. This config is too crucial

Thaina commented 4 years ago

@UragayalaDeepika Is this was fixed in some way?