-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[v15] Set default access request TTL to 1 week. #39509
Conversation
lib/services/access_request_test.go
Outdated
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.
actually, any reason why we removed this test? i ran into an issue where user is able to request a pending TTL pass their access expiry, and am looking at this test and I think it could've caught the 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.
I think this went through a ton of iterations and got pretty confused by the end, so I'll see if I can restore this.
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.
i attempted a fix (i was already working on it to test my ui changes): #39548
ae34f16
to
c157813
Compare
I notice a lot of checks are failing, @mdwn is it worth reviewing now or should I hold off until you have a chance to fix things? |
I'll take a look shortly. I'll ping when this is ready. |
The TTL for a request now defaults to 1 week. This will allow reviewers more time to review an access request before it disappears. Co-authored-by: Lisa Kim <[email protected]>
c157813
to
141fad9
Compare
* Prevent setting pending request TTL pass the access expiry time * Allow setting max duration less than sessionTTL when role max_duration is not set * Add more test, clarify comment * Remove unused params * Add a test for respecting both pending and max duration request * Add a fakeclock to fix access request time diff check differences
141fad9
to
4885dab
Compare
@ibeckermayer This should be good to go now. This is mine and @kimlisa's fixed together. |
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.
Let's hold this backport until @kimlisa pulls it into her branch with future access requests start time UI implementation so we can test full end-to-end flow.
@kimlisa has verified this fix, so I've removed the do-not-merge tag. |
/excludeflake * |
Backport #35799 to branch/v15
Includes Lisa's fix: #39548
changelog: Access Request TTLs default to 1 week.