Mark tokens as expired instead of deleting them #307
Conversation
sync2/tokens_table.go
Outdated
|
|
||
| var deleteExpiredTokens func() | ||
| deleteExpiredTokens = func() { | ||
| deleted, err := t.deleteExpiredTokensAfter(30 * 24 * time.Hour) |
There was a problem hiding this comment.
30 days might be a bit much for OIDC, so maybe a bit lower?
There was a problem hiding this comment.
FWIW I used 30 days in #242 for a different (but related) purpose.
|
With the failing test, I'm wondering the same as @DMRobertson here #302 (comment) |
sync2/tokens_table.go
Outdated
| logger.Trace().Int64("deleted", deleted).Msg("deleted expired tokens") | ||
| time.AfterFunc(time.Hour, deleteExpiredTokens) | ||
| } | ||
| time.AfterFunc(time.Hour, deleteExpiredTokens) |
There was a problem hiding this comment.
Please don't tie this to the New... constructor. This is going to be such a faff to work with in tests otherwise.
sync2/tokens_table.go
Outdated
| deleted, err := t.deleteExpiredTokensAfter(30 * 24 * time.Hour) | ||
| if err != nil { | ||
| logger.Warn().Err(err).Msg("failed to delete expired tokens") | ||
| return |
There was a problem hiding this comment.
Doesn't make sense to then never delete ever again :S surely we should retry in 1h?
sync2/tokens_table.go
Outdated
| @@ -258,3 +275,38 @@ func (t *TokensTable) Delete(accessTokenHash string) error { | |||
| } | |||
There was a problem hiding this comment.
func (t *TokensTable) Delete(accessTokenHash string) error { is unused now then? Remove it please.
sync3/handler/handler.go
Outdated
| StatusCode: http.StatusUnauthorized, | ||
| ErrCode: "M_UNKNOWN_TOKEN", | ||
| // not exactly /whoami, but would be if we didn't keep the expired state | ||
| Err: fmt.Errorf("/whoami returned HTTP 401"), |
There was a problem hiding this comment.
Please don't lie else this will be confusing when we see this in bug reports. Say "token marked as expired" instead.
sync2/tokens_table_test.go
Outdated
| return nil | ||
| }) | ||
| t.Log("We should be able to fetch this token without error.") | ||
| _, err = tokens.Token(accessToken) |
There was a problem hiding this comment.
Verify the token is not expired.
sync2/tokens_table_test.go
Outdated
| } | ||
|
|
||
| t.Log("Deletes expired tokens") | ||
| deleted, err := tokens.deleteExpiredTokensAfter(time.Nanosecond) |
There was a problem hiding this comment.
I'd check before this that tokens.deleteExpiredTokensAfter(time.Hour) has deleted == 0 i.e it didn't delete anything.
This isn't quite right. The failing test is The failing test is 401ing alice's token because it is old as per comments: // Inject an old token from Alice and a new token from Bob into the DB.It then calls So the token for alice will be marked as expired in the DB in this PR, hence the 401. The failing section of test is: t.Log("Alice makes a new sliding sync request")
res := v3.mustDoV3Request(t, aliceToken, sync3.Request{
Extensions: extensions.Request{
AccountData: &extensions.AccountDataRequest{
Core: extensions.Core{
Enabled: &boolTrue,
},
},
},
})
t.Log("Alice's poller should have been polled.")
v2.waitUntilEmpty(t, aliceToken)Which makes sense as the expired |
|
@kegsay is it still worth taking a look at this or can we close this? |
|
I think we can close this for now. We may want to revisit this later on. |
Fixes #305
This PR changes the following:
expiredcolumn which defaults tofalseexpiredflag when we receive a 401 from upstream