Reapply "Store attestations for PEP740 (#16302)" (#16545)

[woodruffw]

WIP, needs full backstop tests.

This reverts commit da7e1ed.

[woodruffw]

Listing some action items that need to be accomplished before undrafting:

• The current TestSimpleDetail tests should be extended to be include a case where provenance is actually present, not just None
• Reapply "Store attestations for PEP740 (#16302)" (#16545) #16546 (review)
• More generally, go through TestAttestationsService (which should be renamed TestIntegrityService) with a fine-toothed comb and ensure we aren't mocking overly agressively

[di]

I added a functional test for this in 17e9d45 but it's currently failing trying to refresh the TUF metadata. @woodruffw @DarkaMaul thoughts on what we should do about TUF here?

[di]

Oh interesting, the TUF failure seems to have been an intermittent thing, this now works. Will have to look into why the attestation hash digest is different.

[di]

Here's what I was getting, for context:

============================================= short test summary info =============================================
FAILED tests/functional/api/test_simple.py::test_simple_attestations_from_upload - webtest.app.AppError: Bad response: 400 Unknown error while trying to verify included attestations: Failed to refresh TUF metadata (not 200)
400 Unknown error while trying to verify included attestations: Failed to refresh TUF metadata

The server could not comply with the request since it is either malformed or otherwise incorrect.


Unknown error while trying to verify included attestations: Failed to refresh TUF metadata

Results (16.29s):
       1 failed
         - tests/functional/api/test_simple.py:48 test_simple_attestations_from_upload
make: *** [Makefile:79: tests] Error 1

[DarkaMaul]

I added a functional test for this in 17e9d45 but it's currently failing trying to refresh the TUF metadata. @woodruffw @DarkaMaul thoughts on what we should do about TUF here?

I've also started writing some tests for this at trail-of-forks#3063, and my solution there was to mock the verification.

However, I did not assert that the hashes were correct (and they are probably not).

[DarkaMaul]

This test fails here because the hash we get here is

hashlib.sha256(
    b"sampleproject-3.0.0.tar.gz:2"   # notice the 2 here
).hexdigest()

However, the len(attestations) should be 1.

The problem lies in generate_provenance of NullService that creates the DatabaseAttestation and adds it to the file.attestations (like in _persist_attestations )

Because the relationship is noted as back_populates, sqlalchemy automatically adds the newly created DatabaseAttestation to the file instance.

The statement below is thus redundant and creates add a second time the attestation to the file instance.

file.attestations.append(database_attestation)

However, if we look at test_persist_attestations_succeeds, the test passes with the following statements :

        assert len(attestations_db) == 1
        assert len(file.attestations) == 1

I've observed the state in a debugger using sqlalchemy tools and they looked appropriate.

from sqlachemy import inspect
state = inspect(database_attestation)

Any idea here of the reason for this behavior ?

[di]

The issue is that the Attestation objects weren't actually being added to the session, the test was emitting a warning like:

tests/unit/attestations/test_services.py::TestIntegrityService::test_generate_provenance_succeeds[GitHubPublisherFactory]
  /opt/warehouse/src/warehouse/attestations/models.py:53: SAWarning: Object of type <Attestation> not in session, add operation along 'File.attestations' will not proceed (This warning originated from the Session 'autoflush' process, which was invoked automatically in response to a user-initiated operation.)
    f"{Path(self.file.path).name}.attestation",

I think f7e277e and fa1bd58 should resolve this.

[DarkaMaul]

Thanks for sorting this out - I missed the warning in the logs.

[di]

LGTM, will wait to merge until next week though.