Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -377,12 +377,17 @@ public int expireAccessTokens() {

@Transactional
public void scheduleTokenExpirationNotification(PersonalAccessToken token) {
token = entityManager.merge(token);
if (token.getType().isNotify() && !token.isNotified()) {
// find, not merge: only `notified` is this method's to change, and merging the whole
// detached token reverted any column that moved since it was loaded - see #989.
var managedToken = entityManager.find(PersonalAccessToken.class, token.getId());
if (managedToken == null) {
return;
}
if (managedToken.getType().isNotify() && !managedToken.isNotified()) {
try {
mail.scheduleAccessTokenExpiryNotification(token);
mail.scheduleAccessTokenExpiryNotification(managedToken);
} finally {
token.setNotified(true);
managedToken.setNotified(true);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,23 +61,45 @@ public void changeNamespaceInDatabase(
if (createNewNamespace) {
entityManager.persist(newNamespace);
} else {
// Deliberately a merge, unlike the rest of #989's call sites: the caller mutates this
// detached namespace before handing it over (ChangeNamespaceJobRequestHandler sets
// logoName on it), so the update belongs to the caller and only merge can carry it.
// find-then-set would silently drop that rename.
newNamespace = entityManager.merge(newNamespace);
}

changeExtensionNamespace(extensions, newNamespace);
changeMembershipNamespace(oldNamespace, newNamespace, removeOldNamespace);
updatedResources.forEach(entityManager::merge);
renameResources(updatedResources);

if (removeOldNamespace) {
oldNamespace = entityManager.merge(oldNamespace);
entityManager.remove(oldNamespace);
// find, not merge: this row is about to go, so merging every column of a detached copy
// first was a wasted UPDATE - and would have resurrected a row already deleted.
var managedOldNamespace = entityManager.find(Namespace.class, oldNamespace.getId());
if (managedOldNamespace != null) {
entityManager.remove(managedOldNamespace);
}
}

cache.evictSitemap();
cache.evictNamespaceDetails(oldNamespace);
search.updateSearchEntries(extensions.filter(Extension::isActive).toList());
}

/**
* Applies the names the caller computed for the copied resources. Only the name changes there
* (see {@code ChangeNamespaceJobRequestHandler}), so this writes that field rather than merging
* every column of each detached resource back - see #989.
*/
private void renameResources(List<FileResource> updatedResources) {
for (var resource : updatedResources) {
var managedResource = entityManager.find(FileResource.class, resource.getId());
if (managedResource != null) {
managedResource.setName(resource.getName());
}
}
}

private void changeExtensionNamespace(Streamable<Extension> extensions, Namespace newNamespace) {
for (var extension : extensions) {
// findExtensions ran inside changeNamespaceInDatabase's transaction, so these are
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -195,8 +195,14 @@ public EclipseProfile getPublicProfile(String personId) {
*/
@Transactional
public void updateUserData(UserData user, EclipseProfile profile) {
user = entityManager.merge(user);
user.setEclipsePersonId(profile.getName());
// find-then-set rather than merge: merge writes every column of the detached copy, so
// anything that changed on the row since it was loaded gets reverted. Only the field
// below is this method's to change - see #989.
var managedUser = entityManager.find(UserData.class, user.getId());
if (managedUser == null) {
return;
}
managedUser.setEclipsePersonId(profile.getName());
}

public void enrichUserJsonWithPublisherAgreement(UserJson json, UserData user) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,16 +33,24 @@ public CheckPotentiallyMaliciousExtensionVersionsService(EntityManager entityMan

@Transactional
public void checkPotentiallyMaliciousExtensionVersion(ExtensionVersion extVersion, TempFile extensionFile) {
boolean isMalicious;
try (var extProcessor = new ExtensionProcessor(extensionFile)) {
boolean isMalicious = extProcessor.isPotentiallyMalicious();
extVersion.setPotentiallyMalicious(isMalicious);
isMalicious = extProcessor.isPotentiallyMalicious();
if (isMalicious) {
logger.atWarn()
.setMessage("Extension version is potentially malicious: {}")
.addArgument(() -> NamingUtil.toLogFormat(extVersion))
.log();
}
}
entityManager.merge(extVersion);

// find-then-set rather than merge: merge writes every column of the detached copy, so
// anything that changed on the row since it was loaded gets reverted. Only the field
// below is this method's to change - see #989.
var managedVersion = entityManager.find(ExtensionVersion.class, extVersion.getId());
if (managedVersion == null) {
return;
}
managedVersion.setPotentiallyMalicious(isMalicious);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ public void run(MigrationJobRequest jobRequest) throws Exception {

try {
resource.setSize(migrations.getFileSize(resource));
migrations.updateResource(resource);
migrations.updateResourceSize(resource);
} catch (FileNotFoundInStorageException e) {
// The object backing this resource is confirmed gone, not just temporarily
// unreachable -- there's nothing further this job can do about that
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,18 @@ public FileResource getResource(MigrationJobRequest<?> jobRequest) {
return entityManager.find(FileResource.class, jobRequest.getEntityId());
}

/**
* Persists the size its caller determined for {@code resource}, and nothing else. Named for the
* one field it writes: a generic "update this resource" could only be implemented by merging the
* whole detached copy, which reverts any column that changed since it was loaded (#989).
*/
@Transactional
public void updateResource(FileResource resource) {
entityManager.merge(resource);
public void updateResourceSize(FileResource resource) {
var managedResource = entityManager.find(FileResource.class, resource.getId());
if (managedResource == null) {
return;
}
managedResource.setSize(resource.getSize());
}

@Retryable
Expand Down Expand Up @@ -161,8 +170,10 @@ public void persistFileResource(FileResource resource) {

@Transactional
public void deleteFileResource(FileResource resource) {
resource = entityManager.merge(resource);
entityManager.remove(resource);
var managedResource = entityManager.find(FileResource.class, resource.getId());
if (managedResource != null) {
entityManager.remove(managedResource);
}
}

public FileResource getFileResource(ExtensionVersion extVersion, String type) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public void run(MigrationJobRequest jobRequest) throws Exception {
migrations.removeFile(download);

download.setName(name);
service.updateResource(download);
service.updateResourceName(download);
}

logger.info("Updated download name to: {}", name);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,16 @@ public FileResource cloneResource(FileResource resource, String name) {
return clone;
}

/**
* Persists the new name its caller set on {@code resource}, and nothing else - see
* {@code MigrationService#updateResourceSize} for why this is named for its field.
*/
@Transactional
public void updateResource(FileResource resource) {
entityManager.merge(resource);
public void updateResourceName(FileResource resource) {
var managedResource = entityManager.find(FileResource.class, resource.getId());
if (managedResource == null) {
return;
}
managedResource.setName(resource.getName());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,21 @@ public List<ExtensionVersion> getExtensionVersions(MigrationJobRequest jobReques

@Transactional
public void updatePreviewAndPreRelease(ExtensionVersion extVersion, TempFile extensionFile) {
boolean preRelease;
boolean preview;
try (var extProcessor = new ExtensionProcessor(extensionFile)) {
extVersion.setPreRelease(extProcessor.isPreRelease());
extVersion.setPreview(extProcessor.isPreview());
preRelease = extProcessor.isPreRelease();
preview = extProcessor.isPreview();
}

entityManager.merge(extVersion);
// find-then-set rather than merge: merge writes every column of the detached copy, so
// anything that changed on the row since it was loaded gets reverted. Only the field
// below is this method's to change - see #989.
var managedVersion = entityManager.find(ExtensionVersion.class, extVersion.getId());
if (managedVersion == null) {
return;
}
managedVersion.setPreRelease(preRelease);
managedVersion.setPreview(preview);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,14 @@ public boolean verifyExtensionVersion(TempFile extensionFile, TempFile signature

@Transactional
public void setSignatureKeyPair(ExtensionVersion extVersion, SignatureKeyPair keyPair) {
extVersion = entityManager.merge(extVersion);
extVersion.setSignatureKeyPair(keyPair);
// find-then-set rather than merge: merge writes every column of the detached copy, so
// anything that changed on the row since it was loaded gets reverted. Only the field
// below is this method's to change - see #989.
var managedVersion = entityManager.find(ExtensionVersion.class, extVersion.getId());
if (managedVersion == null) {
return;
}
managedVersion.setSignatureKeyPair(keyPair);
}

public TempFile generateSignature(TempFile extensionFile, SignatureKeyPair keyPair) throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,14 @@ public void persistResource(FileResource resource) {

@Transactional
public void markExtensionAsPotentiallyMalicious(ExtensionVersion extVersion) {
extVersion = entityManager.merge(extVersion);
extVersion.setPotentiallyMalicious(true);
// find-then-set rather than merge: merge writes every column of the detached copy, so
// anything that changed on the row since it was loaded gets reverted. Only the field
// below is this method's to change - see #989.
var managedVersion = entityManager.find(ExtensionVersion.class, extVersion.getId());
if (managedVersion == null) {
return;
}
managedVersion.setPotentiallyMalicious(true);
}

@Transactional
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -193,8 +193,17 @@ private String generateTokenValue() {

@Transactional
public ResultJson deactivateRateLimitToken(RateLimitToken token) {
token = entityManager.merge(token);
token.setActive(false);
return ResultJson.success("Deactivated rate limit token for customer " + token.getCustomer().getName() + ".");
// find-then-set rather than merge: merge writes every column of the detached copy, so
// anything that changed on the row since it was loaded gets reverted. Only the field
// below is this method's to change - see #989.
var managedToken = entityManager.find(RateLimitToken.class, token.getId());
if (managedToken == null) {
// RateLimitAPI already established the token exists before calling, so this only
// happens if the row went away in between; matches how this class reports a missing row.
throw new ErrorResultException("Rate limit token not found: " + token.getId());
}
managedToken.setActive(false);
return ResultJson
.success("Deactivated rate limit token for customer " + managedToken.getCustomer().getName() + ".");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ void run_recordsSizeForEveryUnsizedResourceOfTheExtensionVersionAndSkipsAlreadyS

new FileResourceSizeJobRequestHandler(migrations).run(jobRequest);

verify(migrations).updateResource(unsized);
verify(migrations, never()).updateResource(alreadySized);
verify(migrations).updateResourceSize(unsized);
verify(migrations, never()).updateResourceSize(alreadySized);
verify(migrations, never()).getFileSize(alreadySized);
}

Expand All @@ -78,8 +78,8 @@ void run_skipsAMissingResourceButStillProcessesTheRestOfTheExtensionVersion() th

assertThatCode(() -> handler.run(jobRequest)).doesNotThrowAnyException();

verify(migrations, never()).updateResource(missing);
verify(migrations).updateResource(present);
verify(migrations, never()).updateResourceSize(missing);
verify(migrations).updateResourceSize(present);
}

private ExtensionVersion extVersion() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,51 @@ void mirrorResource_recordsTheSizeOfTheExtractedBytes() throws Exception {
verify(entityManager).persist(resource);
}

// #989: markExtensionAsPotentiallyMalicious used to merge the whole detached extVersion to set
// one boolean, so any column that had moved since the caller loaded it was reverted - the same
// failure mode that once left published extensions inactive. It now writes the flag on the
// managed row, matching what activateExtension above already did.
@Test
void markExtensionAsPotentiallyMalicious_flagsTheManagedRow() {
var stale = version(1L, false);
var managed = version(1L, false);
when(entityManager.find(ExtensionVersion.class, 1L)).thenReturn(managed);

svc.markExtensionAsPotentiallyMalicious(stale);

assertThat(managed.isPotentiallyMalicious()).isTrue();
verify(entityManager, never()).merge(any());
}

@Test
void markExtensionAsPotentiallyMalicious_leavesConcurrentChangesAlone() {
// The caller's copy was loaded while the version was still inactive; it has since been
// activated. Merging that stale copy back would have deactivated it again.
var stale = version(1L, false);
stale.setActive(false);
var managed = version(1L, false);
managed.setActive(true);
when(entityManager.find(ExtensionVersion.class, 1L)).thenReturn(managed);

svc.markExtensionAsPotentiallyMalicious(stale);

assertThat(managed.isPotentiallyMalicious()).isTrue();
assertThat(managed.isActive()).as("a concurrent activation must survive the flag write").isTrue();
}

// The row can be purged between the caller loading it and this running; merge would have tried
// to resurrect it rather than doing nothing.
@Test
void markExtensionAsPotentiallyMalicious_ignoresAVersionThatIsGone() {
var stale = version(1L, false);
when(entityManager.find(ExtensionVersion.class, 1L)).thenReturn(null);

svc.markExtensionAsPotentiallyMalicious(stale);

verify(entityManager, never()).merge(any());
verify(entityManager, never()).persist(any());
}

private ExtensionVersion version(long id, boolean removed) {
var namespace = new Namespace();
namespace.setName("redhat");
Expand Down