types.proto: Bring back RecoveryAttemptLockExpires field as deprecated

[jentfoo]

Raised from @codingllama here:

The removal of this field is making UpdateAndSwapUser fail.

If the stored user had the "recovery_attempt_lock_expires" field set, then the comparison always fails. That is because when we read and unmarshal the user the unknown field gets dropped, but the actual CompareAndSwap operation compares using the storage blobs instead.

This PR does the short term fix by reverting this field change but marking the field as deprecated.

[jentfoo]

@codingllama and @zmb3 the lint failure makes it look like we can't simply bring this field back. Will this be API compatible?

[codingllama]

@codingllama and @zmb3 the lint failure makes it look like we can't simply bring this field back. Will this be API compatible?

I don't see why restoring the field would be a breaking change, neither for proto or JSON. @espadolini, any ideas?

[espadolini]

It's ok, it's just that the backwards compatibility checker can't know that it's ok, and we never added a way to bypass it for a given PR (and I'm not sure of how we could do that for the check in the merge queue, actually).

Couldn't we fix the function instead? The correct way to do compare-and-swap with object is to load the item, unmarshal it according to the current understanding of the type by the auth, compare the object with the expected object, then do a conditional update with the new item (in pre-condupdate days, compare-and-swap the new item with the one that was fetched). It's always been slightly wrong to assume that the marshaling would never change.

[rosstimothy]

Couldn't we fix the function instead? The correct way to do compare-and-swap with object is to load the item, unmarshal it according to the current understanding of the type by the auth, compare the object with the expected object, then do a conditional update with the new item (in pre-condupdate days, compare-and-swap the new item with the one that was fetched). It's always been slightly wrong to assume that the marshaling would never change.

+1 to using conditional update instead of compare and swap in this scenario

[espadolini]

Prior art:

• improve ca cmp #10351

[codingllama]

Yeah, fixing the CAS was one of my suggestions. This is just a simpler/quicker fix while we get that done.

[jentfoo]

I am on PTO today and tomorrow, and next week the security team has a lot of ISO audit work ahead. Given that, I was just trying to get a quick fix out

[codingllama]

I am on PTO today and tomorrow, and next week the security team has a lot of ISO audit work ahead. Given that, I was just trying to get a quick fix out

Enjoy your PTO, Mike. I'll take a look at the CompareAndSwap changes.

[rosstimothy]

#37660 addresses the issue by fixing the CAS operation

[rosstimothy]

Closing now that #37660 has been merged