Log usage of legacy identities #5893
Conversation
bmesuere
changed the title
WalkthroughThe changes in this pull request involve modifications to user authentication logging, updates to event types in the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
app/models/event.rb (2)
Line range hint
14-21: Consider adding scopes to support legacy provider analysis.To facilitate the identification of unused providers after one month, consider adding specific scopes for date-based queries and provider analysis.
Add these scopes to the model:
# Scope to find events within the last month scope :within_last_month, -> { where('created_at >= ?', 1.month.ago) } # Scope to find legacy provider usage events scope :legacy_provider_usage, -> { where(event_type: :legacy_provider_usage) } # Combined scope for analyzing provider usage scope :unused_legacy_providers, -> { legacy_provider_usage .where('created_at < ?', 1.month.ago) .group(:message) .having('MAX(created_at) < ?', 1.month.ago) }
Line range hint
1-13: Consider enhancing the schema for better provider tracking.The current schema might limit the ability to effectively track provider usage. The
messagefield asstring(255)stores the provider information, but this makes it harder to query and analyze specific providers.Consider adding dedicated fields for provider tracking:
- Add a
providerfield to store the provider name- Add a
provider_metadataJSON field for additional tracking data- Index the
providerfield for better query performanceExample migration:
class EnhanceEventsForProviderTracking < ActiveRecord::Migration[7.0] def change add_column :events, :provider, :string add_column :events, :provider_metadata, :jsonb, default: {} add_index :events, :provider add_index :events, :created_at # For date-based queries end end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- app/controllers/auth/omniauth_callbacks_controller.rb (2 hunks)
- app/models/event.rb (1 hunks)
- config/locales/models/en.yml (1 hunks)
- config/locales/models/nl.yml (1 hunks)
🔇 Additional comments (2)
config/locales/models/nl.yml (1)
179-179: LGTM! Verify the event type usage.The addition of "other: Overige" as a new event type aligns with the PR objective of logging legacy provider usage. The Dutch translation is appropriate.
Let's verify the usage of this new event type in the codebase:
✅ Verification successful
Let me check the actual content of these files to verify the proper integration of the event type.
Event type integration verified and properly used
The new event type
:otheris correctly integrated:
- Defined in the Event model's enum with value 4
- Used appropriately in the authentication controller to log legacy sign-in attempts for both Office365 and Smartschool providers
- Has matching translations in both Dutch ("Overige") and English ("Other")
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the new event type is properly integrated # Expected: Find related event type definition and its usage # Check for event type definition in models echo "Checking event type definition..." rg -l "other" app/models/event.rb # Check for usage in the authentication controller echo "Checking usage in auth controller..." rg -l "other" app/controllers/auth/omniauth_callbacks_controller.rb # Verify English translation exists echo "Checking English translation..." rg "other:" config/locales/models/en.ymlLength of output: 472
Script:
#!/bin/bash # Check the event type definition in the model echo "=== Event model definition ===" rg -A 5 -B 5 "other" app/models/event.rb # Check how it's used in the controller echo -e "\n=== Controller usage ===" rg -A 5 -B 5 "other" app/controllers/auth/omniauth_callbacks_controller.rbLength of output: 2921
app/controllers/auth/omniauth_callbacks_controller.rb (1)
Line range hint
238-255: Consider adding analytics support for legacy provider usage.The current implementation logs usage but doesn't fully support the stated goal of removing unused legacy providers after one month of inactivity. Consider these architectural improvements:
- Add timestamp tracking in the event data
- Create database indexes for efficient querying
- Add a background job to analyze usage patterns
- Implement automated notifications for unused providers
Let's verify if the Event model supports the required functionality:
Would you like me to propose a detailed implementation for these improvements?
| @@ -11,7 +11,7 @@ | |||
| # | |||
|
|
|||
| class Event < ApplicationRecord | |||
| enum :event_type, { rejudge: 0, permission_change: 1, exercise_repository: 2, error: 3 } | |||
| enum :event_type, { rejudge: 0, permission_change: 1, exercise_repository: 2, error: 3, other: 4 } | |||
There was a problem hiding this comment.
Consider using a more specific event type for legacy provider logging.
Using 'other' as the event type for legacy provider usage might make it difficult to track and analyze this specific use case. Consider adding a more descriptive event type like 'legacy_provider_usage' to make the intention clear and facilitate easier querying.
Apply this diff:
- enum :event_type, { rejudge: 0, permission_change: 1, exercise_repository: 2, error: 3, other: 4 }
+ enum :event_type, { rejudge: 0, permission_change: 1, exercise_repository: 2, error: 3, legacy_provider_usage: 4 }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| enum :event_type, { rejudge: 0, permission_change: 1, exercise_repository: 2, error: 3, other: 4 } | |
| enum :event_type, { rejudge: 0, permission_change: 1, exercise_repository: 2, error: 3, legacy_provider_usage: 4 } |
| @@ -175,6 +175,7 @@ en: | |||
| permission_change: Change in permission level | |||
| exercise_repository: Creation of exercise repository | |||
| error: Error | |||
| other: Other | |||
There was a problem hiding this comment.
Consider using specific event types instead of "other" for tracking legacy providers.
The addition of a generic "other" event type may not align well with the PR's objective of tracking legacy sign-in method usage. To effectively monitor which providers remain unused for a month (as mentioned in PR #5791), consider using specific event types for each legacy provider instead.
Replace the generic "other" with specific provider event types:
- other: Other
+ legacy_saml_signin: "Legacy SAML Sign-in"
+ legacy_office365_signin: "Legacy Office 365 Sign-in"
+ legacy_smartschool_signin: "Legacy Smartschool Sign-in"
+ legacy_google_signin: "Legacy Google Sign-in"Committable suggestion was skipped due to low confidence.
| Event.new(event_type: :other, message: 'Office365 user signed in with legacy identifier').save! | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance logging for Office365 legacy identifier usage.
The current logging implementation could be improved in several ways:
- Use a more specific event type instead of
:other - Include user context in the event message
- Handle event creation failures
- Add structured data for better analytics
Consider this implementation:
- Event.new(event_type: :other, message: 'Office365 user signed in with legacy identifier').save!
+ Event.create!(
+ event_type: :legacy_provider_usage,
+ message: "User #{identity.user.email} signed in with Office365 legacy identifier",
+ data: {
+ provider: 'office365',
+ legacy_identifier: identity.identifier,
+ new_identifier: auth_uid,
+ user_id: identity.user_id
+ }
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Event.new(event_type: :other, message: 'Office365 user signed in with legacy identifier').save! | |
| Event.create!( | |
| event_type: :legacy_provider_usage, | |
| message: "User #{identity.user.email} signed in with Office365 legacy identifier", | |
| data: { | |
| provider: 'office365', | |
| legacy_identifier: identity.identifier, | |
| new_identifier: auth_uid, | |
| user_id: identity.user_id | |
| } | |
| ) |
| Event.new(event_type: :other, message: 'Smartschool user signed in with legacy identifier').save! | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract duplicated legacy provider logging logic.
The logging code is duplicated between Office365 and Smartschool providers. This should be extracted into a reusable method.
Consider extracting the logging logic:
+ def log_legacy_identifier_usage(identity, provider_name)
+ Event.create!(
+ event_type: :legacy_provider_usage,
+ message: "User #{identity.user.email} signed in with #{provider_name} legacy identifier",
+ data: {
+ provider: provider_name.downcase,
+ legacy_identifier: identity.identifier,
+ new_identifier: auth_uid,
+ user_id: identity.user_id
+ }
+ )
+ end
def find_identity_by_uid
# ... existing code ...
if provider.class.sym == :office365 && auth_email.present?
# ... existing code ...
return nil if identity.nil?
- Event.new(event_type: :other, message: 'Office365 user signed in with legacy identifier').save!
+ log_legacy_identifier_usage(identity, 'Office365')
# ... existing code ...
elsif provider.class.sym == :smartschool && auth_username.present?
# ... existing code ...
return nil if identity.nil?
- Event.new(event_type: :other, message: 'Smartschool user signed in with legacy identifier').save!
+ log_legacy_identifier_usage(identity, 'Smartschool')
# ... existing code ...
end
identity
endCommittable suggestion was skipped due to low confidence.
This pull request adds logging of usage of legacy sign in methods. If they remain unused for at least 1 month, they will be removed by #5791