Skip to content
New issue

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

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

Already on GitHub? # to your account

fix: InactivitySessionTimeoutMiddleware: Check for last_login (allow Django admin login + access) #7260

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

maerteijn
Copy link

@maerteijn maerteijn commented Mar 20, 2025

Reapply this PR: The original PR was closed due to inactivity,

Please reconsider and if you do not want to merge it a valid reason why not would be much appreciated.

PR fulfills these requirements

  • Commit message(s) and PR title follows the format [fix|feat|ci|chore|doc]: TICKET-ID: Short description of change made ex. fix: DEV-XXXX: Removed inconsistent code usage causing intermittent errors
  • Tests for the changes have been added/updated (for bug fixe
    s/features)
  • Docs have been added/updated (for bug fixes/features)
  • Best efforts were made to ensure docs/code are concise and coherent (checked for spelling/grammatical errors, commented out code, debug logs etc.)
  • Self-reviewed and ran all changes on a local instance (for bug fixes/features)

Note: I do not (yet) understand what should be the TICKET-ID and how to decide the commit message with this prefix DEV-XXXX, therefore I left it empty for now and just added the commit message. (Suggestion: Add documentation for outside collaborators how they can determine this).

Change has impacts in these area(s)

(check all that apply)

  • Product design
  • Backend (Database)
  • Backend (API)
  • Frontend

Not really sure which one to choose.

Describe the reason for change

Allows login into the Django admin "Out-of-the-box", and makes sure no last_login comparison is made when never logged in. See also the issue Admin Page - Not letting login #4083

What does this fix?

Allow users to login via the Django admin login screen (/admin/#)

What is the new behavior?

The InactivitySessionTimeoutMiddleware won't automatically logout a user when last_login is not set in the user session.

What is the current behavior?

The InactivitySessionTimeoutMiddleware logs out all users which do not have the last_login value set > 0 in the user session.

What libraries were added/updated?

N/A

Does this change affect performance?

No

Does this change affect security?

No

What alternative approaches were there?

I suggest to completely remove the InactivitySessionTimeoutMiddleware in a future release. Session expiration time can be set out of the box via the SESSION_COOKIE_AGE.

If manual extension / adjustion of the session time is required, you can use the .set_expiry() method. (In the /users/# view for example).

What feature flags were used to cover this change?

N/A

Does this PR introduce a breaking change?

(check only one)

  • Yes, and covered entirely by feature flag(s)
  • Yes, and covered partially by feature flag(s)
  • No
  • Not sure (briefly explain the situation below)

What level of testing was included in the change?

(check all that apply)

  • e2e
  • integration
  • unit

Which logical domain(s) does this change affect?

(for bug fixes/features, be as precise as possible. ex. Authentication, Annotation History, Review Stream etc.)
Authentication

Copy link

netlify bot commented Mar 20, 2025

👷 Deploy request for heartex-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 38653b7

Copy link

netlify bot commented Mar 20, 2025

👷 Deploy request for label-studio-docs-new-theme pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 38653b7

@github-actions github-actions bot added the fix label Mar 20, 2025
Only check for last_login when it is actually set in the session. Otherwise a user is immediately logged out when not logged in via the user_login view (for example via the Django admin)
@maerteijn maerteijn force-pushed the fix/inactivity-session-timeout-middleware-last-login branch from 4546f40 to 38653b7 Compare March 20, 2025 14:57
@makseq
Copy link
Member

makseq commented Mar 21, 2025

/git merge develop

Workflow run
Successfully merged: Already up to date.

@maerteijn
Copy link
Author

@makseq Hi, could you merge the branch manually? All workflows fail because they require a token:
image

As I'm an outside contributor, I don't have access to this token. And now I receive notifications the pipeline fails, each and every night ;)

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

Successfully merging this pull request may close these issues.

2 participants