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

[Feature Request] Decorrelate AuthManager internal types from its neighbors #2970

Open
Alastor-git opened this issue Jan 14, 2025 · 1 comment
Labels
Enhancement A feature request or improvement

Comments

@Alastor-git
Copy link
Contributor

Alastor-git commented Jan 14, 2025

The issue
There are 3 main correlated issues the way AuthManager is currently done.

  • refreshTokenIfExpired() takes and returns a ClientOAuth2.Data typed object.
    This function is used by the IntegrationManager and fundamentally defines on the integration side how a token is structured. So the internal library used by the AuthManager needs to be known by the IntegrationManager and by integrations.
  • Tokens loose track of their creation date.
    More specifically, refreshTokenIfExpired() calls ClientOAUth2's createToken(tokenData) method to recreate a token from the stored data every time.
    Unfortunately, this method assumes that the token was just issued, and as a result, refreshTokenIfExpired() will never even attempt to refresh a token when called.
  • There is an existing internal library-independant type for storing a token called AuthDetails, used by FirebotDeviceAuthProvider. But the AuthManager makes no use of it and it defines property expires_at as a Date object, which doesn't lend very well to converting into JSON format. The only reason this hasn't been an issue yet is because account-access.js is not Typescript yet. Otherwise, it would have to manually convert the date from string to a Date object.

Proposed work

  • Make all interfaces for AuthManager use the AuthDetails type already defined.
    This particularly impacts refreshTokenIfExpired(), which is only used in the IntegrationManager.
    This will make it so the token as seen by integrations and IntegrationsManager is defined as an AuthDetails object, without need to know about the inner working of the AuthManager.
  • Create 2 methods in the AuthManager that convert from ClientOAuth2.Token to AuthDetails and vice versa.
    Moving the conversion logic to 2 functions ensures that this is done in a consistent way.
    These function's responsibility is in particular to make sure we don't lose the expiration date information along the way.

Additional context
I think this proposed work is fundamental to allowing to solve 2 issues that will be tackled in later proposals

  • Lifting the IntegrationManager from JS to TS and in doing so, providing a properly defined interface and structure for exchanges between Firebot and integrations.
    As demonstrated here, some of what's exposed to integrations is actually internal library types, and to avoid having to require them knowing about that, what's currently done is defining it as Record<string, unknown>, so in essence, providing no insight.
    By decorrelating the IntegrationManager from the AuthManager internal structure and using AuthDetails as an interface type, this allows to give proper insight to integrations and facilitates the lifting to Typescript.
  • Fixing the tokens not auto-refreshing.
    Despite having an autoRefreshToken option for integrations, there is currently no refreshing of tokens even attempted by firebot. That means that each integration has to have its own implementation of tokens refreshing and essentially bypasses firebot's utilities.
    There are more ingredients to that issue, but they should be tackled separately.
    However, not loosing track of when a token is created and expires is essential for that and means at least the bot will attempt to auto refresh it once when the integration is connected if that option is enabled.

There has been talk around the idea of moving away from client-oauth2 library, because it has been archived and moving to another library could allow supporting more auth types than OAuth2.
Decorrelating the inner working of AuthManager from other components will considerably facilitate that endeavor when the time comes.

@Alastor-git Alastor-git added the Enhancement A feature request or improvement label Jan 14, 2025
@github-project-automation github-project-automation bot moved this to Todo in Firebot Jan 14, 2025
@Alastor-git
Copy link
Contributor Author

Edited the FR to keep a minimal scope, reflecting the discussions in discord.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement A feature request or improvement
Projects
Status: Todo
Development

No branches or pull requests

1 participant