PXP-10250 PXP-10268 OIDC clients expliration #1057
Conversation
Pull Request Test Coverage Report for Build 13173
💛 - Coveralls |
tests/scripting/test_fence-create.py
Outdated
| else: | ||
| expected_expires_at = now + expires_in * 24 * 60 * 60 | ||
| # allow up to 4 seconds variation to account for test execution | ||
| assert saved_client.expires_at <= expected_expires_at + 4000 |
There was a problem hiding this comment.
isn't expires in seconds not ms? I think this should be +- 4 not 4000
There was a problem hiding this comment.
this made me go down a rabbit hole - there was a ~3600s difference. i assumed it was in ms and added 4s
but i found out this only happened for the expires_in=1000 days case. Turns out getting a timestamp is not as easy as now + <n_days> * 24 * 60 * 60. On March 10 2024 there is an additional hour to take into account, I couldn't really figure out why (the time change is supposed to be on March 12). But datetime.timestamp() does the job so i just replaced the manual calculation 😅
There was a problem hiding this comment.
oh shoot, yeah I bet that was fun to look into 😅 time is fun. I'm glad datetime just does everything for us :D
| .all() | ||
| ) | ||
| assert len(client_sa_after) == 0 | ||
|
|
There was a problem hiding this comment.
you should clean up the non-expired clients here too (if that's not done elsewhere)
There was a problem hiding this comment.
the database is emptied after each test so it should be fine:
Line 545 in 6177892
tests/scripting/test_fence-create.py
Outdated
| else: | ||
| expected_expires_at = now + expires_in * 24 * 60 * 60 | ||
| # allow up to 4 seconds variation to account for test execution | ||
| assert client.expires_at <= expected_expires_at + 4000 |
There was a problem hiding this comment.
see above, I think this is 4000 seconds, not 4 seconds
| @@ -184,9 +221,11 @@ class Client(Base, OAuth2ClientMixin): | |||
| _default_scopes = Column(Text) | |||
| _scopes = ["compute", "storage", "user"] | |||
|
|
|||
| expires_at = Column(Integer, nullable=False, default=0) | |||
There was a problem hiding this comment.
how are existing clients getting migrated to this since it's not nullable?
There was a problem hiding this comment.
turns out the client table already has the expires_at column, it was added to match the authlib model. It's already non-nullable and defaults to 0 (so for backwards compatibility, 0=no expiration)
fence/bin/old_migration_script.py
Line 595 in 6177892
There was a problem hiding this comment.
if it already has it, why do we need to add it here?
There was a problem hiding this comment.
it's in the table but it's not declared in the model. looks like it works fine without declaring it, but isn't it better that it's explicit?
There was a problem hiding this comment.
Yeah, it's better. I was just worried if it was necessary to add it here to make things work. But it sounds like if the table already has it, then this is more just for clarity in the future and everything should work fine without needing to run this migration?
There was a problem hiding this comment.
yes it all works fine with no migration 👍
Jira Tickets: PXP-10250 and PXP-10268
Goes with uc-cdis/cloud-automation#2075
New Features
--expires-inparameter to thefence-createclient-createandclient-modifycommands to specify the number of days in which in a client expiresfence-create client-delete-expiredcommand to remove expired OIDC clients and optionally post warnings in Slack