-
Notifications
You must be signed in to change notification settings - Fork 72
THREESCALE-14969: Rotate OIDC tokens for Zync #4310
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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]) } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 But it's in reality:
Maybe at least smth like
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 😅 Mmm, OK, makes more sense now, but maybe we can at least leave a comment explaining the name choice?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What should this mean 😬
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. full resync (#4307) is a scenario when concurrent cache misses can happen
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. :unacceptable: |
||
| lock.where(name: OIDC_SYNC_TOKEN).load | ||
|
jlledom marked this conversation as resolved.
|
||
|
|
||
| Rails.cache.fetch(cache_key, expires_in: 1.hour) do | ||
|
jlledom marked this conversation as resolved.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so cool 🫠