Teleterm: Define / Set AssumeStartTime fields #38480
Conversation
kimlisa
added
no-changelog
kimlisa
force-pushed
the
lisa/teleterm-assume-start-time-fields
branch
from
6a28114 to
85664ad
Compare
kimlisa
force-pushed
the
lisa/teleterm-assume-start-time-fields
branch
from
f591e9a to
0d50dd1
Compare
| // can overwrite the start time requested by the requester | ||
| // by the reviewer. |
There was a problem hiding this comment.
| // can overwrite the start time requested by the requester | |
| // by the reviewer. | |
| // The reviewer can overwrite the requested time. |
There was a problem hiding this comment.
Maybe just "Overwrites the requested start time"? We're inside AccessRequestReview, so it should be clear who does what and directly states that this specific field, not just "the reviewer", overwrites the requested time.
The only question is whether this field is optional or not and whether it always overwrites the requested start time. An answer to this question should be included in the comment as well.
| // can overwrite the start time requested by the requester | ||
| // by the reviewer. |
There was a problem hiding this comment.
Maybe just "Overwrites the requested start time"? We're inside AccessRequestReview, so it should be clear who does what and directly states that this specific field, not just "the reviewer", overwrites the requested time.
The only question is whether this field is optional or not and whether it always overwrites the requested start time. An answer to this question should be included in the comment as well.
|
|
||
| // Too far in the future. | ||
| invalidStartTime := clock.Now().UTC().Add(1000000 * time.Hour) | ||
| err := ValidateAssumeStartTime(invalidStartTime) |
There was a problem hiding this comment.
I don't know where this function comes from, but shouldn't it accept the clock as an argument? In Go we cannot (shouldn't?) mock the system clock like in Jest. Most of the code that I saw that uses the clockwork package accepts the clock from outside so that tests can pass a fake clock. See lib/utils.VerifyCertificateExpiry for an example.
| @@ -1096,6 +1097,14 @@ func (m *RequestValidator) Validate(ctx context.Context, req types.AccessRequest | |||
| return trace.BadParameter("only promoted requests can set the promoted access list title") | |||
| } | |||
|
|
|||
| if req.GetAssumeStartTime() != nil { | |||
| assumeStartTime := *req.GetAssumeStartTime() | |||
| if time.Until(assumeStartTime) > constants.MaxAssumeStartDuration { | |||
There was a problem hiding this comment.
We should probably use m.clock to calculate this, with similar argumentation to one of my previous comments.
It seems like there's quite a few places in this file that already do this.
There was a problem hiding this comment.
Even though this change is not that big, I feel like it deserves its own PR rather than being bundled together with teleterm changes. I think backend changes deserve more scrutiny by the virtue of being used by the server vs teleterm code which is merely a client.
There was a problem hiding this comment.
Oh, I noticed that we're also moving the validation from tool/ – this would be another argument in favor of preparing a separate PR.
|
closing in favor of #39050, which extracts just defining new fields to teleterm api |
part of #35436
suggest reivewing by commit
This PR just defines and sets the
AssumeStartTimefields in backend/frontend when: creating, reviewing, and listing