sonatype-nexus-community / nexus-repository-apt

A Nexus Repository 3 plugin that allows usage of apt repositories
Eclipse Public License 1.0
105 stars 50 forks source link

Fix transaction pattern in AptUploadHandler #72

Closed DarthHater closed 5 years ago

DarthHater commented 6 years ago

Thanks for creating an issue! Please fill out this form so we can be sure to have all the information we need, and to minimize back and forth.

Ideally ensurePermitted would occur before a Transaction is opened, and likely right after the TempBlob is created (as it looks like that's how you get your path)

DarthHater commented 6 years ago

You can see how this was reimplemented in: https://github.com/sonatype/nexus-public/blob/729ac4987d99f581e6ff95a2c1b92945057107aa/plugins/nexus-repository-maven/src/main/java/org/sonatype/nexus/repository/maven/MavenUploadHandler.java

mpoindexter commented 6 years ago

What's the downside of opening the transaction? Is it super heavyweight to do so? Certainly what you propose would be more efficient, but it doesn't seem like it would be a huge difference?

DarthHater commented 6 years ago

@mpoindexter @mcculls can better explain it than I can, likely, but the situation that could be encountered is a) tempBlobs being created before security is checked (albeit, this looks unavoidable in your case), and long running transactions, and nested transactions, as security opens one to config AFAIK. This is from the internal issue:

Several upload handlers currently follow this anti-pattern:

  • start transaction
  • do some validation
  • create temporary blobs
  • create components/assets
  • commit transaction

The problem with this approach is that it leaves the transaction open for a long time. If the transaction needs to be retried then it will repeat the same checks and create more temporary blobs.

EDIT: The other issue with starting the storage transaction early on is that it breaks the later call to check content selector permissions (but only if content selector permissions apply to the upload). This is because the current active transaction is for the component DB, but the content selector check wants to query the config DB. We don't support nested active transactions, so the content selector basically ends up querying the current DB (ie. component) for config related information - this leads to schema problems and other exceptions.

Ideally the transaction should only be started just before it's needed, ie. calling into the facet to create the component/assets, and closed as soon as possible. This also solves the content selector issue because the call to check permissions is now made outside of an active transaction, before the storage transaction is started.

Blacktiger commented 6 years ago

The main problem is we have an open transaction to the security databases as well as an open transaction to the component database. If both of these transactions are nested this can cause strange errors where the wrong db connection is used.

mpoindexter commented 6 years ago

Thanks, that makes sense!

DarthHater commented 6 years ago

I'll see if I can float a branch your way for this, but I'll likely need you (and others) to test it and make sure I didn't flub anything @mpoindexter