fix(auth): sync claim/token times in SA creds #789
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #789 +/- ##
==========================================
- Coverage 95.63% 95.62% -0.02%
==========================================
Files 37 37
Lines 1375 1371 -4
==========================================
- Hits 1315 1311 -4
Misses 60 60 ☔ View full report in Codecov by Sentry. |
| @@ -14,7 +14,6 @@ | |||
|
|
|||
| use crate::credentials::CredentialError; | |||
| use crate::credentials::Result; | |||
| use derive_builder::Builder; | |||
There was a problem hiding this comment.
Thanks. I think there is an unused dependency in src/auth/Cargo.toml now.
There was a problem hiding this comment.
Still used elsewhere:
| let now = OffsetDateTime::now_utc() - CLOCK_SKEW_FUDGE; | ||
| let exp = now + DEFAULT_TOKEN_TIMEOUT; | ||
| let claims = JwsClaims { | ||
| iss: self.service_account_info.client_email.clone(), | ||
| scope: Some(DEFAULT_SCOPES.map(|s| s.to_string()).to_vec()), | ||
| aud: None, | ||
| exp, | ||
| iat: now, | ||
| typ: None, | ||
| sub: None, | ||
| }; |
There was a problem hiding this comment.
Alternatively, you could create a JwsClaims::new(now: OffsetDateTime) -> Self function.
There was a problem hiding this comment.
I think we are going to support options to override the scope, so that function will make less sense in the future.
(Also, we should not bother to have typ or sub if they are always None. I assume there will be some code path where they have concrete values in the future)
| pub iss: &'a str, | ||
| #[derive(Serialize)] | ||
| pub struct JwsClaims { | ||
| pub iss: String, |
There was a problem hiding this comment.
I like the simplicity of using String. It is more expensive, but this function will be called like once per hour so we should not complicate things too much in search of small performance improvements.
| @@ -116,44 +112,30 @@ mod tests { | |||
| ) | |||
| .unwrap(); | |||
|
|
|||
| // 5 seconds is like 640KiB, good enough for everybody | |||
Fixes #733
Make sure the token's
expires_attime matches theexpfield in the claim.We drop the
JwsClaimsBuilderand do any defaulting inservice_account_credential.rs. Note that theexpandiatfields are required, so they are probably better represented asOffsetDateTimethanOption<OffsetDateTime>.