Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
31 changes: 29 additions & 2 deletions app/controllers/slack_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,34 @@ def create
SlackCommand::SailorsLogJob.perform_later(params_hash)
end

def events
return render(json: { challenge: params[:challenge] }) if params[:type] == "url_verification"

if params[:type] == "event_callback" && params.dig(:event, :type) == "user_change"
slack_user = params.dig(:event, :user)
user = User.find_by(slack_uid: slack_user[:id]) if slack_user
SlackProfileSyncJob.perform_later(user.id) if user && slack_profile_changed?(user, slack_user)
end

head :ok
end

private

def slack_profile_changed?(user, slack_user)
profile = slack_user[:profile] || {}
slack_username =
profile[:display_name_normalized].presence ||
profile[:real_name_normalized].presence ||
slack_user[:name].presence
slack_avatar_url = profile[:image_192].presence || profile[:image_72].presence
slack_email = profile[:email].to_s.strip.downcase.presence

(slack_username.present? && slack_username != user.slack_username) ||
(slack_avatar_url.present? && slack_avatar_url != user.slack_avatar_url) ||
(slack_email.present? && slack_email != user.email_addresses.source_slack.pick(:email))
end

def params_hash
@params_hash ||= params.permit(:command, :text, :response_url, :user_id, :team_id, :team_domain,
:channel_id, :channel_name, :user_name, :trigger_word).to_h
Expand All @@ -32,10 +58,11 @@ def params_hash
def verify_slack_request
return true if Rails.env.development?

signing_secret = ENV["SAILORS_LOG_SLACK_SIGNING_SECRET"]
signing_secret_name = action_name == "events" ? "SLACK_SIGNING_SECRET" : "SAILORS_LOG_SLACK_SIGNING_SECRET"
signing_secret = ENV[signing_secret_name]
if signing_secret.blank?
# we will never hit this in prod but this is good prep for `config.saas_mode`
Rails.logger.error "[SlackController] SAILORS_LOG_SLACK_SIGNING_SECRET is not configured"
Rails.logger.error "[SlackController] #{signing_secret_name} is not configured"
return head(:unauthorized)
end

Expand Down
2 changes: 1 addition & 1 deletion app/jobs/slack_profile_sync_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ def perform(user_id)
polynomial_delay = executions**4 + (Kernel.rand * executions**4 * 0.15) + 2
retry_job(wait: [ e.retry_after, polynomial_delay ].max.seconds)
rescue => e
report_error(e, message: "Failed to update Slack username and avatar for user #{user_id}")
report_error(e, message: "Failed to update Slack profile for user #{user_id}")
raise
end
end
24 changes: 13 additions & 11 deletions app/models/concerns/oauth_authentication.rb
Original file line number Diff line number Diff line change
Expand Up @@ -97,17 +97,19 @@ def from_slack_token(code, redirect_uri, ip_address = nil)
u.email_addresses << email_address unless u.email_addresses.include?(email_address)
end

user.email_addresses.source_slack.where.not(email: email).update_all(source: :signing_in)
email_address.source = :slack
email_address.save! if email_address.persisted?

user.slack_uid = data.dig("authed_user", "id")
user.apply_slack_profile_attributes(slack_user)
user.parse_and_set_timezone(slack_user["tz"])
user.slack_access_token = data["authed_user"]["access_token"]
user.slack_scopes = data["authed_user"]["scope"]&.split(/,\s*/)
user.country_code = country_code_from_ip(ip_address) if user.country_code.blank?
user.save!
User.transaction do
user.email_addresses.source_slack.where.not(email: email).destroy_all

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 OAuth deletes another account's emails

If the Slack email is already attached to a different Hackatime user, email_address.user selects that user and this destroy_all permanently removes their other Slack-sourced email records before replacing their Slack credentials.

Knowledge Base Used: Authentication: sessions, API keys, and OAuth

Prompt To Fix With AI
This is a comment left during a code review.
Path: app/models/concerns/oauth_authentication.rb
Line: 101

Comment:
**OAuth deletes another account's emails**

If the Slack email is already attached to a different Hackatime user, `email_address.user` selects that user and this `destroy_all` permanently removes their other Slack-sourced email records before replacing their Slack credentials.

**Knowledge Base Used:** [Authentication: sessions, API keys, and OAuth](https://app.greptile.com/mahadk/-/custom-context/knowledge-base/hackclub/hackatime/-/docs/api-authentication.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

email_address.source = :slack
email_address.save! if email_address.persisted?

user.slack_uid = data.dig("authed_user", "id")
user.apply_slack_profile_attributes(slack_user)
user.parse_and_set_timezone(slack_user["tz"])
user.slack_access_token = data["authed_user"]["access_token"]
user.slack_scopes = data["authed_user"]["scope"]&.split(/,\s*/)
user.country_code = country_code_from_ip(ip_address) if user.country_code.blank?
user.save!
end
user
rescue => e
report_error(e, message: "Error creating user from Slack data: #{e.message}")
Expand Down
21 changes: 21 additions & 0 deletions app/models/concerns/slack_integration.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,12 @@ def initialize(retry_after)
end
end

class EmailConflictError < StandardError
def initialize(email)
super("Slack email is already linked to another Hackatime account: #{email}")
end
end

STATUS_EMOJI_BUCKETS = [
[ 30.minutes, %w[thinking cat-on-the-laptop loading-tumbleweed rac-yap] ],
[ 1.hour, %w[working-parrot meow_code] ],
Expand Down Expand Up @@ -49,6 +55,7 @@ def update_from_slack
return unless user_data.present?

apply_slack_profile_attributes(user_data)
sync_slack_email(user_data.dig("profile", "email"))
self.slack_synced_at = Time.current
end

Expand All @@ -63,6 +70,20 @@ def apply_slack_profile_attributes(slack_user)
slack_user["name"].presence
end

def sync_slack_email(raw_email)
email = raw_email.to_s.strip.downcase.presence
return unless email

email_address = EmailAddress.find_by(email: email)
raise EmailConflictError, email if email_address && email_address.user_id != id

transaction do
email_address ||= email_addresses.create!(email: email, source: :slack)
email_addresses.source_slack.where.not(id: email_address.id).destroy_all
email_address.update!(source: :slack) unless email_address.source_slack?
end
Comment on lines +80 to +84

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Email reconciliation commits before profile

When user.save! fails after a Slack email change, this inner transaction has already committed the replacement email and deleted the former email, leaving authentication records updated while the profile fields and slack_synced_at remain unsaved.

Knowledge Base Used: Authentication: sessions, API keys, and OAuth

Prompt To Fix With AI
This is a comment left during a code review.
Path: app/models/concerns/slack_integration.rb
Line: 80-84

Comment:
**Email reconciliation commits before profile**

When `user.save!` fails after a Slack email change, this inner transaction has already committed the replacement email and deleted the former email, leaving authentication records updated while the profile fields and `slack_synced_at` remain unsaved.

**Knowledge Base Used:** [Authentication: sessions, API keys, and OAuth](https://app.greptile.com/mahadk/-/custom-context/knowledge-base/hackclub/hackatime/-/docs/api-authentication.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

end

def update_slack_status
return unless uses_slack_status?

Expand Down
1 change: 1 addition & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ def matches?(request)
get "my/wakatime_setup/step-4", to: redirect("/setup")

post "/sailors_log/slack/commands", to: "slack#create"
post "/slack/events", to: "slack#events"

get "/hackatime/v1", to: redirect("/", status: 302) # some clients seem to link this as the user's dashboard instead of /api/v1/hackatime
# API routes
Expand Down
4 changes: 4 additions & 0 deletions slack_manifest_harbor.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ oauth_config:
- users:read
- users:read.email
settings:
event_subscriptions:
request_url: https://hackatime.hackclub.com/slack/events
user_events:
- user_change
org_deploy_enabled: false
socket_mode_enabled: false
token_rotation_enabled: false
132 changes: 132 additions & 0 deletions spec/requests/slack_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,136 @@
end
end
end

path '/slack/events' do
post('Handle Slack Events') do
tags 'Slack'
description 'Handle Slack Events API callbacks for the Hackatime Slack app.'
consumes 'application/json'
produces 'application/json'

parameter name: :event_payload, in: :body, schema: {
type: :object,
properties: {
type: { type: :string },
challenge: { type: :string },
event: { type: :object }
}
}

response(200, 'successful', document: false) do
let(:event_payload) { { type: 'url_verification', challenge: 'challenge-token' } }
before { allow(Rails.env).to receive(:development?).and_return(true) }
run_test!
end
end
end

describe 'POST /slack/events' do
include ActiveJob::TestHelper

let(:signing_secret) { 'signing-secret' }
let(:timestamp) { Time.current.to_i.to_s }

around do |example|
original_secret = ENV['SLACK_SIGNING_SECRET']
ENV['SLACK_SIGNING_SECRET'] = signing_secret
example.run
ensure
ENV['SLACK_SIGNING_SECRET'] = original_secret
end

before do
ActiveJob::Base.queue_adapter = :test
clear_enqueued_jobs
end

def post_signed_event(payload)
body = payload.to_json
signature = 'v0=' + OpenSSL::HMAC.hexdigest('SHA256', signing_secret, "v0:#{timestamp}:#{body}")
post '/slack/events', params: body, headers: {
'CONTENT_TYPE' => 'application/json',
'X-Slack-Request-Timestamp' => timestamp,
'X-Slack-Signature' => signature
}
end

it 'responds to Slack URL verification' do
post_signed_event(type: 'url_verification', challenge: 'challenge-token')

expect(response).to have_http_status(:ok)
expect(response.parsed_body).to eq('challenge' => 'challenge-token')
end

it 'enqueues a profile sync when the Slack email changes' do
user = User.create!(
timezone: 'UTC',
slack_uid: 'U_EVENT_USER',
slack_username: 'old-name',
slack_avatar_url: 'https://example.com/old.png'
)
user.email_addresses.create!(email: 'old@example.com', source: :slack)

expect {
post_signed_event(
type: 'event_callback',
event_id: 'EvProfileChanged',
event: {
type: 'user_change',
user: {
id: user.slack_uid,
name: 'fallback-name',
profile: {
display_name_normalized: user.slack_username,
image_192: user.slack_avatar_url,
email: 'new@example.com'
}
}
}
)
}.to have_enqueued_job(SlackProfileSyncJob).with(user.id)

expect(response).to have_http_status(:ok)
end

it 'ignores a user_change event when only unrelated profile data changed' do
user = User.create!(
timezone: 'UTC',
slack_uid: 'U_STATUS_USER',
slack_username: 'same-name',
slack_avatar_url: 'https://example.com/same.png'
)

expect {
post_signed_event(
type: 'event_callback',
event_id: 'EvStatusChanged',
event: {
type: 'user_change',
user: {
id: user.slack_uid,
name: 'fallback-name',
profile: {
display_name_normalized: user.slack_username,
image_192: user.slack_avatar_url,
status_text: 'Coding'
}
}
}
)
}.not_to have_enqueued_job(SlackProfileSyncJob)

expect(response).to have_http_status(:ok)
end

it 'rejects requests with an invalid Slack signature' do
post '/slack/events', params: { type: 'event_callback' }.to_json, headers: {
'CONTENT_TYPE' => 'application/json',
'X-Slack-Request-Timestamp' => timestamp,
'X-Slack-Signature' => 'v0=invalid'
}

expect(response).to have_http_status(:unauthorized)
end
end
end
61 changes: 61 additions & 0 deletions test/jobs/slack_profile_sync_job_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,67 @@ class SlackProfileSyncJobTest < ActiveJob::TestCase
assert_not_nil user.slack_synced_at
end

test "reconciles the Slack email without preserving the previous email for sign-in" do
user = User.create!(timezone: "UTC", slack_uid: "U_EMAIL_SYNC")
old_email = user.email_addresses.create!(email: "old@example.com", source: :slack)
stub_request(:get, "https://slack.com/api/users.info?user=U_EMAIL_SYNC")
.with(headers: { "Authorization" => "Bearer workspace-token" })
.to_return(body: {
ok: true,
user: {
name: "email-sync",
profile: { email: "New@Example.com" }
}
}.to_json)

SlackProfileSyncJob.perform_now(user.id)

assert_not EmailAddress.exists?(old_email.id)
assert_predicate user.email_addresses.find_by!(email: "new@example.com"), :source_slack?
end

test "promotes an already-linked sign-in email when Slack changes to it" do
user = User.create!(timezone: "UTC", slack_uid: "U_EXISTING_EMAIL_SYNC")
old_email = user.email_addresses.create!(email: "old@example.com", source: :slack)
new_email = user.email_addresses.create!(email: "new@example.com", source: :signing_in)
stub_request(:get, "https://slack.com/api/users.info?user=U_EXISTING_EMAIL_SYNC")
.to_return(body: {
ok: true,
user: {
name: "existing-email-sync",
profile: { email: "new@example.com" }
}
}.to_json)

SlackProfileSyncJob.perform_now(user.id)

assert_not EmailAddress.exists?(old_email.id)
assert_predicate new_email.reload, :source_slack?
end

test "does not claim a Slack email linked to another account" do
user = User.create!(timezone: "UTC", slack_uid: "U_EMAIL_CONFLICT")
old_email = user.email_addresses.create!(email: "old@example.com", source: :slack)
other_user = User.create!(timezone: "UTC")
other_email = other_user.email_addresses.create!(email: "taken@example.com", source: :signing_in)
stub_request(:get, "https://slack.com/api/users.info?user=U_EMAIL_CONFLICT")
.to_return(body: {
ok: true,
user: {
name: "email-conflict",
profile: { email: "taken@example.com" }
}
}.to_json)

assert_raises(SlackIntegration::EmailConflictError) do
SlackProfileSyncJob.perform_now(user.id)
end

assert_predicate old_email.reload, :source_slack?
assert_equal other_user, other_email.reload.user
assert_nil user.reload.slack_synced_at
end

test "retries Slack rate limits without changing the existing profile" do
user = User.create!(
timezone: "UTC",
Expand Down
Loading