Added Audited to Proxy Class #3226
Conversation
app/services/audit_log_service.rb
Outdated
| def log(message) | ||
| Rails.logger.info "[AUDIT]: #{message}" | ||
| end | ||
| end |
There was a problem hiding this comment.
This looks good. Please follow a service class pattern as described here:
https://github.com/selleo/pattern#service
You basically need to include Callable and then log like AuditLogService.call(my_message).
| :error_headers_auth_missing, :error_headers_no_match, | ||
| :error_auth_failed, :error_auth_missing, :error_no_match, | ||
| :error_headers_limits_exceeded, :error_limits_exceeded, | ||
| format: { with: HTTP_HEADER } |
There was a problem hiding this comment.
please avoid just formatting changes even if formatting is not ideal to make review easier and git blame easier to follow
app/models/proxy.rb
Outdated
| @@ -114,6 +114,8 @@ class Proxy < ApplicationRecord # rubocop:disable Metrics/ClassLength | |||
| delegate :backend_apis, :backend_api_configs, to: :service | |||
| delegate :scheduled_for_deletion?, to: :account, allow_nil: true | |||
|
|
|||
| audited allow_mass_assignment: true | |||
There was a problem hiding this comment.
I assume this is where trouble comes from. It is probably because we are using a custom type PoliciesConfig and this somehow confuses audoted. I need to investigate that tomorrow.
There was a problem hiding this comment.
@RobGri001 , still not sure why this happens in the first place. Seems to be the way rails or psych does serialization or how audited uses that. But there is a workaround that should get you going for the time being.
--- a/app/models/attributes/policies_config.rb
+++ b/app/models/attributes/policies_config.rb
@@ -13,3 +13,5 @@ class Attributes::PoliciesConfig < ActiveRecord::Type::Text
new_value != cast(raw_old_value)
end
end
+
+PoliciesConfig = Attributes::PoliciesConfigAdd this line and it should stop failing. I'll check later whether a better fix will be available.
There was a problem hiding this comment.
Issue is easy to reproduce and is IMO some bug in Psych
1] pry(main)> pc = Proxy.last.policies_config
=> #<Proxy::PoliciesConfig:0x000000000d074358
@policies_config=
[#<Proxy::PolicyConfig:0x000000000d07be50
@configuration={},
@enabled=true,
@name="apicast",
@version="builtin">,
#<Proxy::PolicyConfig:0x000000000d07be00
@configuration={},
@enabled=true,
@name="3scale_referrer",
@version="builtin">]>
[2] pry(main)> ser = pc.to_yaml
=> "--- !ruby/object:PoliciesConfig\npolicies_config:\n- !ruby/object:Proxy::PolicyConfig\n name: apicast\n version: builtin\n configuration: {}\n enabled: true\n- !ruby/object:Proxy::PolicyConfig\n name: 3scale_referrer\n version: builtin\n configuration: {}\n enabled: true\n"
[3] pry(main)> YAML.load ser
ArgumentError: undefined class/module PoliciesConfig
from /home/avalon/.asdf/installs/ruby/2.6.7/lib/ruby/2.6.0/psych/class_loader.rb:54:in `path2class'
Caused by ArgumentError: undefined class/module Struct::PoliciesConfig
from /home/avalon/.asdf/installs/ruby/2.6.7/lib/ruby/2.6.0/psych/class_loader.rb:54:in `path2class'This also shows that my above workaround is not ideal. But need more to investigate.
There was a problem hiding this comment.
I think I see where the issue comes from:
Lines 564 to 566 in 962e945
I have zero idea why this was needed. But it obviously is used by Psych as class name
[10] pry(main)> pc.class.name
=> "PoliciesConfig"So one option would be to remove this method. But then something in presentation may break. We can see if any tests would fail. A safer approach might be to extract this class not to be a sub-class of Proxy so this method will not be needed anymore as the class will be at the root layer anyway.
There was a problem hiding this comment.
So obviously the workaround would rather be
PoliciesConfig = Proxy::PoliciesConfigright under the Proxy class definition. Not really a proper fix but should get you going.
There was a problem hiding this comment.
Hello Aleksandar,
we added the "PoliciesConfig = Proxy::PoliciesConfig" line directly under: class PoliciesConfig - in line 548
Because adding the constant definition under the ProxyClass definition causes in an compilation error.
With this change, we could find a log entry in the Database Audits Table.
Also we deleted the lines:
def self.name
'PoliciesConfig'
end
| class AuditLogService | ||
| include Callable | ||
|
|
||
| def self.call(message) |
There was a problem hiding this comment.
| def self.call(message) | |
| def call(message) |
The Callable concern should handle this transparently.
|
This PR is stale because it has not received activity for more than 14 days. Remove stale label or comment or this will be closed in 7 days. |
|
Hi Aleksandar, we just added two unit tests |
| @user, strategy = authenticate_user | ||
|
|
||
| if @user | ||
| self.current_user = @user | ||
| create_user_session!(strategy.authentication_provider_id) | ||
| flash[:notice] = 'Signed in successfully' | ||
|
|
||
| AuditLogService.call("Signed in: #{self.current_user.username} #{self.current_user.id} #{self.current_user.first_name} #{self.current_user.last_name}") |
There was a problem hiding this comment.
I wonder if it makes sense to log both - successful and unsuccessful login attempts.
|
Migrating changes from here to #3342 |
Hello all,
we like to extend the proxy class with an audited annotation. So we changed the proxy.rb File.
This caused in an exception, as we assigned a policy. Please have a look at the screenshot.
This agreed with Alexander Kostatinov for help with the analyses.
Best Regards
Special notes for your reviewer:
More Informations: https://issues.redhat.com/browse/THREESCALE-6853