Add security event log #4367

segiddins · 2024-01-19T00:40:35Z

Targets #4406
Implements https://docs.google.com/document/d/1ApREh07FzNFOtJnEkdvI1owWIQ-K2lIRML1z1jvs7eE/edit
Closes #4360

codecov · 2024-01-19T01:04:17Z

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (a3467f8) 96.99% compared to head (2552351) 97.06%.

Files	Patch %	Lines
...s/events/rubygem_event/version/pushed_component.rb	81.81%	2 Missing ⚠️
app/avo/fields/event_additional_field.rb	94.44%	1 Missing ⚠️
app/avo/fields/global_id_field.rb	87.50%	1 Missing ⚠️
app/avo/resources/ip_address_resource.rb	92.30%	1 Missing ⚠️
app/policies/events/rubygem_event_policy.rb	87.50%	1 Missing ⚠️
app/policies/events/user_event_policy.rb	87.50%	1 Missing ⚠️
app/policies/ip_address_policy.rb	87.50%	1 Missing ⚠️
...views/components/events/table_details_component.rb	95.45%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4367      +/-   ##
==========================================
+ Coverage   96.99%   97.06%   +0.06%     
==========================================
  Files         349      383      +34     
  Lines        7695     8166     +471     
==========================================
+ Hits         7464     7926     +462     
- Misses        231      240       +9

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

segiddins · 2024-01-31T09:23:43Z

app/models/events/rubygem_event.rb

+
+  include Events::Tags
+
+  VERSION_PUSHED = define_event "rubygem:version:pushed" do


Review: rubygem event definitions

segiddins · 2024-01-31T09:23:52Z

app/models/events/user_event.rb

+
+  include Events::Tags
+
+  LOGIN_SUCCESS = define_event "user:login:success" do


Review: user event definitions

martinemde

Partial review. Will come back soon.

8000

martinemde · 2024-02-01T18:37:51Z

app/controllers/sessions_controller.rb

      render "multifactor_auths/prompt"
    else
-      do_login
+      do_login(two_factor_label: nil, two_factor_method: nil, authentication_method: "password")


Maybe we could use a constant for the magic strings in here to keep it consistent. Minor nitpick.

app/models/api_key.rb

app/models/events/recordable.rb

indirect

This looks good to me at a high level, I'm sure we'll discover things that need adjusting in the future but I'm happy to start here.

martinemde

I'm most focused on the way this is modeled, serialized and deserialized. I think the underlying storage is ok. I'm just thinking about ways we might be able to make the model a little simpler. However, I think it's possible to refactor this later. The storage structure seems simple enough, so not a blocker.

This is quite a feature. I didn't get to look at it all but we can adjust these parts after this is merged.

martinemde · 2024-02-06T03:10:01Z

app/models/events/tags.rb

+    def additional_type
+      tags.fetch(tag, nil)
+    end
+
+    def additional
+      additional_type&.new(super) || super
+    end
+
+    def additional=(value)
+      super(value&.to_h)
+    end


I suggest adding these methods to the module rather than in included do. I think it's better for the method lookup table, so they can be shared by the module instead of unique per singleton.

martinemde · 2024-02-06T03:48:22Z

app/models/events/tags.rb

+  end
+
+  class_methods do
+    def define_event(string, &blk)


Maybe some of this method would be more local in Event::Tag, since it's almost all about that class?

I'm not really seeing what else can be pushed there, since we're dealing with the tags method and setting consts on self

martinemde · 2024-02-06T03:50:50Z

app/models/events/tags.rb

+      tag = Events::Tag.new(string).freeze
+      raise ArgumentError, "Tag #{tag.inspect} already defined" if tags.key?(tag)
+      event = Events::Tag.to_struct(&blk)
+      tags[tag] = event


Could we shift to using the string as key? It's is actually the key since that's how we store and retrieve it from the database. Then deserializing might just be "look it up in the hash table" instead of reconstructing.

except we use the methods on it elsewhere, and then we have to write a custom active record serializer to handle the lookup into the map

Unless I misunderstood, the methods we use are string manipulations of the key. The code would not change much if they were class methods. It would certainly save complexity and remove a lot of low level code.

I could be totally wrong, you spent more time in this code than me. Just noting complexity where I see it.

martinemde

Still approve as is, but I do think this is worth changing if you're willing.

martinemde · 2024-02-08T05:12:07Z

app/models/events/tag.rb

+    values = value.split(":")
+
+    source_type = values[0]
+    subject_type = values[1...-1]&.join(":").presence || source_type
+    action = values[-1]


This is the critical code from Tag, the rest is added to load and dump this class. (beside to_struct which is a pure function)

Without this complexity you add a few methods like Event.subject_type(tag) and then the tag can just be a string.

I think this is worthwhile, but I'm open to other approaches or happy to just let this slide. We could make this change even after it merges because this serializes back to the original string.

martinemde · 2024-02-08T05:18:09Z

app/models/events/tags.rb

+      event = Events::Tag.to_struct(&blk)
+      tags[tag] = event
+
+      const_name = [tag.subject_type == tag.source_type ? nil : tag.subject_type, tag.action].compact.join("_").upcase


Places where we use tag. methods just become class methods for processing the tag string. It's like a global ID.

martinemde

👏

martinemde · 2024-02-09T16:00:51Z

app/models/events/tag.rb

@@ -0,0 +1,21 @@
+module Events::Tag
+  module_function


Oh neat. So this just turns the following functions into singleton functions?

martinemde · 2024-02-09T16:01:47Z

app/models/events/tags.rb

+  end
+
+  class_methods do
+    def define_event(tag, &blk)


This all looks great. Thanks for hearing me out. You got it even simpler than I had imagined.

segiddins force-pushed the segiddins/event-log branch from f2d0ea1 to 8005337 Compare January 19, 2024 00:57

segiddins force-pushed the segiddins/event-log branch 8 times, most recently from 07a43a9 to f76d884 Compare January 30, 2024 05:01

This was referenced Jan 30, 2024

Add records for user & rubygem event logging #4406

Merged

Add lookbook & component tests #4407

Merged

segiddins changed the base branch from master to segiddins/add-records-for-user-rubygem-event-logging January 30, 2024 19:52

segiddins force-pushed the segiddins/event-log branch from f76d884 to b005e88 Compare January 31, 2024 09:20

segiddins commented Jan 31, 2024

View reviewed changes

segiddins marked this pull request as ready for review January 31, 2024 09:36

segiddins changed the title ~~[WIP] Add security event log~~ Add security event log Jan 31, 2024

segiddins requested review from indirect, martinemde and simi February 1, 2024 06:46

martinemde reviewed Feb 1, 2024

View reviewed changes

Base automatically changed from segiddins/add-records-for-user-rubygem-event-logging to master February 1, 2024 19:24

segiddins force-pushed the segiddins/event-log branch 2 times, most recently from 3a35c17 to 3420808 Compare February 2, 2024 17:51

indirect approved these changes Feb 5, 2024

View reviewed changes

martinemde approved these changes Feb 6, 2024

View reviewed changes

martinemde approved these changes Feb 8, 2024

View reviewed changes

segiddins force-pushed the segiddins/event-log branch from 3420808 to 9fd9c6c Compare February 8, 2024 19:44

segiddins added 2 commits February 8, 2024 23:11

[WIP] Add security event log

0077ac3

Lookbooks for every component

d724ca3

segiddins added 14 commits February 8, 2024 23:11

Handle unknown event tag in avo

0c57488

Fill locales

3de0867

Rename OIDC::BaseModel to ApplicationModel

b14a198

add missing translation

4467588

store geoip_info directly on event

ac634b5

fix preview test

9bf2ef2

fix application model test

7620754

Add missing assertions

7a9bad1

Add missing method

f5c94ff

Fix avo resources

cccffe3

Move methods out of included block

1e79827

Add test coverage for event recording

5f286d0

String tags

95030e0

Clean up test code / make it work on linux

2552351

segiddins force-pushed the segiddins/event-log branch from 9fd9c6c to 2552351 Compare February 9, 2024 07:11

martinemde approved these changes Feb 9, 2024

View reviewed changes

segiddins merged commit fafb028 into master Feb 9, 2024

segiddins deleted the segiddins/event-log branch February 9, 2024 16:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Add security event log #4367

Add security event log #4367

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		include Events::Tags

		VERSION_PUSHED = define_event "rubygem:version:pushed" do


		include Events::Tags

		LOGIN_SUCCESS = define_event "user:login:success" do

Uh oh!

Add security event log #4367

Add security event log #4367

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Codecov Report

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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants