feat: Define onTokenExpired event

[danielSbastos]

Description of change

Hello there,

In this PR, two main changes are introduced:

1. A callback handler for expired access token is incorporated into the client.
2. The need for a client to manually set the accessToken is removed and is instead now done inside the code, together with the setting of refreshToken and expiresAt

NOTE: This PR is still in draft because I am unsure of the approach to set the accessToken, refreshToken and expiresAt. I think this is internal knowledge of the SDK and should be handle there, however, the previous method to set the accessToken by the client existed, therefore I am a bit confused as to which approach to follow. Also, by removing the getter and setter of accessToken, a breaking change would be introduced.

Issues resolved by this PR

This PR solves #25

[cla-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @danielSbastos on file. In order for us to review and merge your code, please contact the project maintainers to get yourself added.

[cla-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @danielSbastos on file. In order for us to review and merge your code, please contact the project maintainers to get yourself added.

[jfoliveira]

@cla-bot check

[cla-bot]

The cla-bot has been summoned, and re-checked this pull request!

[jfoliveira]

Hello @danielSbastos! 👋
Welcome to Devopness open source projects!

The user danielSbastos has been added to our allowed contributors list.

Thank you 💖 and congrats 🎉 for opening your very first pull request in this repository.
Hope you have a great time contributing and learning around here! 😄

[jfoliveira]

The previous example was intentionally showing new users how to retrieve the accessToken generated by the login API endpoint.

How would the access token be obtained now?
Could you please make that clear in the README example code/comments?

[danielSbastos]

So, this is a good question. Now, in the PR, the accessToken is being internally set at:

devopness/packages/sdks/javascript/src/services/ApiBaseService.ts

Lines 129 to 135 in e6797f9

	private setAuthParams(accessToken : string, refreshToken : string, expiresIn : number) : void {
	ApiBaseService._accessToken = accessToken || ApiBaseService._accessToken;
	ApiBaseService._refreshToken = refreshToken || ApiBaseService._refreshToken;
	ApiBaseService._accessTokenExpiresAt = expiresIn
	? Date.now() + expiresIn
	: ApiBaseService._accessTokenExpiresAt;
	}

together with the refreshToken and expiresIn, which removes the need for the user to set it and instead delegates the responsibility for the SDK. My question is:

• Should the user need to manually set the accessToken? If so, then is this behaviour also extended for the refreshToken and expiresIn?
  • I thought of removing this responsibility from the user mainly because of the expiresIn token, since it is transformed in a expiresAt at

    devopness/packages/sdks/javascript/src/services/ApiBaseService.ts

    Lines 132 to 134 in e6797f9

    	ApiBaseService._accessTokenExpiresAt = expiresIn
    	? Date.now() + expiresIn
    	: ApiBaseService._accessTokenExpiresAt;

    (this new field _accessTokenExpiresAt is used after to check if the token is expired, see here). If the user were to set it manually, just like with the accessToken, my opinion is that there would be more code burdensome and margin for error. They would have to do something along lines of:

async function authenticate(email, pass) {
  const userTokens = await devopnessApi.users.loginUser({ email: email, password: pass });

  devopnessApi.accessToken = userTokens.data.access_token;
  devopnessApi.refreshToken = userTokens.data.refresh_token;
  devopnessApi.accessTokenExpiresAt =  userTokens.data.expiresIn
            ? Date.now() +  userTokens.data.expiresIn
            : devopnessApi.accessTokenExpiresAt;
}
authenticate('[email protected]', 'secret-password');

WDYT?

[jfoliveira]

Should the user need to manually set the accessToken? If so, then is this behaviour also extended for the refreshToken and expiresIn?

No, it should not be needed but must be possible.
A simple way for you to test is creating a simple web page that consumes your version of this SDK-JS (npm link can be helpful for local tests).

Imagine you logged into the SDK and SDK set the token internally for the current instance. Fine!
Now the user reloads the page (hits F5) and a new login will be performed instead of re-using the previous token.
That's not the desired state!

A common practice in web development is to store the token in the browser local storage, which is persisted in between page loads, close browser, etc, so the user can get back to the page and still be logged in without being prompted again for login.

For having this behavior in an application that consumes this SDK, the client applications must be able to read/set the accessToken if that's their intended behavior - which very likely will be, as this a common practice across the web development community.

Makes sense now?

[danielSbastos]

Ahhh, I see now why the user was defining the accessToken before, it makes sense after you mentioned the issue with the session! 😄

Okay.... I will revert the changes and give the user the responsibility to set the accessToken, refreshToken and expiresIn (it will be converted to a expiresAt). I will create a setter for both three and isolate the logic of the expiresIn to expiresAt conversion (Date.now() + expiresIn) inside the devopnessApi object. Maybe with

devopnessApi.setAuth(userTokens.data)

but also with the choice of individually setting each:

devopnessApi.accessToken = <...> 
devopnessApi.refreshToken = <...>
devopnessApi.accessTokenExpiresAt = <...>

WDYT?

[jfoliveira]

I don't see any need at the moment to allow user to set refreshToen and accessTokenExpiresAt.
They already available to the user on the response of users.loginUser(), so client apps can choose to store it or not, but not to set it to the SDK.
All the SDK needs to be externally set is the accessToken.

Once the onTokenExpired event is triggered, client apps can intercept it and use the stored refreshToken without the need to set it in the SDK instances.

Does it make sense?

[danielSbastos]

Yes, it does. Thank you for explaining it clearly :) I'll make the changes!

[jfoliveira]

NOTE: This PR is still in draft because I am unsure of the approach to set the accessToken,

@danielSbastos please see my comments near
https://github.com/devopness/devopness/pull/724/files#diff-c3e0a26c94e6f04b527990186dc2c64eaf953b64927e54536f3cf607290ace6eL45
As the existing approach to set the accessToken is already documented in the README file, but it's being removed in your proposed changes.

I think this is internal knowledge of the SDK and should be handle there, however, the previous method to set the accessToken by the client existed, therefore I am a bit confused as to which approach to follow. Also, by removing the getter and setter of accessToken, a breaking change would be introduced.

You can follow the introduction guide to Devopness and once logged in you can inspect the call to /login and /refresh-token in a browser network panel, so you can see how the API endpoints are invoked and which data is returned by each one of them.

You can also check the attributes on the response model for each of these endpoints:

1. UserLoginResponse
2. UserRefreshTokenResponse

Hope that helps!

[danielSbastos]

Hey @jfoliveira, thank you for the inputs! I replied to some and asked a few questions. I think there is a point I did not explained is that I was not able to differentiate solely on the status code 401 if the accessToken was expired, therefore, I added the logic to use the expiresIn field (later transformed at expiresAt) coupled with a 401 response to decide.

If the status code if 401 AND the current time is later than expiresAt, then the accessToken is expired and the callback handler is invoked passing in the refreshToken as the argument, so that the user can later call the UserRefreshToken. One could even remove the 401 status check, but I think it is safer to continue with this extra validation.

[danielSbastos]

Hey @jfoliveira, sorry for the delay. I'm thinking of ways to verify the expiresIn (or expiresAt) without relying on the application state. Would it be okay to assume that the user has set it on the localStorage? And then on the function to verify the authentication error, fetch from it?

By assuming the user sets in the localStorage also implies the addition on the respective documentation/instructions for them to know that this is needed

[jfoliveira]

Hey @jfoliveira, sorry for the delay. I'm thinking of ways to verify the expiresIn (or expiresAt) without relying on the application state. Would it be okay to assume that the user has set it on the localStorage?

We can't assume that for many reasons including, but not limited to, the fact that this SDK is designed to be used not only browser, but also in Node.js command line applications, which cannot access localStorage.

Moreover, the developers consuming this SDK are the ones responsible to manage if/how any SDK exposed data, such as accessToken, are going to be stored by their apps.

We should not impose any requirement on how the client apps consuming this SDK are designed by their developers.

[danielSbastos]

Hey @jfoliveira, sorry for the delay. I'm thinking of ways to verify the expiresIn (or expiresAt) without relying on the application state. Would it be okay to assume that the user has set it on the localStorage?

We can't assume that for many reasons including, but not limited to, the fact that this SDK is designed to be used not only browser, but also in Node.js command line applications, which cannot access localStorage.

Moreover, the developers consuming this SDK are the ones responsible to manage if/how any SDK exposed data, such as accessToken, are going to be stored by their apps.

We should not impose any requirement on how the client apps consuming this SDK are designed by their developers.

Gotcha, that makes sense. Maybe the best option is then for the client to implement an interface, injecting it into the SDK, on how to access this token, which its implementation would be up to them?

[jfoliveira]

Maybe the best option is then for the client to implement an interface, injecting it into the SDK, on how to access this token, which its implementation would be up to them?

We should not care or influence how SDK clients implement anything on their side.
We should not interfere on any client application design.
We should not judge or limit how client apps consume this SDK.

We should just expose data and trigger events so client apps can handle the events anyway their developers feel like and read/store the exposed data in any ways that suit them better.

I believe I'm not following what is making this implementation so hard and why you're trying to find ways to tell the SDK clients what to do.

Are you sure we can't just keep things simple and expose an event handler method as discussed in previous comments and in original issue #25?

[danielSbastos]

We should not care or influence how SDK clients implement anything on their side. We should not interfere on any client application design. We should not judge or limit how client apps consume this SDK.

We should just expose data and trigger events so client apps can handle the events anyway their developers feel like and read/store the exposed data in any ways that suit them better.

I believe I'm not following what is making this implementation so hard and why you're trying to find ways to tell the SDK clients what to do.

Are you sure we can't just keep things simple and expose an event handler method as discussed in previous comments and in original issue #25?

What is making it not so simple is because, right now, the solution to invoke the callback depends on the 401 status code and the expiresIn value, which should be stored somewhere. I did not find a difference on the 401 response from an expired token or an general authentication error.

Given this, there is a need to define a strategy on how to handle the expiresIn. Previously you mentioned that you saw no need for the client to set the refreshToken and expiresIn, so I opted to remove it from the SDK, but then the issue on how to access the exporesIn remained. This lead to the previous option I suggested.

If you have another idea on how to manage the expiresIn, please tell me, given that right now, the solutions I mentioned were the only ones I was able to come up.

[jfoliveira]

I did not find a difference on the 401 response from an expired token or an general authentication error.

@danielSbastos well, that's the basic requirement of this task. If you didn't know how to check that, which is understandable, I would expect you to have written no code at all and specially before proposing to remove existing working code. As you started adding and removing code, I thought you knew exactly what you were doing. 🤷‍♂️
We'd better plan before writing code. Keep that in mind!

That being said ...

If you just keep the accessToken property exposed and required to be set by client apps when creating an instance of this SDK, as it was already working before, you could then decode the accessToken JWT and check if the token is expired by checking the value on the token exp claim/attribute.

We would just need to base64 decode the accessToken and check its exp value:

const decodedToken = JSON.parse(
  Buffer.from(accessToken.split(".")[1], "base64").toString()
);
console.log(new Date(decodedToken.exp * 1000));

Having that in place, you could have something like the pseudo code below:

if (response.statusCode == 401) {
  const isTokenExpired = decodedToken.exp < new Date().getTime() / 1000;

  if (isTokenExpired) {
    "trigger `onTokenExpired` callback"(<params>);
  }
}