-
Notifications
You must be signed in to change notification settings - Fork 346
Prototype silent authentication #992
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
base: main
Are you sure you want to change the base?
Conversation
e1a5fc1
to
a4b0345
Compare
Nice thank you @lebaudantoine ! |
src/frontend/apps/impress/src/features/auth/api/useAuthQuery.tsx
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/auth/components/Auth.tsx
Outdated
Show resolved
Hide resolved
src/frontend/apps/impress/src/features/auth/api/useAuthQuery.tsx
Outdated
Show resolved
Hide resolved
0498ae1
to
53796bc
Compare
current UX (insufficient RAM on my computer are negatively impacting the current UX.) wip.mov |
While reducing the number of requests to the |
Stop retry attempts when receiving 401 Unauthorized from /me endpoint since this clearly indicates authentication status. The original purpose of the /me call is simply to determine if user is authenticated, and a 401 provides sufficient information. Prevents unnecessary network requests caused by React Query's automatic retry behavior when re-raising exceptions, which was hiding the 401 status. Improves performance and reduces server load during authentication failures.
Use returnTo capability to pass target URL during authentication, eliminating redundant renders when returning to app. Work around Next.js router limitations (can't pass data when replacing URL) by adding temporary target parameter. Discovered issue: 401 responses on document fetch trigger retry loop instead of redirecting to login. Will address in subsequent commits.
Remove variable that is no longer referenced in the codebase to improve code cleanliness and reduce potential confusion.
Reduce unnecessary fetch requests when retrieving documents with permission or authentication issues. Previous implementation was triggering multiple document requests despite having sufficient error information from initial attempt to determine appropriate user redirection. Additionally, fix issue where resetting the auth cache was triggering redundant authentication verification requests. The responsibility for checking auth status should belong to the 401 page component on mount, rather than being triggered by cache resets during error handling. Known limitations: - Not waiting for async function completion makes code harder to maintain - Added loading spinner as temporary solution to prevent UI flicker - Future improvement should implement consistent error-based redirects rather than rendering error messages directly on document page
Add initial naive implementation of silent login functionality using prompt=None parameter to request OIDC provider to skip login screen and return proper HTTP error response instead.
Update APIError message that incorrectly mentioned document when configuration fetching fails.
62d6689
to
4cecfec
Compare
We should add a configuration, I propose |
To avoid spamming the user with too many redirections, in my code, the default retry silent login is every hour. |
Hey @lebaudantoine thanks for the video. |
@virgile-dev I've not modified this behavior, it should be still functional. In the video, I'm logged out. The video illustrates that silent login is tried once per 1hour |
@@ -38,7 +38,10 @@ export const getConfig = async (): Promise<ConfigResponse> => { | |||
const response = await fetchAPI(`config/`); | |||
|
|||
if (!response.ok) { | |||
throw new APIError('Failed to get the doc', await errorCauses(response)); | |||
throw new APIError( | |||
'Failed to get the configurations', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Failed to get the configurations', | |
'Failed to get the configuration', |
attemptSilentLogin(3600); | ||
|
||
while (window.location.href === currentLocation) { | ||
await new Promise((resolve) => setTimeout(resolve, 100)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If attemptSilentLogin
fails, it seems to stay blocked in the loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OIDC_AUTHENTICATE_CLASS = "lasuite.oidc_login.views.OIDCAuthenticationRequestView" | ||
OIDC_CALLBACK_CLASS = "lasuite.oidc_login.views.OIDCAuthenticationCallbackView" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add them in /docs/env.md
.
OIDC_REDIRECT_FIELD_NAME = values.Value( | ||
"returnTo", environ_name="OIDC_REDIRECT_FIELD_NAME", environ_prefix=None | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same, should be added in /docs/env.md
.
|
||
if (!response.ok && response.status == 401 && canAttemptSilentLogin()) { | ||
const currentLocation = window.location.href; | ||
attemptSilentLogin(3600); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be in const in the conf I think, for everybody to know that it is something that we could configure, wdyt?
export const gotoLogin = (returnTo = '/', isSilent = false) => { | ||
const authenticateUrl = | ||
LOGIN_URL + | ||
`?silent=${encodeURIComponent(isSilent)}&returnTo=${window.location.origin + returnTo}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The redirection does not seems to work, returnTo
seems to be set though.
This contribution is totally draft, and waiting for peer-programming with @AntoLC .
Requested by @virgile-dev, I propose a prototype of the silent login authentication implemented in La Suite Django package.