expires doesn't work for facebook's oauth2

[kelostrada]

Referencing #59 @jeffutter
Facebook's oauth2 returns expires field as a number of seconds for the token to expire. The referenced PR made it this way that expires is interpreted as a timestamp on which the token will expire, not the number of seconds till the expiration. What is the basis for this decision?

[jeffutter]

I may be misunderstanding, but as far as i can tell, It was made so that expires is pared to an int if it is a string. Expires isn't coerced to a timestamp. I think it may ultimately be stored on the struct as a timestamp, but expires itself is converted to an integer of seconds in the future.

It was to resolve this: #58 an API from Honeywell that was using "expires_in"

[scrogson]

@jeffutter that's how I understand it as well.

[kelostrada]

yeah guys, but looking at the code I don't see expires being later converted to timestamp by calling for instance expires_at.

This is what I found in the code (access_token.ex):

expires_at: (std["expires_in"] |> expires_at) || (other["expires"] |> expires),

Maybe it should be something like this?

expires_at: (std["expires_in"] |> expires_at) || (other["expires"] |> expires |> expires_at),

What I mean is, should expires be interpreted as a timestamp or number of seconds till expiration? Looking how facebook uses it I would guess it should be the latter.

[kelostrada]

Another thing that comes on my mind is we might just change the struct to:

@type access_token  :: binary
@type refresh_token :: binary
@type expires_at    :: integer
@type expires       :: integer
@type token_type    :: binary
@type other_params  :: %{}
@type body          :: binary | %{}

And make the developer of strategy be responsible to use expires_at or expires (if any of them is available)

[scrogson]

Ultimately, expires_at should be a timestamp. We should not add another field to the struct, that will just make things more complicated.

When you get this value from the provider (Facebook), and you store the token somewhere, and then use it again, how can you tell if it's expired or not without converting the expires in seconds, to a timestamp?

If Facebook is sending a timestamp already, then the value is simply converted from a string to an integer and stored in the expires_at field.

@kelostrada does this help?

[kelostrada]

The case is as a user of oauth2 I'm not responsible of converting expires to timestamp. I get a structure AccessToken and there is a field expires_at which when I use this with Facebook is equal to something like 5180577 (which is obviously not a correct timestamp). You can check ueberauth_facebook - this is the value I get when I use their strategy.

Ah and facebook as far as I know doesn't send timestamp in the response but just the field expires which is number of seconds the token is gonna be valid for.

[scrogson]

Ok. Got it. Which version of the Facebook API are you using? Looking [here](scroll to the very bottom), they seem to be sending an expires_in field, not expires.

If we're not handling things properly, I agree that it needs to be fixed. We just need to figure out exactly how to proceed.

Also, the extra fields that are sent in the token (including expires) are available in the other_params field on the AccessToken struct (just in case you need them).

[kelostrada]

Im not sure right now :/ i will sit to this tomorrow cause right now im not available anymore, but what I can tell you is I'm just using ueberauth_facebook package and as far as I checked they do not send expires_in, maybe some older API is used by this package. I will check that tomorrow, but I know for sure now that oauth2 package handles expires field just by directly converting it to int, it doesn't try to interpret it as time stamp.

[jeffutter]

What about changing:

expires_at: (std["expires_in"] |> expires_at) || (other["expires"] |> expires),

To

expires_at: (std["expires_in"] |> expires_at) || (other["expires"] |> expires_at),

Note: expires -> expires_at at the end.

Without testing it, I think that may fix this issue.... however I'm guessing it was that way for a reason, and that would break other things.

It sounds like, perhaps, some APIs send expires as an isn't of seconds and some as a time stamp?

[kelostrada]

About expires field being sometimes sent as seconds and sometimes as timestamp - that was the same thing I was wondering. That's why I suggested changing the structure of AccessToken if that was the case.

[kelostrada]

@jeffutter

however I'm guessing it was that way for a reason, and that would break other things.

Wasn't that your idea to introduce expires function? :) Before your PR it was only expires_at which acted differently if the input was int or binary. Maybe we should just go back to using only expires_at function and assume that expires field always acts the same way as expires_in.

What I mean is that before your PR this would work exactly this way if expires field was integer instead of binary. So I guess the original idea was that expires is always the same as expires_in.

[scrogson]

If you look at the original comment by @jeffutter in #58, it looks like expires was coming back as the amount of seconds until it expires:

{"access_token": "k8sbPR4is2C7ipTYgEbi8fe470mp", "refresh_token": "dQJiREMfaHhDBoGohIj7JEpIOYYk9Jif", "expires_in": "599" }

So, the correct handling should simply be to run other["expires"] through expires_at/1.

PR welcome!

[scrogson]

Wait a minute...that key is expires_in ...either way...seems like "expires" should be converted to an integer and then added to unix_now so it's a proper timestamp.

[kelostrada]

Okay, i can prepare a PR with removal of expires function and just use expires_at in both cases

[scrogson]

#78