Skip to content
Merged
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
19 changes: 18 additions & 1 deletion app/models/access_token.rb
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,25 @@ def self.find_from_id_or_value(id_or_value)
# This can't change or it will create new tokens for everyone
OIDC_SYNC_TOKEN = 'OIDC Synchronization Token'.freeze

# nil covers legacy tokens created before expiration was added; can be removed after a release
scope :stale, -> { where(name: OIDC_SYNC_TOKEN, expires_at: [...Time.now.utc, nil]) }

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.

so cool 🫠

@mayorova mayorova Jun 3, 2026

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.

Can we find a better name for this scope please? 🙃

If I see AccessToken.stale, I would assume that it's any expired token 🙃

But it's in reality:

  • only tokens related to OIDC sync
  • including non-expirable tokens

Maybe at least smth like stale_oidc_sync ?

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.

UPDATE: ah, as I progressed with the review, I understood why you chose this name 😅
That's to make DeleteAllStaleObjectsWorker pick these objects automatically...

Mmm, OK, makes more sense now, but maybe we can at least leave a comment explaining the name choice?

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.

stale is a pretty meaningful name IMO :) Objects that will not be used and hold no value for anybody. We do not do this to user created tokens because users may wish to see what they previously had.

If you think a comment will help, please suggest one, rather than wait for the ai to add something meaningless 👼


def self.oidc_sync
create_with(scopes: %w[account_management], permission: 'ro').find_or_create_by!(name: OIDC_SYNC_TOKEN)
user_id = scope_attributes["owner_id"]
cache_key = "access_tokens/user:#{user_id}/oidc"

# Hot path: skip the transaction entirely on cache hit (zero DB queries).
cached = Rails.cache.read(cache_key)
return cached if cached

transaction do
# Lock to serialize concurrent cache misses (e.g. full resync).

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.

(e.g. full resync)

What should this mean 😬

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

full resync (#4307) is a scenario when concurrent cache misses can happen

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.

IMO the comment can be removed. It brought only confusion to me, also it is clear that the lock prevents concurrent refreshes. But leaving up to you.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I prefer to keep the comment, I think it'll help me in the future if I come back to this.

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.

:unacceptable:

lock.where(name: OIDC_SYNC_TOKEN).load
Comment thread
jlledom marked this conversation as resolved.

Rails.cache.fetch(cache_key, expires_in: 1.hour) do
Comment thread
jlledom marked this conversation as resolved.

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.

Also, I am wondering - this approach means that at least every hour (given that zync updates happen regularly) a new token will be generated. So, in a week (Janitor runs once a week, right?), there will be about 24*7 tokens generated. Just trying to think if this can be an issue 🙃 I guess not, it's not a huge amount.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's actually 24 x 7 per provider, so it can be a lot.

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.

Do zync updates happen regularly in a normal situation?

Btw the cache survival for 1 hour is not a hard guarantee.

create!(name: OIDC_SYNC_TOKEN, scopes: %w[account_management], permission: 'ro', expires_at: 1.day.from_now.utc.iso8601).plaintext_value
end
end
end

def scopes=(values)
Expand Down
2 changes: 1 addition & 1 deletion app/workers/janitor_worker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ def perform

PurgeOldUserSessionsWorker.perform_async
PurgeStaleObjectsWorker.perform_later(EventStore::Event.name, DeletedObject.name)
DeleteAllStaleObjectsWorker.perform_later(MessageRecipient.name, Message.name)
DeleteAllStaleObjectsWorker.perform_later(MessageRecipient.name, Message.name, AccessToken.name)

true
end
Expand Down
2 changes: 1 addition & 1 deletion app/workers/zync_worker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ def provider_endpoint(provider)
def self.provider_access_token(provider)
user = provider.find_impersonation_admin || provider.first_admin!

user.access_tokens.oidc_sync.value
user.access_tokens.oidc_sync
end

def notification_url
Expand Down
140 changes: 140 additions & 0 deletions test/models/access_token_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,146 @@ def test_find_from_value_rejects_leaked_hash_as_token
assert access_token.valid?
end

# oidc_sync tests

class RefreshOidcSyncTest < ActiveSupport::TestCase
setup do
@original_cache = Rails.cache
Rails.cache = ActiveSupport::Cache::MemoryStore.new
@provider = FactoryBot.create(:simple_provider)
@user = FactoryBot.create(:admin, account: @provider)
end

teardown do
Rails.cache.clear
Rails.cache = @original_cache
end

test 'returns a plaintext string' do
result = @user.access_tokens.oidc_sync

assert_instance_of String, result
assert_equal 96, result.length
assert_match(/\A[0-9a-f]+\z/, result)
end

test 'creates an OIDC sync token in DB whose digest matches the returned plaintext' do
plaintext = @user.access_tokens.oidc_sync

token = @user.access_tokens.find_by!(name: AccessToken::OIDC_SYNC_TOKEN)
assert_equal AccessToken.compute_digest(plaintext), token.read_attribute(:value)
end

test 'second call returns same plaintext from cache without rotating' do
first = @user.access_tokens.oidc_sync
second = @user.access_tokens.oidc_sync

assert_equal first, second
assert_equal 1, @user.access_tokens.where(name: AccessToken::OIDC_SYNC_TOKEN).count
end

test 'returns a new plaintext after cache expires and does not modify old tokens' do
first = @user.access_tokens.oidc_sync
old_token = @user.access_tokens.find_by!(name: AccessToken::OIDC_SYNC_TOKEN)
old_expires_at = old_token.expires_at

travel_to(1.hour.from_now + 1.second) do
second = @user.access_tokens.oidc_sync

assert_not_equal first, second
assert_equal old_expires_at, old_token.reload.expires_at, "Old token should not be modified"
assert_equal 2, @user.access_tokens.where(name: AccessToken::OIDC_SYNC_TOKEN).count
end
end

test 'new tokens are created with a 1-day expiration' do
@user.access_tokens.oidc_sync

token = @user.access_tokens.find_by!(name: AccessToken::OIDC_SYNC_TOKEN)
assert token.expires_at.present?
assert token.expires_at > Time.now.utc, "Token should not be expired yet"
assert token.expires_at < Time.now.utc + 1.day + 1.minute, "Token should expire within ~1 day"
end

test 'expired tokens remain valid for authentication' do
plaintext = @user.access_tokens.oidc_sync

travel_to(1.hour.from_now + 1.second) do
@user.access_tokens.oidc_sync

old_token = AccessToken.find_from_value(plaintext)
assert_not_nil old_token, "Old token should still be findable by plaintext"
assert_not old_token.expired?, "Old token should not be expired yet (expires in ~1 day)"
end
end

test 'works on first call with no existing tokens' do
assert_equal 0, @user.access_tokens.where(name: AccessToken::OIDC_SYNC_TOKEN).count

result = @user.access_tokens.oidc_sync

assert_instance_of String, result
assert_equal 1, @user.access_tokens.where(name: AccessToken::OIDC_SYNC_TOKEN).count
end

test 'tokens from different users are independent' do
other_provider = FactoryBot.create(:simple_provider)
other_user = FactoryBot.create(:admin, account: other_provider)

plaintext_a = @user.access_tokens.oidc_sync
plaintext_b = other_user.access_tokens.oidc_sync

assert_not_equal plaintext_a, plaintext_b
assert_equal 1, @user.access_tokens.where(name: AccessToken::OIDC_SYNC_TOKEN).count
assert_equal 1, other_user.access_tokens.where(name: AccessToken::OIDC_SYNC_TOKEN).count
end

test 'returns cached value without DB queries on cache hit' do
@user.access_tokens.oidc_sync

assert_number_of_queries(0) { @user.access_tokens.oidc_sync }
end
end

# stale scope tests

test 'stale scope returns only expired OIDC sync tokens' do
provider = FactoryBot.create(:simple_provider)
user = FactoryBot.create(:admin, account: provider)

expired_token = FactoryBot.create(:access_token, owner: user, name: AccessToken::OIDC_SYNC_TOKEN,
scopes: %w[account_management], permission: 'ro')
expired_token.update_columns(expires_at: 1.hour.ago)

active_token = FactoryBot.create(:access_token, owner: user, name: "#{AccessToken::OIDC_SYNC_TOKEN} 2",
scopes: %w[account_management], permission: 'ro')
active_token.update_columns(expires_at: 1.day.from_now)

other_token = FactoryBot.create(:access_token, owner: user)
other_token.update_columns(expires_at: 1.hour.ago)

scope_ids = AccessToken.stale.pluck(:id)
assert_includes scope_ids, expired_token.id
assert_not_includes scope_ids, active_token.id
assert_not_includes scope_ids, other_token.id
end

test 'stale scope includes OIDC sync tokens with nil expires_at (legacy tokens)' do
provider = FactoryBot.create(:simple_provider)
user = FactoryBot.create(:admin, account: provider)

legacy_token = FactoryBot.create(:access_token, owner: user, name: AccessToken::OIDC_SYNC_TOKEN,
scopes: %w[account_management], permission: 'ro')
legacy_token.update_columns(expires_at: nil)

other_legacy_token = FactoryBot.create(:access_token, owner: user)
other_legacy_token.update_columns(expires_at: nil)

scope_ids = AccessToken.stale.pluck(:id)
assert_includes scope_ids, legacy_token.id
assert_not_includes scope_ids, other_legacy_token.id
end

private

def assert_access_token_audit_all_data(access_token, audit)
Expand Down
2 changes: 1 addition & 1 deletion test/workers/janitor_worker_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,6 @@ def test_not_enabled
def mock_purge_workers(called_times:)
PurgeOldUserSessionsWorker.expects(:perform_async).send(called_times)
PurgeStaleObjectsWorker.expects(:perform_later).send(called_times).with(EventStore::Event.name, DeletedObject.name)
DeleteAllStaleObjectsWorker.expects(:perform_later).send(called_times).with(MessageRecipient.name, Message.name)
DeleteAllStaleObjectsWorker.expects(:perform_later).send(called_times).with(MessageRecipient.name, Message.name, AccessToken.name)
end
end