-
Notifications
You must be signed in to change notification settings - Fork 994
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
Fixes #38037 - Update last login time for External Auth user #10387
base: develop
Are you sure you want to change the base?
Conversation
The code looks logical to me as in calling the post_login method before redirecting.. I took some help from QE to test it and am not sure if the issue is reproducible though. |
I'm not sure if I completely understand the issue here, but looking at the fix it seems @hao-yu is just moving the Also I wonder if we need to keep I'm sorry for my ignorance, not an expert here, I just got somehow curious about this one. Thank you! |
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.
In general, this seems to resolve the issue, but it would be nice if we could avoid updating the time twice for internal users.
What I tested:
- Logging in as internal user through UI - this behaves worse with this patch
- Making api requests as internal user - this behaves the same
- Logging in as external user through UI - this behaves better - the time is updated exactly once
- Creating a session as an external user with hammer - this behaves better - the time is updated exactly once
Sort of unrelated note:
When making API requests as an internal user, user.post_successful_login
is called twice, but this is not introduced by this patch.
app/controllers/users_controller.rb
Outdated
@@ -217,6 +216,7 @@ def login_user(user) | |||
store_default_taxonomy(user, 'location') unless session.has_key?(:location_id) | |||
TopbarSweeper.expire_cache | |||
telemetry_increment_counter(:successful_ui_logins) | |||
user.post_successful_login |
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.
With this change, this gets called twice when logging to the UI as an internal user, once from the depths of User.try_to_login
on L127 and second time from here by calling login_user
on L143.
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.
After this patch, I also notice that user.post_successful_login
is also called twice when the external user login successfully for the first time after auto create user in find_or_create_external_user
.
This change seems better?
diff --git a/app/models/user.rb b/app/models/user.rb
index a8ca2a1ae..639029c3f 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -337,6 +337,7 @@ class User < ApplicationRecord
user.usergroups = new_usergroups.uniq
end
+ user.post_successful_login
user
# not existing user and creating is disabled by settings
elsif auth_source_name.nil?
e3fcbe5
to
614c1ee
Compare
This bug1 has been reported previously in Satellite 6.2 and fixed in Satellite 6.3
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1460963
Later on it broke again by the PR below 5 years ago:
https://github.com/theforeman/foreman/pull/7155/files#diff-cfdccd0a9d5df5a43aaad2a35d36ebbe187c52ad5fdc9846fa189d04537adb6eL201