Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Log usage of legacy identities #5893

Merged
merged 1 commit into from
Oct 25, 2024
Merged

Log usage of legacy identities #5893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions app/controllers/auth/omniauth_callbacks_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,8 @@ def find_identity_by_uid
identity = Identity.joins(:user).find_by(user: { first_name: auth_hash.info.first_name, last_name: auth_hash.info.last_name }, provider: provider, identifier_based_on_email: true) if identity.nil?
return nil if identity.nil?

Event.new(event_type: :other, message: 'Office365 user signed in with legacy identifier').save!

Comment on lines +238 to +239
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance logging for Office365 legacy identifier usage.

The current logging implementation could be improved in several ways:

  1. Use a more specific event type instead of :other
  2. Include user context in the event message
  3. Handle event creation failures
  4. 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.

Suggested change
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
}
)

# Update the identifier to the new uid
identity.update(identifier: auth_uid, identifier_based_on_email: false)
elsif provider.class.sym == :smartschool && auth_username.present?
Expand All @@ -249,6 +251,8 @@ def find_identity_by_uid
identity = Identity.joins(:user).find_by(user: { first_name: auth_hash.info.first_name, last_name: auth_hash.info.last_name }, provider: provider, identifier_based_on_username: true) if identity.nil?
return nil if identity.nil?

Event.new(event_type: :other, message: 'Smartschool user signed in with legacy identifier').save!

Comment on lines +254 to +255
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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
     end

Committable suggestion was skipped due to low confidence.

# Update the identifier to the new uid
identity.update(identifier: auth_uid, identifier_based_on_username: false)
end
Expand Down
2 changes: 1 addition & 1 deletion app/models/event.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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 }

belongs_to :user, optional: true

validates :event_type, presence: true
Expand Down
1 change: 1 addition & 0 deletions config/locales/models/en.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,7 @@ en:
permission_change: Change in permission level
exercise_repository: Creation of exercise repository
error: Error
other: Other
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

export:
statuses:
started: Started
Expand Down
1 change: 1 addition & 0 deletions config/locales/models/nl.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ nl:
permission_change: Verandering in toegangsniveau
exercise_repository: Aanmaak van oefeningenrepository
error: Error
other: Overige
export:
statuses:
started: Begonnen
Expand Down