mauriciovigolo / keycloak-angular

Easy Keycloak setup for Angular applications.
MIT License
725 stars 278 forks source link

feat: add option to automatically refresh token on getToken() #532

Closed natereprogle closed 6 months ago

natereprogle commented 9 months ago

The JSDoc states that getToken() calls updateToken() if necessary, however this never happens. An option has been added that automatically refreshes the token when getToken() is called. By default this is true. This allows devs to not have to rely on the RxJS Subject or the HttpAdapter, if they chose not to use Angular's HTTP module (For example, if they use Axios instead).

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #531 describes the current behavior that is being fixed

What is the new behavior?

keycloak-angular will now attempt to refresh the token when getToken() is called if it is A) expired, and B) refreshOnGet is set to true. By default, this value is true, however, developers may opt to disable this functionality by setting refreshOnGet to false.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

getToken() now takes an optional minValidity similar to other keycloak-angular methods, which allows the developer to specify a minValidity value directly from the getToken() call instead of having to call updateToken() with a minValidity prior to calling getToken()

natereprogle commented 7 months ago

Hi! Is there an update on this? Just curious as to the current status. No rush, thanks for the work you do! :)

mauriciovigolo commented 6 months ago

Hi @natereprogle, thank you for submitting the request and your contribution.

In a previous version, the method did include this functionality, but it led to undesired errors and page redirections. To align more closely with keycloak-js, we decided to remove this check. This allows for the functionality to be implemented in the application if necessary.

I will review and update the JSDocs for this method to ensure clarity. We should not proceed with merging this change, as it could reintroduce errors.

natereprogle commented 6 months ago

Hi @natereprogle, thank you for submitting the request and your contribution.

In a previous version, the method did include this functionality, but it led to undesired errors and page redirections. To align more closely with keycloak-js, we decided to remove this check. This allows for the functionality to be implemented in the application if necessary.

I will review and update the JSDocs for this method to ensure clarity. We should not proceed with merging this change, as it could reintroduce errors.

Great, thank you! Either change will benefit the developer experience, so I appreciate the feedback!