Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Added OidcTokenExpiredEvent and OidcTokenExpiringEvent #91

Merged
merged 4 commits into from
Nov 27, 2024

Conversation

akaegi
Copy link
Contributor

@akaegi akaegi commented Jun 12, 2024

Background: I use auth flow jwt-bearer where there is no refresh token. Still I want to auto-refresh before the token expires. The new callback onTokenExpiring can be used to achieve this.

@akaegi akaegi force-pushed the feature/on_token_expiring_callback branch from 994a534 to 878f36f Compare June 12, 2024 14:04
Copy link

codecov bot commented Jun 12, 2024

Codecov Report

Attention: Patch coverage is 0% with 12 lines in your changes missing coverage. Please review.

Project coverage is 21.28%. Comparing base (37e5bff) to head (90bbc16).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
.../oidc_core/lib/src/managers/user_manager_base.dart 0.00% 6 Missing ⚠️
...ore/lib/src/models/events/token_expired_event.dart 0.00% 3 Missing ⚠️
...re/lib/src/models/events/token_expiring_event.dart 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #91      +/-   ##
==========================================
- Coverage   21.34%   21.28%   -0.07%     
==========================================
  Files          60       63       +3     
  Lines        1799     1809      +10     
==========================================
+ Hits          384      385       +1     
- Misses       1415     1424       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@akaegi akaegi changed the title Added callbacks onTokenExpiring and onTokenExpired feat: Added callbacks onTokenExpiring and onTokenExpired Jun 12, 2024
@akaegi akaegi force-pushed the feature/on_token_expiring_callback branch from 878f36f to 670bd98 Compare June 12, 2024 14:20
@@ -803,6 +794,8 @@ abstract class OidcUserManagerBase {

@protected
Future<void> handleTokenExpiring(OidcToken event) async {
settings.onTokenExpiring?.call(event);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we still refresh using refreshToken if the user provides their own onTokenExpiring callback?

also would it make sense to expose a stream of expiring/expired events instead of callbacks ?

Copy link
Contributor Author

@akaegi akaegi Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we still refresh using refreshToken if the user provides their own onTokenExpiring callback?

Maybe there are scenarios where the user has to do some preparatory work before refresh can happen?

Copy link
Contributor Author

@akaegi akaegi Jun 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also would it make sense to expose a stream of expiring/expired events instead of callbacks ?

Whatever you prefer. I could also add events to the existing events stream using new subclasses of OidcEvent (OidcTokenExpiring and OidcTokenExpired)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahmednfwela Any news from your side? Are there any open points that prevent you from merging this PR? This is somehow important to me as I'm currently referencing my fork in our production app 🙈

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry I got busy the past months, I want to use a stream based approach instead of callbacks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ahmednfwela Please have another look, I changed to a stream-based approach. No idea why the CI run failed though (web integration tests)...

@akaegi akaegi force-pushed the feature/on_token_expiring_callback branch 4 times, most recently from 5004c16 to efeae2d Compare October 13, 2024 16:25
@ahmednfwela
Copy link
Member

hi @akaegi can you please rebase off the latest main to trigger the new CI ?

@akaegi akaegi force-pushed the feature/on_token_expiring_callback branch from efeae2d to 9a4cc47 Compare November 25, 2024 07:00
@akaegi
Copy link
Contributor Author

akaegi commented Nov 25, 2024

hi @akaegi can you please rebase off the latest main to trigger the new CI ?

done

eventsController.add(
OidcTokenExpiringEvent.now(currentToken: event),
);

final refreshToken = event.refreshToken;
if (refreshToken == null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should also check here if the server supports refresh_token grants

if (!discoveryDocument.grantTypesSupportedOrDefault
      .contains(OidcConstants_GrantType.refreshToken)) {
  //Server doesn't support refresh_token grant.
  return;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

@ahmednfwela
Copy link
Member

please run dart format to fix ci

@ahmednfwela ahmednfwela changed the title feat: Added callbacks onTokenExpiring and onTokenExpired feat: Added OidcTokenExpiredEvent and OidcTokenExpiringEvent Nov 27, 2024
@ahmednfwela ahmednfwela merged commit 85ba41c into Bdaya-Dev:main Nov 27, 2024
5 checks passed
@akaegi
Copy link
Contributor Author

akaegi commented Nov 27, 2024

Thank you for merging @ahmednfwela

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants