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

[App] Migrate authentication and refreshing to the SDK #21938

Merged
merged 36 commits into from Mar 31, 2024

Conversation

br41nslug
Copy link
Member

@br41nslug br41nslug commented Mar 21, 2024

Fixes #20810

Scope

What's changed:

  • Refactored the app refresh logic to use the SDK
  • Added shares authentication to the SDK
  • Implemented int32 setTimeout guard in the SDK
  • Improved error handling in the SDK
  • Added ofetch to handle interceptors in a similar way to axios while both are still in use.

Potential Risks / Drawbacks

  • Lorem ipsum dolor sit amet
  • Consectetur adipiscing elit

Review Notes / Questions

  • Is this a minor or major app change?

This comment was marked as resolved.

@br41nslug br41nslug marked this pull request as ready for review March 22, 2024 12:17
app/src/auth.ts Outdated Show resolved Hide resolved
app/src/auth.ts Outdated Show resolved Hide resolved
app/src/events.ts Outdated Show resolved Hide resolved
app/src/api.ts Show resolved Hide resolved
app/src/auth.ts Outdated Show resolved Hide resolved
sdk/src/auth/composable.ts Outdated Show resolved Hide resolved
Copy link
Member

@paescuj paescuj left a comment

Choose a reason for hiding this comment

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

Noticed the share login as well as login via provider requiring a identifier didn't work. Therefore reverted that part resp. added separate requests for them for now.
Since the signature & request payload for the two cases above are actually quite different to the regular login, I think we need a new approach for the adaptation of the SDK login methods (e.g. credentials object as first param or overloading). Let's better do that in a separate PR where we can focus on that (potentially breaking) change.
Therefore this PR is no longer a breaking change.
All auth methods are working fine now:

  • Default auth
  • Local auth
  • SSO auth
  • LDAP auth (identifier)
  • Share
    • with password
    • password-less
  • Refreshing
  • Logout

@paescuj paescuj merged commit 988bc87 into main Mar 31, 2024
5 checks passed
@paescuj paescuj deleted the restitutio-auth-migration branch March 31, 2024 12:28
@github-actions github-actions bot added this to the Next Release milestone Mar 31, 2024
@paescuj paescuj restored the restitutio-auth-migration branch March 31, 2024 12:31
@paescuj paescuj deleted the restitutio-auth-migration branch March 31, 2024 14:04
@br41nslug br41nslug restored the restitutio-auth-migration branch April 1, 2024 11:48
Copy link
Member Author

@br41nslug br41nslug left a comment

Choose a reason for hiding this comment

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

Changes requested if it wasnt merged, these authentication methods need to be fixed instead of working around them for our app only.


await sdk.request(login());
// To initialize auto-refresh
response = await sdk.refresh();
Copy link
Member Author

@br41nslug br41nslug Apr 1, 2024

Choose a reason for hiding this comment

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

This should just be setting the token and expiry received from the login instead of doing a 2 http call.

Comment on lines +49 to +55
const login =
<Schema extends object>(): RestCommand<AuthenticationData, Schema> =>
() => {
const path = getAuthEndpoint(loginOptions.provider);
const data = { identifier, password, otp, mode: 'session' };
return { path, method: 'POST', body: JSON.stringify(data) };
};
Copy link
Member Author

@br41nslug br41nslug Apr 1, 2024

Choose a reason for hiding this comment

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

This options should be added to the SDK as sdk.login(identifier, pass, opts)

Comment on lines +36 to +38
await sdk.request(authenticateShare(share, password, 'session'));
// To initialize auto-refresh
response = await sdk.refresh();
Copy link
Member Author

@br41nslug br41nslug Apr 1, 2024

Choose a reason for hiding this comment

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

Why the old function and double http call? If the authentication handler doesnt work here that needs to be fixed instead of reverted/worked around.

Comment on lines +3 to +10
export type LoginOptions = {
/** The user's one-time-password (if MFA is enabled). */
otp?: string;
/** Whether to retrieve the refresh token in the JSON response, or in a httpOnly cookie. One of `json`, `cookie` or `session`. Defaults to `cookie`. */
mode?: AuthenticationMode;
/** Use a specific authentication provider (does not work for SSO that relies on browser redirects). */
provider?: string;
};
Copy link
Member Author

Choose a reason for hiding this comment

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

These comments are redundant

@paescuj paescuj deleted the restitutio-auth-migration branch April 4, 2024 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[App] Migrate authentication and refreshing to the SDK
2 participants