bytefish / FcmSharp

Firebase Cloud Messaging (FCM) with .NET
MIT License
6 stars 7 forks source link

Proxy server issues #44

Closed kdlslyv closed 5 years ago

kdlslyv commented 5 years ago

Hi,

First of all, it's a neat and well done library. Thank you for that.

Kind of an edge-case issue nowadays, but still relevant in some environments: If you want to send a notification via FcmSharp over a proxy server, it's partially messed up due to how the short-lived access tokens are obtained.

By modifying the FcmClient constructor, you can set a proxy server manually like this:

var fcmClient = new FcmClient(_fcmClientSettings, JsonSerializer.Default, new FcmHttpClient(_fcmClientSettings, new ConfigurableHttpClient(new ConfigurableMessageHandler(new HttpClientHandler() { UseProxy = true, Proxy = _proxy, })), JsonSerializer.Default));

This however only works the for actual request - I haven't found a way to apply it to requests that happen rather automatically e.g. for the credentials. This request then times out and basically it won't work.

Only when modifying the CreateServiceAccountCredential method by using a custom ServiceAccountCredential.Initializer we can set a different HttpClientFactory to make the requests via a proxy work.

It works but right now it's not quite what I would envision for when submitting a pull request though - I created the initializer and copied the properties from the GoogleCredential as ServiceAccountCredential.

Sadly when using .net Core it's also quite broken trying to set a DefaultProxy for requests like these.

Would you accept a pull request allowing to set HttpClientFactory for the ServiceAccountCredentials? (I'd put it in FcmClientSettings)

Kind regards

bytefish commented 5 years ago

Yes. Please make a Pull Request so we can iterate on it!

bytefish commented 5 years ago

Do you think changing the creation of the ServiceAccountCredential to something like this would do the job?

private ServiceAccountCredential CreateServiceAccountCredential(HttpClientFactory httpClientFactory, IFcmClientSettings settings)
{
    var serviceAccountCredential = GoogleCredential.FromJson(settings.Credentials)
        // We need the Messaging Scope:
        .CreateScoped("https://www.googleapis.com/auth/firebase.messaging")
        // Cast to the ServiceAccountCredential:
        .UnderlyingCredential as ServiceAccountCredential;

    if (serviceAccountCredential == null)
    {
        throw new Exception($"Error creating ServiceAccountCredential from JSON File {settings.Credentials}");
    }

    var initializer = new ServiceAccountCredential.Initializer(serviceAccountCredential.Id, serviceAccountCredential.TokenServerUrl)
    {
        User = serviceAccountCredential.User,
        AccessMethod = serviceAccountCredential.AccessMethod,
        Clock = serviceAccountCredential.Clock,
        Key = serviceAccountCredential.Key,
        Scopes = serviceAccountCredential.Scopes,
        HttpClientFactory = httpClientFactory
    };

    return new ServiceAccountCredential(initializer);
}
bytefish commented 5 years ago

@kdlslyv I have created a Branch for this Issue at:

I have made all necessary changes, so you can pass a HttpClientFactory into the FcmHttpClient. Would you mind trying out, if this solves the problem?

kdlslyv commented 5 years ago

Hi

Thank you for personally instantly taking care of that issue - I agree with passing the HttpClientFactory into the FcmHttpClient like this, its the most fitting solution. I tried it out like: var fcmClient = new FcmClient(_fcmClientSettings, JsonSerializer.Default, new FcmHttpClient(_fcmClientSettings, new ProxyConfiguredHttpClientFactory(), new CreateHttpClientArgs())); and it applies properly to all requests.

It's even tidier than digging into the ConfigurableHttpClient and so on than before.

bytefish commented 5 years ago

Great, thanks for the feedback! 😎 I will create an example for passing in Proxy Settings into the Client and add it to the README. Then I am going to release the library as 2.8.0. I don't think many people have overridden the FcmHttpClient so I am not regarding this as a Major Breaking change.

bytefish commented 5 years ago

OK! I released it to NuGet. Please give it a try and let me know, if you are now able to use the Proxy Server correctly. Please reopen the ticket, if the error persists.

kdlslyv commented 5 years ago

No further errors ever came up so far, so I suppose it supports this case correctly.

bytefish commented 5 years ago

Thanks for the feedback. 🤜🤛