michielpost / Q42.HueApi

C# helper library to talk to the Philips Hue bridge
MIT License
411 stars 114 forks source link

Unable to get access token after upgrade from 3.18.1 to 3.19.0 #265

Closed StevenJDH closed 2 years ago

StevenJDH commented 2 years ago

Hello,

Last week, I saw that a new NuGet package was published. As such, I upgraded from 3.18.1 to 3.19.0 in order to get the refresh token fix from #264. However, after this upgrade, exchanging an authorization token for an access token using the GetToken() method on the RemoteAuthenticationClient class will now return nothing. Downgrading back to 3.18.1 fixes this particular issue.

I have yet to look into this deeply, but as there have been very little changes since 3.18.1, my best guess after a quick glance is that the issue could be hidden in #260. Any thoughts?

michielpost commented 2 years ago

Hi! Yes, this can very well be caused by the #260 change. The OAth endpoints needed to be upgraded, so I did and I tried to follow the documentation and do all the changes. But I don't have a working sample app anymore. The Q42.HueApi.RemoteApi.Sample included in this repository just gives a blank screen, with both the old and new endpoints. So I have no way of properly testing these changes.

Is it possible that you include the source code in your project, debug it and see what's wrong? Thanks a lot!

StevenJDH commented 2 years ago

Fair enough, and curious to what has changed to break the sample app. As soon as I have a free moment I will take a closer look and debug the changes, and if I manage to get it working I'll do a PR. I may also do another PR later to add PKCE support to compliment the API updates, which is already working in my project with your library via additional info I pass to the method parameters that play nice with the different url builders.

michielpost commented 2 years ago

Thanks for looking into it and happy to include your contributions!

teilmeier commented 2 years ago

I found the reason and created a PR. The Hue docs state that form parameters should be used for the code and grant_type parameters. #267

michielpost commented 2 years ago

Thanks for the fix. The code from your PR is released with version 3.19.1 https://www.nuget.org/packages/Q42.HueApi/

teilmeier commented 2 years ago

Added another PR to fix refresh token and JSON deserialization of the token response #268

michielpost commented 2 years ago

Thanks! Merged and it's being uploaded to NuGet

StevenJDH commented 2 years ago

Added another PR to fix refresh token and JSON deserialization of the token response #268

Nice work @teilmeier, can't wait to try out the changes. @michielpost Thanks for publishing the changes so fast.

@teilmeier Regarding #268, I haven't tested it to confirm, but based on the change, the expected behavior here is possibly impacted. If we look at the below change:

public async Task <string ?> GetValidToken() {
    if (_lastAuthorizationResponse != null) {
        if (_lastAuthorizationResponse.AccessTokenExpireTime() > DateTimeOffset.UtcNow.AddMinutes(-5)) {
            return _lastAuthorizationResponse.Access_token;
        } else {
            var newToken = await this.RefreshToken(_lastAuthorizationResponse.Refresh_token).ConfigureAwait(false);

            return newToken?.Access_token;
        }
    }
    throw new HueException("Unable to get access token. Access token and Refresh token expired.");
}

We no longer have the check on the refresh token to see if it expires, which is like 112 days if memory serves me, which I fixed in #264 as it was treated as expired when valid. By removing this check, we lose the fail fast logic which drops to the exception thrown below, and instead we will always try to request a new token along with any error that would be thrown there that might not be as clear as the one below. I would recommend adding back the check instead of the else.

Just some food for thought, but I guess this isn't the right thread for this.

teilmeier commented 2 years ago

@StevenJDH I got your point. Actually I don't use that method because I wrote my own refresh logic for my use case (https://github.com/teilmeier/azure-functions-hue).

The issue is that the current Remote Auth API does not return an expiry date anymore. If Signify confirms that refresh tokens are valid for x seconds we could calculate the expiry date by using the creation time of the token and add a constant value.

The current Hue developer docs do not mention anything about the token lifetimes. So I assumed refresh tokens do not expire. I'd contact Signify and ask for a statement why they removed the refresh token lifetime from the response and what the actual lifetime is.

I hope to get a good guidance. Philip

StevenJDH commented 2 years ago

@teilmeier Thanks for the info. In taking a similar approach to using my own implementation for the remote authentication API, I can see they did indeed drop the refresh_token_expires_in field. Let's see if the documentation gets updated with something concrete around its expiration.

@michielpost I think we can close this issue as I've tested the changes with 3.19.1, and all the token workflows along with the needed modifications for the latest auth api are working for me.

michielpost commented 2 years ago

Great to hear and thanks for the PRs to fix this @teilmeier and @StevenJDH