-
Notifications
You must be signed in to change notification settings - Fork 133
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
feat: Add expiration date to access token & hmac keys #1219
base: develop
Are you sure you want to change the base?
feat: Add expiration date to access token & hmac keys #1219
Conversation
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.
Sorry you had to wait so long for feedback.
The PR looks nice, although I have questions about the reasons for some of the "implementation" choices.
Is it possible to generate a token without a time limit? |
Sure! Just skip that argument when generating a token. It will follow the defined token lifetime though. |
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.
@CosDiabos Thank you for the updates. I have a few other comments.
Of other things, I see that one action "Detect Merge Commits" reports an error. I'm not sure because of which commit, but you can try to rebase (there were some changes in the codebase since you started this PR). Or alternatively, if you identify the commit that is interfering, you can try to remove it from this PR - here are quite clear instructions that should help: https://graphite.dev/guides/how-to-delete-a-git-commit#deleting-old-or-specific-commits
@datamweb - is this failed action a deal breaker for us or not really?
@michalsn , commit e6c10c0d65c3360639fe39120d48e3949b740238 was correctly detected by the action. The user has likely used We prefer avoiding merge commits to keep the graph clean, but it is not mandatory. However, based on your explanation, I believe the user should be able to handle the issue. If the user needs to update their branch, they should use |
73283d8
to
7e2432e
Compare
Hey! Thank you all for your help and support. Yes I had merged because I had a couple of out of date files that I was working on. I've removed the merge commit with the rebase. I think now everything is in order with most change requests updated! |
fix invalidateAll hmac cli command. implements CanAccessTokenExpire() and CanHmacTokenExpire() methods. removes hasAccessTokenExpired() and hasHmacTokenExpired() methods.
fix hmac invalidateAll() expires field.
fix docs refinement. fix test.
333811e
to
023b310
Compare
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.
As for Psalm errors, this is out of the scope of this PR. Seems like the new release has changed some default settings. You can add ensureOverrideAttribute="false"
to the psalm.xml
file, right after findUnusedCode="false"
to fix this.
Yeah, I skimmed the errors looking for a file that I could have edited but couldn't find it. Thanks for the tip. |
fix tokens / hmac docs. fix psalm xml attribute: ensureOverrideAttribute.
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.
Thank you - it looks good. Let's see what others think.
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.
@CosDiabos Thank you for submitting the PR, and I apologize for the long delay. I have provided some suggested changes.
Regarding the method names, my understanding is based on the documentation, but I might have misinterpreted it. If I have misunderstood any part of the code, I would appreciate your clarification.
fix docs and tests.
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.
Please update the PR description. Also, be aware that, based on coordination with the team, others may provide suggestions for improvements here. I appreciate your patience in advance.
Updated! No problem! |
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.
Some grammar fixes
fix updateHmacTokenExpiration() token refresh.
@@ -154,6 +154,19 @@ public function check(array $credentials): Result | |||
|
|||
assert($token->last_used_at instanceof Time || $token->last_used_at === null); | |||
|
|||
// Is expired ? | |||
if ( | |||
$token->expires !== null |
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.
can this use instanceof, eg :
$token->expires !== null | |
$token->expires instanceof Time |
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.
Sounds good.
Description
Implements custom date expiration for AccessTokens and Hmac Tokens leveraging the
expires
column within auth_identities table.Implements
setIdentityExpirationById()
model method,updateAccessTokenExpiration()
,removeAccessTokenExpiration()
,isAccessTokenExpired()
,hasAccessTokenExpiry()
,updateHmacTokenExpiration()
,removeHmacTokenExpiration()
,isHmacTokenExpired()
,hasHmacTokenExpiry()
trait methods and the CLI commandshield:hmac invalidateAll
.Implements and closes #926
Checklist: