Code review comment for lp:~mir-team/mir/attestable-timestamps-server

Sorry about the long list here, most of it is about fitting in with the design styles that have grown in Mir and you would not yet be familiar with. I've voted "Needs Fixing" but many of the points might be addressed by discussion.

~~~~

src/server/default_server_configuration.cpp

There's a lot of code added here (to generate a seed) that probably doesn't belong in a translation unit that is about constructing the right objects to configure the system. I'd prefer to see it put in a separate file. (Is there any reason it can't be part of the new library?)

~~~~

+void fill_vector_with_random_data(std::vector<uint8_t>& buffer)

Could we not use a return value instead of an out parameter?

~~~~

+std::string mir_test_framework::udev_recordings_path()
+{
+ std::string share_path = MIR_INSTALL_PREFIX"/share/udev_recordings";
+ std::string bin_path = MIR_BUILD_PREFIX"/bin/udev_recordings";
+
+ if (boost::filesystem::exists(share_path))
+ return share_path;
+ else if (boost::filesystem::exists(bin_path))
+ return bin_path;
+
+ BOOST_THROW_EXCEPTION(std::runtime_error("Failed to find udev_recordings in standard search locations"));
+}

Can this be right? A development test binary looks in the location where it would be installed? Even though a developer may be making changes to the version in the build path.

The older logic (based on the executable path) seems less surprising.

~~~~

+class CookieFactory
+{
+public:
+ CookieFactory(std::vector<uint8_t> const& secret);
+ ~CookieFactory() noexcept;
+
+ MirCookie timestamp_to_cookie(uint64_t const& timestamp);
+
+ bool attest_timestamp(MirCookie const& cookie);
+
+private:
+ class CookieImpl;
+ std::unique_ptr<CookieImpl> impl;
+};

What is the reason for preferring the Cheshire Cat idiom over Interface Class? This makes it far harder to substitute alternative implementations (e.g. for testing that cookies actually are validated when necessary).

I can also imagine a desire to provide a different verification model in a (hypothetical) downstream server.

~~~~~

+void mir::Server::set_cookie_secret(std::vector<uint8_t> const& secret)
+{
+ verify_setting_allowed(self->server_config);
+ self->cookie_factory = std::make_shared<CookieFactory>(secret);
+}

It is a little surprising that this sets the cookie factory, not the secret. I can't immediately think of anything this breaks but...

Between this and the previous, I think this would fit better into Mir's design patterns if:

1. CookieFactory were an interface and CookieImpl a derived class.
2. the configuration point were override_the_cookie_factory

~~~~~

=== added file 'tests/acceptance-tests/test_mir_cookie.cpp'

This reads more like a unit test

~~~~~

+ mir_discover_tests_with_fd_leak_detection(mir_acceptance_tests LD_PRELOAD=libumockdev-preload.so.0 G_SLICE=always-malloc G_DEBUG=gc-friendly)

The acceptance tests (before and after this MP) don't need libumockdev-preload.so.0 (which is convenient when running them by hand) why change that?

~~~~~

AFAICS the_cookie_factory() isn't used. I've not looked in the parent branch - I assume use (and real acceptance tests) is coming soon?