Skip to content
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

Client side clock drift causes 409 on Save after "Update and Continue Editing" #2458

Closed
ralphschindler opened this issue Mar 26, 2020 · 3 comments

Comments

@ralphschindler
Copy link

ralphschindler commented Mar 26, 2020

  • Laravel Version: 6.18.2
  • Nova Version: 2.12.0
  • PHP Version: 7.3.14
  • Database Driver & Version:
  • Operating System and Version: n/a
  • Browser type and version: all
  • Reproduction Repository: n/a

Description:

Very short description

In general, the front end should not be using the Client Side time as a source of truth when attempting to determine if a request is valid when checked against a Model's updated_at time, instead Nova should consider a server generated time.

The Specifics

Some of our users on company issued PC's are not time-syncing against an NTP server (for network reasons, policy reasons, etc). Some of them have already experienced a clock drift of, for example, 5 seconds (meaning their computer clock is 5 seconds in the past - this was checked via https://time.is. (Clock drift is a naturally occurring thing, esp. on laptops that go to sleep/wakeup, etc, it is something we cannot completely stop from happening.)

For these users that are 5 seconds in the past, they can update a resource cleanly, but if they save via Save & Continue Editing the clock drift becomes an issue for their next save.

Since the next fetch happens immediately after save/update, the client generated _retrieved_at timestamp from their own machine is also 1 second behind the server generated updated_at time on the model.

(Client side time is generated at resources/js/views/Update.vue#L238)

This mismatch in time, when saved a second time causes a 409 since the ResourceUpdateController is checking _retrieved_at time against the model's updated_at time.

(Server checked in src/Http/Controllers/ResourceUpdateController.php#L57-L75)

Steps To Reproduce:

  1. Use a nova instance somewhere on a server that has a well regulated clock (like in AWS).

  2. On a client computer, disable time syncing and move the clock a few seconds into the past.

  3. Update a resource by clicking "Save & Continue Editing", attempt to save again.

Proposed Solution

Let the server generate the time to populate the _retrieved_at, OR if the model is using timestamps, populate the _retrieved_at with the updated_at time and ensure the check is that the retrieved is >= the models updated time.

Debugging information

To catch the issue we put some logging into the application (in the nova middleware stack):

        // log nova update requests, making note of clock drift
        if ($request->routeIs('nova.*') && $request->isMethod('PUT')) {
            $retrievedAt = $request->input('_retrieved_at');
            logger()->warning('Nova PUT method', [
                'request_url'            => $request->url(),
                'response_status_code'   => $response->getStatusCode(),
                'retrieved_at_actual'    => $retrievedAt,
                'retrieved_at_formatted' => Carbon::createFromTimestamp($retrievedAt)->__toString(),
                'resource_updated_at'    => $response->original['resource']['updated_at'] ?? 'n/a',
            ]);
        }

And this is what we saw (this particular log shows a 7 second drift on the client's clock). The thing to notice here is that the updated_at time in the top request is later than the retrieved_at time sent in the second request:

2020-03-26-09-27-16

Similar issues:
#1082 & #1853

@jbrooksuk
Copy link
Member

You can disable Traffic Cop if you need to, https://nova.laravel.com/docs/3.0/resources/#disabling-traffic-cop

@SebastianMueller87
Copy link

Disabling this feature does not prevent conceptional miss implementation. This feature itself is a very good and helpfull feature. The current implementation concept is just wrong/error prone.

I just can repeat my suggestion again.

Solution
A good solution would be to send the updated_at-timestamp (from the database) to the client and later on back to the server. A comparison of the updated_at time of the database with the retrieved updated_at time by the client would really prevent data from getting overwritten.

Link: #1082 (comment)

Possible, very good and valid solutions are discussed in that issue.

You can also find some helpfull workarounds there.

@bastihilger
Copy link

I just experienced exactly the same problem - my clients clock ist not synchronized and about a minute late. I also see a lot of merit in @ralphschindler's solution getting the time compare against from the server - makes much more sense than from the client.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants