THREESCALE-10843: Audit user sessions #3885

jlledom · 2024-09-10T12:52:52Z

What this PR does / why we need it:

This PR adds audits for UserSession. In particular fot :create and :revoke operations

The Jira issue also mentions auditing when a User is locked. I'm not sure what "locked" means but I assume it means "suspended". If that's the case, then that was already done.

Besides, the issue also asks to add some audit logs for some operations:

Login
- success (this was already done)
- failed
User account
- password change success
- password change failed

In both admin and developer portals.

The PR also adds some tests for the new functionality.

Which issue(s) this PR fixes

https://issues.redhat.com/browse/THREESCALE-10843

Verification steps

Just perform the operations above and check the audits appear.

app/controllers/provider/admin/user/personal_details_controller.rb

app/controllers/provider/sessions_controller.rb

akostadinov · 2024-09-11T12:50:21Z

...veloper_portal/app/controllers/developer_portal/admin/account/personal_details_controller.rb

@@ -19,7 +19,10 @@ def show
  def update
    resource.validate_fields!
    if resource.errors.empty? && resource.update(user_params)
-      resource.kill_user_sessions(user_session) if resource.just_changed_password?
+      if resource.just_changed_password?
+        AuditLogService.call("User changed password: #{resource.id}/#{resource.username}")


Don't we audit the whatever object stores the password already?

That's the user model. I'd say the corresponding attribute is password_digest, and we don't audit it:

porta/app/models/user.rb

Lines 35 to 36 in 07e52fe

audited except: %i[salt posts_count janrain_identifier cas_identifier password_digest

authentication_id open_id last_login_at last_login_ip crypted_password].freeze

Anyway, since the jira description explicitly mentions Audit logs, I assumed that was the thing to do.

I think it makes sense to start auditing that field. Just redact the value.

https://github.com/collectiveidea/audited/blob/9aad4d46e52625d1608917df22d0f31fbd873db4/lib/audited/auditor.rb#L40-L48

Do you think we should remove the audit logs then? or should we have both?

I removed the logs here: dd3665a

akostadinov · 2024-09-11T12:53:25Z

lib/developer_portal/app/controllers/developer_portal/login_controller.rb

@@ -31,19 +31,29 @@ def create
      self.current_user = @user
      create_user_session!
      flash[:notice] = @strategy.new_user_created? ? 'Signed up successfully' : 'Signed in successfully'
+
+      AuditLogService.call("Signed in: #{current_user.id}/#{current_user.username} #{current_user.first_name} #{current_user.last_name}")


Isn't full name a little bit too much logging?

Also, provided that we audit UserSession, why do we need a separate log message that a user logged in? Maybe one can watch the sessions creation audits.

Isn't full name a little bit too much logging?

Maybe, I copied this from what we do in the admin portal:

porta/app/controllers/provider/sessions_controller.rb

Line 31 in ca21bb0

AuditLogService.call("Signed in: #{current_user.id}/#{current_user.username} #{current_user.first_name} #{current_user.last_name}")

Also, provided that we audit UserSession, why do we need a separate log message that a user logged in? Maybe one can watch the sessions creation audits.

Maybe it's not required. I added the log because that's what the issue description says. But I'm open to remove this traces if you think they are not needed.

Yeah, I think we should add redundant logging carefully as our logs are huge already. I see no value in this log. But if you see that it adds something useful, that might be fine.

And what about the audit log already existing in the admin portal, should we remove it also? It's not required anymore now we are auditing UserSession

+1, with a note in the release notes.

Aternatively we may stop auditing session and keep only the log 😎

Removed the logs: dd3665a

Also added some release notes to the issue.

lib/developer_portal/app/controllers/developer_portal/login_controller.rb

mayorova · 2024-09-17T10:59:47Z

app/controllers/provider/sessions_controller.rb

@@ -28,12 +28,12 @@ def create
      create_user_session!(strategy&.authentication_provider_id)
      flash[:notice] = 'Signed in successfully'

-      AuditLogService.call("Signed in: #{current_user.id}/#{current_user.username} #{current_user.first_name} #{current_user.last_name}")


Is this log removed because now there are audit logs for the "UserSession" model?

Yes, exactly. Now we audit UserSession and also User#password_digest, so we don't need augit logs for login/logout and user password change

mayorova · 2024-09-17T11:07:34Z

app/controllers/provider/sessions_controller.rb

      redirect_back_or_default provider_admin_path
    else
      new
      flash.now[:error] ||= strategy&.error_message
+      attempted_cred = auth_params.fetch(:username, 'SSO')


Why the fallback is "SSO"? 🤔

What I want here is to provide useful information about which user tried to login. When the auth strategy is user/password, then we can log the usename, but when the strategy is SSO, then I was initially logging the given token. @akostadinov was concerned about how safe was to log that, so I just log SSO now.

hm, I am not quite sure about the SSO flow, I'd need to check, but would there potentially be some value in logging the system_name of the auth provider instead?... just thinking out loud.

Maybe, that would allow admins to know which auth provider was being used when it failed to log in. Probably that's better than just print SSO

akostadinov · 2024-09-17T19:14:43Z

...veloper_portal/app/controllers/developer_portal/admin/account/personal_details_controller.rb

-      resource.kill_user_sessions(user_session) if resource.just_changed_password?
+      if resource.just_changed_password?
+        resource.kill_user_sessions(user_session)
+      end


why this change? Seems like rubocop prefers single line :)

But actually wasn't password_digest attr important now?

why this change? Seems like rubocop prefers single line :)

Yeah, it was single line before but I made it complete form because I was also adding the audit log here. But then I removed the audit log and forgot to make it single line again.

But actually wasn't password_digest attr important now?

password is a field in the form, not the DB column, that is called password_digest since we don't save passwords in plain text. In fact, look how is just_changed_password implemented:

porta/app/lib/authentication/by_password.rb

Lines 41 to 43 in 843f799

def just_changed_password?

saved_change_to_password_digest? || super

end

Fixed the if: df2fa19

akostadinov

I have a line question that is probably irrelevant. And would suggest adding a test for the password change audit unless I missed one existing.

Approved in case its just my oversight.

akostadinov · 2024-09-19T12:21:53Z

test/functional/developer_portal/admin/account/personal_details_controller_test.rb

+    login_as @buyer.admins.first
+    assert_difference(Audited.audit_class.method(:count)) do
+      User.with_synchronous_auditing do
+        put :update, params: { user: {current_password: 'supersecret', password: 'new_password', password_confirmation: 'new_password'} }


also asserting that the value of old and new password in audit is [REDACTED] could be useful

Mmmm, actually...

[27] pry(main)> user.audits.last => #<Audited::Audit:0x000000004247d5a0 id: 1454, auditable_id: 2, auditable_type: "User", user_id: 2, user_type: "User", username: nil, action: "update", version: 2, created_at: Thu, 19 Sep 2024 12:02:25.000000000 UTC +00:00, tenant_id: 2, provider_id: 2, kind: "User", audited_changes: {"password_digest"=>"[FILTERED]"}, comment: nil, associated_id: nil, associated_type: nil, remote_address: "127.0.0.1", request_uuid: "d97ee5f5-ccf1-4dc8-a39a-e4e2aec767e3"> [28] pry(main)> user.audits.last.audited_changes => {"password_digest"=> ["$2a$12$TshOAone/WSVbXsCmOB1JuLGBQI698CButPpA2F7hw1jEb2db4YWi", "$2a$12$.0G4VigmmeE1cjozQbXPkuyYF5iBjcfIwtjqcqJCPomceKUr7REkS"]}

Is that expected?

Shit. It was actually not redacting the value. I'll fix it and add the test.

Fixed: efa0fdf

And updated the tests: 5f93b2c

jlledom · 2024-09-19T12:22:06Z

And would suggest adding a test for the password change audit unless I missed one existing.

I added that test for provider and developer portals: 7607ae0

So please, approve or die.

akostadinov · 2024-09-19T14:45:26Z

test/functional/developer_portal/admin/account/personal_details_controller_test.rb

+    expected = [Audited::Auditor::AuditedInstanceMethods::REDACTED] * 2
+    assert_equal expected,user.audits.last.audited_changes['password_digest']


I don't understand why it is *2 but better than the actual value :)

[Audited::Auditor::AuditedInstanceMethods::REDACTED] * 2

is a cool way to do

[Audited::Auditor::AuditedInstanceMethods::REDACTED, Audited::Auditor::AuditedInstanceMethods::REDACTED]

ah, now I saw the []

Audit create/revoke sessions

4ea6e2d

jlledom self-assigned this Sep 10, 2024

jlledom requested review from mayorova and akostadinov September 11, 2024 07:29

mayorova reviewed Sep 11, 2024

View reviewed changes

app/controllers/provider/admin/user/personal_details_controller.rb Outdated Show resolved Hide resolved

akostadinov reviewed Sep 11, 2024

View reviewed changes

app/controllers/provider/admin/user/personal_details_controller.rb Show resolved Hide resolved

akostadinov reviewed Sep 11, 2024

View reviewed changes

app/controllers/provider/sessions_controller.rb Outdated Show resolved Hide resolved

akostadinov reviewed Sep 11, 2024

View reviewed changes

lib/developer_portal/app/controllers/developer_portal/login_controller.rb Show resolved Hide resolved

akostadinov reviewed Sep 11, 2024

View reviewed changes

lib/developer_portal/app/controllers/developer_portal/login_controller.rb Outdated Show resolved Hide resolved

jlledom requested review from mayorova and akostadinov September 11, 2024 15:24

mayorova reviewed Sep 17, 2024

View reviewed changes

akostadinov reviewed Sep 17, 2024

View reviewed changes

akostadinov previously approved these changes Sep 17, 2024

View reviewed changes

jlledom dismissed akostadinov’s stale review via 7607ae0 September 19, 2024 12:03

akostadinov reviewed Sep 19, 2024

View reviewed changes

jlledom requested review from mayorova and akostadinov September 19, 2024 13:03

akostadinov approved these changes Sep 19, 2024

View reviewed changes

jlledom added 4 commits September 19, 2024 17:39

Audit User password_digest

4460fce

Audit log when login fails

37cd6aa

Audit logs to password change fail

b3802b7

Add tests for sessions audit

d09a4dd

jlledom force-pushed the THREESCALE-10843-user-session-audited branch from 5f93b2c to d09a4dd Compare September 19, 2024 15:43

jlledom merged commit fd324cc into 3scale:master Sep 19, 2024
16 of 21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

THREESCALE-10843: Audit user sessions #3885

THREESCALE-10843: Audit user sessions #3885

jlledom commented Sep 10, 2024

akostadinov Sep 11, 2024

jlledom Sep 11, 2024

akostadinov Sep 11, 2024

jlledom Sep 16, 2024

jlledom Sep 17, 2024

akostadinov Sep 11, 2024

jlledom Sep 11, 2024

akostadinov Sep 11, 2024

jlledom Sep 16, 2024

akostadinov Sep 17, 2024

akostadinov Sep 17, 2024

jlledom Sep 17, 2024

jlledom Sep 17, 2024

mayorova Sep 17, 2024

jlledom Sep 17, 2024

mayorova Sep 17, 2024

jlledom Sep 17, 2024

mayorova Sep 17, 2024

jlledom Sep 17, 2024

akostadinov Sep 17, 2024

akostadinov Sep 17, 2024

jlledom Sep 19, 2024

jlledom Sep 19, 2024

akostadinov left a comment

akostadinov Sep 19, 2024

jlledom Sep 19, 2024

jlledom Sep 19, 2024

jlledom Sep 19, 2024

jlledom commented Sep 19, 2024

akostadinov Sep 19, 2024

jlledom Sep 19, 2024

akostadinov Sep 19, 2024

	audited except: %i[salt posts_count janrain_identifier cas_identifier password_digest
	authentication_id open_id last_login_at last_login_ip crypted_password].freeze

	def just_changed_password?
	saved_change_to_password_digest? \|\| super
	end

		expected = [Audited::Auditor::AuditedInstanceMethods::REDACTED] * 2
		assert_equal expected,user.audits.last.audited_changes['password_digest']

THREESCALE-10843: Audit user sessions #3885

THREESCALE-10843: Audit user sessions #3885

Conversation

jlledom commented Sep 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akostadinov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlledom commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment