feat: Support Traceable Sessions #442

itsmechlark · 2024-02-10T08:37:05Z

Allows having a history of login sessions with flexible active session limits.

Related #71

coveralls · 2024-02-14T15:43:22Z

coverage: 98.321% (+0.2%) from 98.169%
when pulling 882d2a5 on itsmechlark:feat-session-traceable
into 125b0d0 on devise-security:main.

dillonwelch

Looks like merge conflicts now since you so kindly fixed the test suite in a separate PR.

Can you look at some of the changes you made to the test config and see if they could be pulled out separately as well? Would make the review on this one easier, which I want to be thorough on since it's a new module and impacts an existing module. Also, I want to get other changes in so I can cut a new minor version and then have this in a new major version.

Note that I'm reviewing this while very sleepy and will review again in the near future with a more clear mind.

dillonwelch · 2024-02-14T15:45:30Z

lib/devise-security/models/session_limitable.rb

        end
      end

+      # Removes old/inactive session if allowed.


I don't think we should be deactivating / updating records in a boolean method like this. It should just be returning true / false without any side effects.

The purpose of allow_limitable_authentication? is to:

If session_traceable module is not included then it will always be true.

If didn't matched to 1 and number of sessions is still within the allowable number of session then it will allow authentication.

If didn't matched to 2 and will only reject authentication on limit then it will try to remove any expired session to accomodate new authentication using deactivate_timeout_sessions!.

If didn't matched to 3 (which reject authentication is disabled) then it will expire the oldest session regardless if its already expired.

If didn't matched to 4 then disallow authentication.

Steps 3-5 don't feel like they belong in this method - boolean methods should only be returning true / false based on whatever logic you're adding. Steps 1-2 seem fine and what this method should be used for.

dillonwelch · 2024-02-14T15:48:54Z

lib/devise-security/models/session_limitable.rb

+        return true unless devise_modules.include?(:session_traceable)
+
+        opts = session_traceable_condition(active: true)
+        return true if max_active_sessions > session_traceable_adapter.find_all(opts).size


Having a little trouble following here - what is the return of session_traceable_adapter and what are you doing with find_all?

session_traceable_adapter will always be the session_history_class ORM compatible on session_traceable module. It will count the number of active sessions to check whether it is already on its maximum allowable number of sessions.

Unclear to me why we can't just use SessionHistory.

lib/generators/active_record/devise_security_generator.rb

dillonwelch · 2024-02-14T16:21:20Z

lib/devise-security/models/session_traceable.rb

+
+      # Log user session.
+      # @return [true] if session is allowed to log.
+      # @return [false] if session isn't allowed to log when already exceeded to `:max_active_sessions`


Should indicate the allow_limitable_authentication? part as well.

lib/devise-security/models/session_traceable.rb

dillonwelch · 2024-02-14T16:25:37Z

lib/devise-security/models/session_traceable.rb

+      end
+
+      # Update the `:last_accessed_at` to current time.
+      def update_traceable_token(token)


Thoughts on making these update / expire methods bang methods?

dillonwelch · 2024-02-14T16:30:17Z

lib/devise-security.rb

+
+  # Allow customization of the session history class
+  mattr_accessor :session_history_class
+  @@session_history_class = 'SessionHistory'


Can you elaborate on why you added this? It adds a lot of complexity and I'm not sure what benefit it gives.

dillonwelch · 2024-02-14T16:31:05Z

lib/devise-security/models/session_traceable.rb

+      # Update the `:last_accessed_at` to current time.
+      def update_traceable_token(token)
+        record = find_traceable_by_token(token)
+        record.update_attribute_without_validatons_or_callbacks(:last_accessed_at, Time.current.utc)


Seems like you should have a presence check like you have for expire_session_token.

dillonwelch

Pressed the button before I was done :)

lib/devise-security/models/session_traceable.rb

lib/devise-security/models/session_limitable.rb

dillonwelch · 2024-02-14T16:41:04Z

lib/devise-security/models/session_limitable.rb

Reading through this again, this feels separate from the SessionTraceable module you're adding.

If this is the case, it feels like it would be better as a separate PR.

If this is not the case, please let me know.

This changes will allow to maintain the original logic of session_limitable which only allow 1 session at a time when session_traceable module but when session_traceable is added then it add flexibility on session limitable to customize maximum sessions per user and allowing session to expire. On scenarios that a user already hit the limit then its either reject authentication or expire the oldest session.

lib/devise-security/models/session_traceable.rb

dillonwelch · 2024-02-14T16:59:07Z

lib/devise-security/models/mongoid/session_history.rb

Given that this is supposed to be a history record, it seems like it would be more granular to create a new record each time the session was accessed instead of having one record for the session with last_accessed_at being updated.

If we create a session history every time a user is fetch (on warden.authenticate or on devise current_user) then it will create a lot of session histories. Let's say in a single session of a user every time rails controller is hit, a history which likely in a single session it will be thousand of requests.

The main purpose of last_accessed_at is to track when the specific session (which track using token) is being used. The data relationship will always be a user has many sessions and last activity is tracked.

dillonwelch · 2024-02-15T18:28:17Z

Thank you for the quick responses. I will get to them soon.

dillonwelch

Can fix the merge conflicts now since I merged in the other PR.

dillonwelch · 2024-02-21T20:46:19Z

lib/devise-security/models/session_limitable.rb

+      # @return [1] by default. This can be overridden by application logic as necessary.
+      def max_active_sessions
+        # Always fallback to default logic when session_traceable is not supported.
+        return 1 unless devise_modules.include?(:session_traceable)


Why do we need to have session_traceable turned on before allowing more than 1 active session?

dillonwelch · 2024-02-21T20:50:12Z

lib/devise-security/models/session_limitable.rb

+        return true unless devise_modules.include?(:session_traceable)
+
+        opts = session_traceable_condition(active: true)
+        return true if max_active_sessions > session_traceable_adapter.find_all(opts).size


Unclear to me why we can't just use SessionHistory.

dillonwelch · 2024-02-21T20:52:15Z

lib/devise-security/models/session_limitable.rb

        end
      end

+      # Removes old/inactive session if allowed.


Steps 3-5 don't feel like they belong in this method - boolean methods should only be returning true / false based on whatever logic you're adding. Steps 1-2 seem fine and what this method should be used for.

itsmechlark force-pushed the feat-session-traceable branch 4 times, most recently from 4f0ecdf to 0c4f06f Compare February 11, 2024 16:02

itsmechlark mentioned this pull request Feb 14, 2024

ci: Github Codespaces configuration #443

Merged

dillonwelch reviewed Feb 14, 2024

View reviewed changes

itsmechlark force-pushed the feat-session-traceable branch 2 times, most recently from 20d9954 to 5c1dee4 Compare February 15, 2024 09:20

itsmechlark force-pushed the feat-session-traceable branch 2 times, most recently from 724168b to 6ee1cee Compare February 16, 2024 16:34

dillonwelch reviewed Feb 21, 2024

View reviewed changes

feat: Support Traceable Sessions

882d2a5

itsmechlark force-pushed the feat-session-traceable branch from 6ee1cee to 882d2a5 Compare February 22, 2024 10:23

feat: Support Traceable Sessions #442

Are you sure you want to change the base?

feat: Support Traceable Sessions #442

Uh oh!

Conversation

itsmechlark commented Feb 10, 2024

Uh oh!

coveralls commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dillonwelch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dillonwelch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dillonwelch commented Feb 15, 2024

Uh oh!

dillonwelch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Feb 14, 2024 •

edited

Loading