NtpTimeProvider ignores NTP data

[paulo-raca]

Thank you for this nice library! I happen to run my app in a few devices that don't have properly sync'd clocks, which caused TOTP codes to fail.

It seemed pretty easy, just using NtpTimeProvider should do it!

Unfortunately it didn't work: Turns out that NtpTimeProvider currently uses timeInfo.getReturnTime() which is the the local timestamp for the reply -- No NTP data is used at all!

The fixed code looks like this (Maybe ntpOffset could be cached?)

    @Override
    public long getTime() throws TimeProviderException {
        long ntpOffset = 0;
        try {
            TimeInfo timeInfo = client.getTime(ntpHost);
            timeInfo.computeDetails();
            if (timeInfo.getOffset() != null) {
                ntpOffset = timeInfo.getOffset();
                Log.i(TAG, "NTP offset to calculate TOTP is " + ntpOffset + "ms");
            } else {
                Log.w(TAG, "Unable to calculate NTP offset");
            }
        } catch (Exception e) {
            Log.w(TAG, "Failed to get NTP offset", e);
        }

        return (System.currentTimeMillis() + ntpOffset) / 1000;
    }

[samdjstevens]

Thanks for raising this issue @paulo-raca - it's my understanding that getReturnTime will return the remote timestamp that the server recieved the request - is this not the case? Can you link me to some docs explaining what this method actuallty returns?

[samdjstevens]

Would be happy to accept a PR on this btw - but would like to throw a TimeProviderException if there is any failure to obtain the offset/time - thoughts?

[paulo-raca]

Hello, @samdjstevens. Unfortunately this is not the case. I don't know of any documentation I can point you to, but in the code you can see:

class NTPUDPClient {
    ...
    public TimeInfo getTime(InetAddress host, int port) {
       ...
        long returnTime = System.currentTimeMillis();
        // create TimeInfo message container but don't pre-compute the details yet
        TimeInfo info = new TimeInfo(recMessage, returnTime, false);

        return info;
    }

I've created a PR with the changes and your suggestions

[samdjstevens]

Just tagged & released 1.7.1 with your fix, should hopefully be on Maven Central in a couple of hours. Thanks!