You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@archiva.apache.org by oc...@apache.org on 2009/10/13 12:36:25 UTC
svn commit: r824677 - in
/archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src:
main/java/org/apache/maven/archiva/webdav/
test/java/org/apache/maven/archiva/webdav/ test/resources/
Author: oching
Date: Tue Oct 13 10:36:24 2009
New Revision: 824677
URL: http://svn.apache.org/viewvc?rev=824677&view=rev
Log:
[MRM-747] Archiva should prevent re-deployment of released or non-snapshot versioned artifacts
submitted by Marc Lustig
o added checks in webdav to block re-deployment if artifact version already exists in the repo and throw a 409 in such cases
o added tests for deploying and re-deploying an artifact
additional modifications to the patch:
o update checking for artifact types that will be blocked
o add tests for deploying metadata and support file
Added:
archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/main/java/org/apache/maven/archiva/webdav/ReleaseArtifactAlreadyExistsException.java
archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/test/resources/artifact.jar.sha1
Modified:
archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/main/java/org/apache/maven/archiva/webdav/ArchivaDavResourceFactory.java
archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/test/java/org/apache/maven/archiva/webdav/AbstractRepositoryServletTestCase.java
archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/test/java/org/apache/maven/archiva/webdav/RepositoryServletDeployTest.java
Modified: archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/main/java/org/apache/maven/archiva/webdav/ArchivaDavResourceFactory.java
URL: http://svn.apache.org/viewvc/archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/main/java/org/apache/maven/archiva/webdav/ArchivaDavResourceFactory.java?rev=824677&r1=824676&r2=824677&view=diff
==============================================================================
--- archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/main/java/org/apache/maven/archiva/webdav/ArchivaDavResourceFactory.java (original)
+++ archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/main/java/org/apache/maven/archiva/webdav/ArchivaDavResourceFactory.java Tue Oct 13 10:36:24 2009
@@ -40,6 +40,7 @@
import org.apache.jackrabbit.webdav.lock.LockManager;
import org.apache.jackrabbit.webdav.lock.SimpleLockManager;
import org.apache.maven.archiva.common.utils.PathUtil;
+import org.apache.maven.archiva.common.utils.VersionUtil;
import org.apache.maven.archiva.configuration.ArchivaConfiguration;
import org.apache.maven.archiva.configuration.RepositoryGroupConfiguration;
import org.apache.maven.archiva.model.ArchivaRepositoryMetadata;
@@ -162,12 +163,12 @@
* @plexus.requirement role-hint="md5";
*/
private Digester digestMd5;
-
+
/**
* @plexus.requirement
*/
private ArchivaTaskScheduler scheduler;
-
+
public DavResource createResource( final DavResourceLocator locator, final DavServletRequest request,
final DavServletResponse response )
throws DavException
@@ -190,7 +191,7 @@
throw new DavException( HttpServletResponse.SC_METHOD_NOT_ALLOWED,
"Write method not allowed for repository groups." );
}
-
+
log.debug( "Repository group '" + repoGroupConfig.getId() + "' accessed by '" + activePrincipal + "'" );
// handle browse requests for virtual repos
@@ -200,9 +201,16 @@
}
else
{
- resource =
- processRepositoryGroup( request, archivaLocator, repoGroupConfig.getRepositories(),
- activePrincipal, resourcesInAbsolutePath );
+ try
+ {
+ resource =
+ processRepositoryGroup( request, archivaLocator, repoGroupConfig.getRepositories(),
+ activePrincipal, resourcesInAbsolutePath );
+ }
+ catch ( ReleaseArtifactAlreadyExistsException e )
+ {
+ throw new DavException( HttpServletResponse.SC_CONFLICT );
+ }
}
}
else
@@ -215,8 +223,8 @@
}
catch ( RepositoryNotFoundException e )
{
- throw new DavException( HttpServletResponse.SC_NOT_FOUND, "Invalid repository: "
- + archivaLocator.getRepositoryId() );
+ throw new DavException( HttpServletResponse.SC_NOT_FOUND, "Invalid repository: " +
+ archivaLocator.getRepositoryId() );
}
catch ( RepositoryException e )
{
@@ -225,7 +233,14 @@
log.debug( "Managed repository '" + managedRepository.getId() + "' accessed by '" + activePrincipal + "'" );
- resource = processRepository( request, archivaLocator, activePrincipal, managedRepository );
+ try
+ {
+ resource = processRepository( request, archivaLocator, activePrincipal, managedRepository );
+ }
+ catch ( ReleaseArtifactAlreadyExistsException e )
+ {
+ throw new DavException( HttpServletResponse.SC_CONFLICT, e );
+ }
String logicalResource = RepositoryPathUtil.getLogicalResource( locator.getResourcePath() );
resourcesInAbsolutePath.add( new File( managedRepository.getRepoRoot(), logicalResource ).getAbsolutePath() );
@@ -235,8 +250,8 @@
// MRM-872 : merge all available metadata
// merge metadata only when requested via the repo group
- if ( ( repositoryRequest.isMetadata( requestedResource ) || ( requestedResource.endsWith( "metadata.xml.sha1" ) || requestedResource.endsWith( "metadata.xml.md5" ) ) )
- && repoGroupConfig != null )
+ if ( ( repositoryRequest.isMetadata( requestedResource ) || ( requestedResource.endsWith( "metadata.xml.sha1" ) || requestedResource.endsWith( "metadata.xml.md5" ) ) ) &&
+ repoGroupConfig != null )
{
// this should only be at the project level not version level!
if ( isProjectReference( requestedResource ) )
@@ -334,9 +349,9 @@
private DavResource processRepositoryGroup( final DavServletRequest request,
ArchivaDavResourceLocator archivaLocator, List<String> repositories,
String activePrincipal, List<String> resourcesInAbsolutePath )
- throws DavException
+ throws DavException, ReleaseArtifactAlreadyExistsException
{
- DavResource resource = null;
+ DavResource resource = null;
List<DavException> storedExceptions = new ArrayList<DavException>();
for ( String repositoryId : repositories )
@@ -372,24 +387,24 @@
resourcesInAbsolutePath.add( new File( managedRepository.getRepoRoot(), logicalResource ).getAbsolutePath() );
}
catch ( DavException e )
- {
+ {
storedExceptions.add( e );
}
}
if ( resource == null )
- {
+ {
if ( !storedExceptions.isEmpty() )
- {
+ {
// MRM-1232
- for( DavException e : storedExceptions )
+ for ( DavException e : storedExceptions )
{
- if( 401 == e.getErrorCode() )
+ if ( 401 == e.getErrorCode() )
{
throw e;
}
}
-
+
throw new DavException( HttpServletResponse.SC_NOT_FOUND );
}
else
@@ -402,7 +417,7 @@
private DavResource processRepository( final DavServletRequest request, ArchivaDavResourceLocator archivaLocator,
String activePrincipal, ManagedRepositoryContent managedRepository )
- throws DavException
+ throws DavException, ReleaseArtifactAlreadyExistsException
{
DavResource resource = null;
if ( isAuthorized( request, managedRepository.getId() ) )
@@ -412,13 +427,12 @@
{
path = path.substring( 1 );
}
- LogicalResource logicalResource = new LogicalResource( path );
+ LogicalResource logicalResource = new LogicalResource( path );
File resourceFile = new File( managedRepository.getRepoRoot(), path );
resource =
- new ArchivaDavResource( resourceFile.getAbsolutePath(), path,
- managedRepository.getRepository(), request.getRemoteAddr(), activePrincipal,
- request.getDavSession(), archivaLocator, this, mimeTypes, auditListeners,
- scheduler );
+ new ArchivaDavResource( resourceFile.getAbsolutePath(), path, managedRepository.getRepository(),
+ request.getRemoteAddr(), activePrincipal, request.getDavSession(),
+ archivaLocator, this, mimeTypes, auditListeners, scheduler );
if ( WebdavMethodUtil.isReadMethod( request.getMethod() ) )
{
@@ -432,7 +446,7 @@
if ( !resource.isCollection() )
{
boolean previouslyExisted = resourceFile.exists();
-
+
// Attempt to fetch the resource from any defined proxy.
boolean fromProxy = fetchContentFromProxies( managedRepository, request, logicalResource );
@@ -462,11 +476,11 @@
if ( fromProxy )
{
String event =
- ( previouslyExisted ? AuditEvent.MODIFY_FILE : AuditEvent.CREATE_FILE )
- + PROXIED_SUFFIX;
-
+ ( previouslyExisted ? AuditEvent.MODIFY_FILE : AuditEvent.CREATE_FILE ) +
+ PROXIED_SUFFIX;
+
log.debug( "Proxied artifact '" + resourceFile.getName() + "' in repository '" +
- managedRepository.getId() + "' (current user '" + activePrincipal + "')" );
+ managedRepository.getId() + "' (current user '" + activePrincipal + "')" );
triggerAuditEvent( request.getRemoteAddr(), archivaLocator.getRepositoryId(),
logicalResource.getPath(), event, activePrincipal );
@@ -482,6 +496,35 @@
if ( request.getMethod().equals( HTTP_PUT_METHOD ) )
{
+ String resourcePath = logicalResource.getPath();
+
+ // check if target repo is enabled for releases
+ // we suppose that release-artifacts can deployed only to repos enabled for releases
+ if ( managedRepository.getRepository().isReleases() && !repositoryRequest.isMetadata( resourcePath ) &&
+ !repositoryRequest.isSupportFile( resourcePath ) )
+ {
+ ArtifactReference artifact = null;
+ try
+ {
+ artifact = managedRepository.toArtifactReference( resourcePath );
+ }
+ catch ( LayoutException e )
+ {
+ throw new DavException( HttpServletResponse.SC_BAD_REQUEST, e );
+ }
+
+ if ( !VersionUtil.isSnapshot( artifact.getVersion() ) )
+ {
+ // check if artifact already exists
+ if ( managedRepository.hasContent( artifact ) )
+ {
+ log.warn( "Overwriting released artifacts is not allowed." );
+ throw new ReleaseArtifactAlreadyExistsException( managedRepository.getId(),
+ "Overwriting released artifacts is not allowed." );
+ }
+ }
+ }
+
/*
* Create parent directories that don't exist when writing a file This actually makes this
* implementation not compliant to the WebDAV RFC - but we have enough knowledge about how the
@@ -496,9 +539,10 @@
{
destDir.mkdirs();
String relPath = PathUtil.getRelative( rootDirectory.getAbsolutePath(), destDir );
-
- log.debug( "Creating destination directory '" + destDir.getName() + "' (current user '" + activePrincipal + "')" );
-
+
+ log.debug( "Creating destination directory '" + destDir.getName() + "' (current user '" +
+ activePrincipal + "')" );
+
triggerAuditEvent( request.getRemoteAddr(), logicalResource.getPath(), relPath,
AuditEvent.CREATE_DIR, activePrincipal );
}
@@ -519,8 +563,8 @@
}
catch ( RepositoryNotFoundException e )
{
- throw new DavException( HttpServletResponse.SC_NOT_FOUND, "Invalid repository: "
- + archivaLocator.getRepositoryId() );
+ throw new DavException( HttpServletResponse.SC_NOT_FOUND, "Invalid repository: " +
+ archivaLocator.getRepositoryId() );
}
catch ( RepositoryException e )
{
@@ -572,8 +616,9 @@
File proxiedFile = connectors.fetchFromProxies( managedRepository, artifact );
resource.setPath( managedRepository.toPath( artifact ) );
-
- log.debug( "Proxied artifact '" + artifact.getGroupId() + ":" + artifact.getArtifactId() + ":" + artifact.getVersion() + "'" );
+
+ log.debug( "Proxied artifact '" + artifact.getGroupId() + ":" + artifact.getArtifactId() + ":" +
+ artifact.getVersion() + "'" );
return ( proxiedFile != null );
}
@@ -774,9 +819,9 @@
AuthenticationResult result = httpAuth.getAuthenticationResult( request, null );
SecuritySession securitySession = httpAuth.getSecuritySession( request.getSession( true ) );
- return servletAuth.isAuthenticated( request, result )
- && servletAuth.isAuthorized( request, securitySession, repositoryId,
- WebdavMethodUtil.getMethodPermission( request.getMethod() ) );
+ return servletAuth.isAuthenticated( request, result ) &&
+ servletAuth.isAuthorized( request, securitySession, repositoryId,
+ WebdavMethodUtil.getMethodPermission( request.getMethod() ) );
}
catch ( AuthenticationException e )
{
@@ -878,8 +923,8 @@
catch ( DavException e )
{
// TODO: review exception handling
- log.debug( "Skipping repository '" + managedRepository + "' for user '" + activePrincipal
- + "': " + e.getMessage() );
+ log.debug( "Skipping repository '" + managedRepository + "' for user '" + activePrincipal +
+ "': " + e.getMessage() );
}
}
else
@@ -897,8 +942,8 @@
catch ( UnauthorizedException e )
{
// TODO: review exception handling
- log.debug( "Skipping repository '" + managedRepository + "' for user '" + activePrincipal
- + "': " + e.getMessage() );
+ log.debug( "Skipping repository '" + managedRepository + "' for user '" + activePrincipal +
+ "': " + e.getMessage() );
}
}
}
@@ -1057,7 +1102,7 @@
{
this.repositoryRequest = repositoryRequest;
}
-
+
public void setConnectors( RepositoryProxyConnectors connectors )
{
this.connectors = connectors;
Added: archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/main/java/org/apache/maven/archiva/webdav/ReleaseArtifactAlreadyExistsException.java
URL: http://svn.apache.org/viewvc/archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/main/java/org/apache/maven/archiva/webdav/ReleaseArtifactAlreadyExistsException.java?rev=824677&view=auto
==============================================================================
--- archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/main/java/org/apache/maven/archiva/webdav/ReleaseArtifactAlreadyExistsException.java (added)
+++ archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/main/java/org/apache/maven/archiva/webdav/ReleaseArtifactAlreadyExistsException.java Tue Oct 13 10:36:24 2009
@@ -0,0 +1,38 @@
+package org.apache.maven.archiva.webdav;
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+/**
+ */
+public class ReleaseArtifactAlreadyExistsException
+ extends Exception
+{
+ final private String repositoryName;
+
+ public ReleaseArtifactAlreadyExistsException( String repositoryName, String message )
+ {
+ this.repositoryName = repositoryName;
+ }
+
+ public String getRepositoryName()
+ {
+ return repositoryName;
+ }
+}
Modified: archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/test/java/org/apache/maven/archiva/webdav/AbstractRepositoryServletTestCase.java
URL: http://svn.apache.org/viewvc/archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/test/java/org/apache/maven/archiva/webdav/AbstractRepositoryServletTestCase.java?rev=824677&r1=824676&r2=824677&view=diff
==============================================================================
--- archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/test/java/org/apache/maven/archiva/webdav/AbstractRepositoryServletTestCase.java (original)
+++ archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/test/java/org/apache/maven/archiva/webdav/AbstractRepositoryServletTestCase.java Tue Oct 13 10:36:24 2009
@@ -111,6 +111,13 @@
Assert.assertEquals( "Should have been an 500/Internal Server Error response code.", HttpServletResponse.SC_INTERNAL_SERVER_ERROR, response
.getResponseCode() );
}
+
+ protected void assertResponseConflictError( WebResponse response )
+ {
+ assertNotNull( "Should have received a response", response );
+ Assert.assertEquals( "Should have been a 409/Conflict response code.", HttpServletResponse.SC_CONFLICT,
+ response.getResponseCode() );
+ }
protected ManagedRepositoryConfiguration createManagedRepository( String id, String name, File location )
{
Modified: archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/test/java/org/apache/maven/archiva/webdav/RepositoryServletDeployTest.java
URL: http://svn.apache.org/viewvc/archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/test/java/org/apache/maven/archiva/webdav/RepositoryServletDeployTest.java?rev=824677&r1=824676&r2=824677&view=diff
==============================================================================
--- archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/test/java/org/apache/maven/archiva/webdav/RepositoryServletDeployTest.java (original)
+++ archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/test/java/org/apache/maven/archiva/webdav/RepositoryServletDeployTest.java Tue Oct 13 10:36:24 2009
@@ -38,20 +38,64 @@
public class RepositoryServletDeployTest
extends AbstractRepositoryServletTestCase
{
+ private static final String ARTIFACT_DEFAULT_LAYOUT = "/path/to/artifact/1.0.0/artifact-1.0.0.jar";
+
public void testPutWithMissingParentCollection()
throws Exception
{
setupCleanRepo( repoRootInternal );
- String putUrl = "http://machine.com/repository/internal/path/to/artifact.jar";
+ String putUrl = "http://machine.com/repository/internal" + ARTIFACT_DEFAULT_LAYOUT;
InputStream is = getClass().getResourceAsStream( "/artifact.jar" );
+ // verify that the file exists in resources-dir
assertNotNull( "artifact.jar inputstream", is );
WebRequest request = new PutMethodWebRequest( putUrl, is, "application/octet-stream" );
WebResponse response = sc.getResponse( request );
assertResponseCreated( response );
- assertFileContents( "artifact.jar\n", repoRootInternal, "path/to/artifact.jar" );
+ assertFileContents( "artifact.jar\n", repoRootInternal, ARTIFACT_DEFAULT_LAYOUT );
+ }
+
+ /**
+ * MRM-747
+ * test whether trying to overwrite existing relase-artifact is blocked by returning HTTP-code 409
+ *
+ * @throws Exception
+ */
+ public void testPreventOverwritingReleaseArtifacts()
+ throws Exception
+ {
+ setupCleanRepo( repoRootInternal );
+
+ String putUrl = "http://machine.com/repository/internal" + ARTIFACT_DEFAULT_LAYOUT;
+ String metadataUrl = "http://machine.com/repository/internal/path/to/artifact/maven-metadata.xml";
+ String checksumUrl = "http://machine.com/repository/internal" + ARTIFACT_DEFAULT_LAYOUT + ".sha1";
+
+ InputStream is = getClass().getResourceAsStream( "/artifact.jar" );
+ // verify that the file exists in resources-dir
+ assertNotNull( "artifact.jar inputstream", is );
+
+ // send request #1 and verify it's successful
+ WebRequest request = new PutMethodWebRequest( putUrl, is, "application/octet-stream" );
+ WebResponse response = sc.getResponse( request );
+ assertResponseCreated( response );
+
+ is = getClass().getResourceAsStream( "/artifact.jar.sha1" );
+ request = new PutMethodWebRequest( checksumUrl, is, "application/octet-stream" );
+ response = sc.getResponse( request );
+ assertResponseCreated( response );
+
+ is = getClass().getResourceAsStream( "/maven-metadata.xml" );
+ request = new PutMethodWebRequest( metadataUrl, is, "application/octet-stream" );
+ response = sc.getResponse( request );
+ assertResponseCreated( response );
+
+ // send request #2 and verify it's blocked
+ is = getClass().getResourceAsStream( "/artifact.jar" );
+ request = new PutMethodWebRequest( putUrl, is, "application/octet-stream" );
+ response = sc.getResponse( request );
+ assertResponseConflictError( response );
}
public void testMkColWithMissingParentCollectionFails()
Added: archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/test/resources/artifact.jar.sha1
URL: http://svn.apache.org/viewvc/archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src/test/resources/artifact.jar.sha1?rev=824677&view=auto
==============================================================================
(empty)
Re: svn commit: r824677 - in /archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src:
main/java/org/apache/maven/archiva/webdav/ test/java/org/apache/maven/archiva/webdav/
test/resources/
Posted by Deng Ching <oc...@apache.org>.
On Wed, Oct 14, 2009 at 11:40 PM, Brett Porter <br...@apache.org> wrote:
>
>
>>> // MRM-872 : merge all available metadata
>>>
>>>> // merge metadata only when requested via the repo group
>>>> - if ( ( repositoryRequest.isMetadata( requestedResource ) || (
>>>> requestedResource.endsWith( "metadata.xml.sha1" ) ||
>>>> requestedResource.endsWith( "metadata.xml.md5" ) ) )
>>>> - && repoGroupConfig != null )
>>>> + if ( ( repositoryRequest.isMetadata( requestedResource ) || (
>>>> requestedResource.endsWith( "metadata.xml.sha1" ) ||
>>>> requestedResource.endsWith( "metadata.xml.md5" ) ) ) &&
>>>> + repoGroupConfig != null )
>>>>
>>>>
>>> Should this use "isSupportFile" like below? That will cover the two
>>> metadata checksums
>>>
>>
>>
>> .. but it will also get the other non-metadata checksum files so I don't
>> think we can use isSupportFile(..) here
>>
>>
> ok - could the check be moved to the repository request (eg,
> isMetadataSupportFile), so that it is all in one spot?
ok, will move this one over..
>
>
>
>>
>>>
>>> @@ -482,6 +496,35 @@
>>>
>>>>
>>>> if ( request.getMethod().equals( HTTP_PUT_METHOD ) )
>>>> {
>>>> + String resourcePath = logicalResource.getPath();
>>>> +
>>>> + // check if target repo is enabled for releases
>>>> + // we suppose that release-artifacts can deployed only
>>>> to
>>>> repos enabled for releases
>>>> + if ( managedRepository.getRepository().isReleases() &&
>>>> !repositoryRequest.isMetadata( resourcePath ) &&
>>>> + !repositoryRequest.isSupportFile( resourcePath ) )
>>>> + {
>>>> + ArtifactReference artifact = null;
>>>> + try
>>>> + {
>>>> + artifact =
>>>> managedRepository.toArtifactReference(
>>>> resourcePath );
>>>> + }
>>>> + catch ( LayoutException e )
>>>> + {
>>>> + throw new DavException(
>>>> HttpServletResponse.SC_BAD_REQUEST, e );
>>>> + }
>>>> +
>>>> + if ( !VersionUtil.isSnapshot( artifact.getVersion()
>>>> )
>>>> )
>>>> + {
>>>> + // check if artifact already exists
>>>> + if ( managedRepository.hasContent( artifact ) )
>>>> + {
>>>> + log.warn( "Overwriting released artifacts
>>>> is
>>>> not allowed." );
>>>> + throw new
>>>> ReleaseArtifactAlreadyExistsException( managedRepository.getId(),
>>>> +
>>>> "Overwriting released artifacts is not allowed." );
>>>> + }
>>>> + }
>>>> + }
>>>> +
>>>>
>>>>
>>> Is it necessarily a bad request if the reference can't be derived, or
>>> should the check just be skipped?
>>>
>>>
>> I don't think this is just a check though but it's for getting the
>> artifact
>> object and its coordinates. Maybe we could add a fall back for getting the
>> artifact obj & its coordinates when a LayoutException is thrown instead of
>> immediately propagating it as a bad request error?
>>
>
> The check I meant was the VersionUtil bit.
>
> Say you are trying to store /foo.txt using webdav, which is not a valid
> artifact. I believe this was still allowed previously (though I might be
> wrong) - but now it will be a bad request because of the artifact path, when
> all it needs that for is to check if it is a release. Does that make sense?
Ok, I get it now :) I think we still need to check if the version is a
SNAPSHOT or not though, and block deployment if it is a released artifact or
an invalid artifact (LayoutException is thrown) as long as it already exists
in the repository. We're also using the artifact reference to check if it is
in the repository so it's not just the version we're using. I'll see if
there's another way to do this.
Thanks,
Deng
Re: svn commit: r824677 - in /archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src: main/java/org/apache/maven/archiva/webdav/ test/java/org/apache/maven/archiva/webdav/ test/resources/
Posted by Brett Porter <br...@apache.org>.
On 14/10/2009, at 7:03 PM, Deng Ching wrote:
> Hi Brett,
>
> On Tue, Oct 13, 2009 at 8:49 PM, Brett Porter <br...@apache.org>
> wrote:
>
>>
>> On 13/10/2009, at 9:36 PM, oching@apache.org wrote:
>>
>> - resource =
>>> - processRepositoryGroup( request,
>>> archivaLocator,
>>> repoGroupConfig.getRepositories(),
>>> - activePrincipal,
>>> resourcesInAbsolutePath );
>>> + try
>>> + {
>>> + resource =
>>> + processRepositoryGroup( request,
>>> archivaLocator,
>>> repoGroupConfig.getRepositories(),
>>> + activePrincipal,
>>> resourcesInAbsolutePath );
>>> + }
>>> + catch ( ReleaseArtifactAlreadyExistsException e )
>>> + {
>>> + throw new DavException(
>>> HttpServletResponse.SC_CONFLICT );
>>> + }
>>>
>>
>>
>> it might make more sense just to throw this at the source and
>> eliminate the
>> exception, since the result is always the same?
>
>
> agreed :)
>
>
>>
>> // MRM-872 : merge all available metadata
>>> // merge metadata only when requested via the repo group
>>> - if ( ( repositoryRequest.isMetadata( requestedResource )
>>> || (
>>> requestedResource.endsWith( "metadata.xml.sha1" ) ||
>>> requestedResource.endsWith( "metadata.xml.md5" ) ) )
>>> - && repoGroupConfig != null )
>>> + if ( ( repositoryRequest.isMetadata( requestedResource )
>>> || (
>>> requestedResource.endsWith( "metadata.xml.sha1" ) ||
>>> requestedResource.endsWith( "metadata.xml.md5" ) ) ) &&
>>> + repoGroupConfig != null )
>>>
>>
>> Should this use "isSupportFile" like below? That will cover the two
>> metadata checksums
>
>
> .. but it will also get the other non-metadata checksum files so I
> don't
> think we can use isSupportFile(..) here
>
ok - could the check be moved to the repository request (eg,
isMetadataSupportFile), so that it is all in one spot?
>
>>
>>
>> @@ -482,6 +496,35 @@
>>>
>>> if ( request.getMethod().equals( HTTP_PUT_METHOD ) )
>>> {
>>> + String resourcePath = logicalResource.getPath();
>>> +
>>> + // check if target repo is enabled for releases
>>> + // we suppose that release-artifacts can deployed
>>> only to
>>> repos enabled for releases
>>> + if ( managedRepository.getRepository().isReleases
>>> () &&
>>> !repositoryRequest.isMetadata( resourcePath ) &&
>>> + !repositoryRequest.isSupportFile
>>> ( resourcePath ) )
>>> + {
>>> + ArtifactReference artifact = null;
>>> + try
>>> + {
>>> + artifact =
>>> managedRepository.toArtifactReference(
>>> resourcePath );
>>> + }
>>> + catch ( LayoutException e )
>>> + {
>>> + throw new DavException(
>>> HttpServletResponse.SC_BAD_REQUEST, e );
>>> + }
>>> +
>>> + if ( !VersionUtil.isSnapshot
>>> ( artifact.getVersion() )
>>> )
>>> + {
>>> + // check if artifact already exists
>>> + if ( managedRepository.hasContent
>>> ( artifact ) )
>>> + {
>>> + log.warn( "Overwriting released
>>> artifacts is
>>> not allowed." );
>>> + throw new
>>> ReleaseArtifactAlreadyExistsException( managedRepository.getId(),
>>> +
>>> "Overwriting released artifacts is not allowed." );
>>> + }
>>> + }
>>> + }
>>> +
>>>
>>
>> Is it necessarily a bad request if the reference can't be derived, or
>> should the check just be skipped?
>>
>
> I don't think this is just a check though but it's for getting the
> artifact
> object and its coordinates. Maybe we could add a fall back for
> getting the
> artifact obj & its coordinates when a LayoutException is thrown
> instead of
> immediately propagating it as a bad request error?
The check I meant was the VersionUtil bit.
Say you are trying to store /foo.txt using webdav, which is not a
valid artifact. I believe this was still allowed previously (though I
might be wrong) - but now it will be a bad request because of the
artifact path, when all it needs that for is to check if it is a
release. Does that make sense?
>
>
>> Also, given this is a point release (1.2.3), I don't think this
>> kind of
>> functionality change should be imposed on users - can we offer a
>> configuration option?
>>
>
> +1
> I've left the issue open yesterday since this functionality also
> needs to be
> applied to the web upload.
>
> Thanks,
> Deng
>
Thanks!
- Brett
Re: svn commit: r824677 - in /archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src:
main/java/org/apache/maven/archiva/webdav/ test/java/org/apache/maven/archiva/webdav/
test/resources/
Posted by Deng Ching <oc...@apache.org>.
Hi Brett,
On Tue, Oct 13, 2009 at 8:49 PM, Brett Porter <br...@apache.org> wrote:
>
> On 13/10/2009, at 9:36 PM, oching@apache.org wrote:
>
> - resource =
>> - processRepositoryGroup( request, archivaLocator,
>> repoGroupConfig.getRepositories(),
>> - activePrincipal,
>> resourcesInAbsolutePath );
>> + try
>> + {
>> + resource =
>> + processRepositoryGroup( request, archivaLocator,
>> repoGroupConfig.getRepositories(),
>> + activePrincipal,
>> resourcesInAbsolutePath );
>> + }
>> + catch ( ReleaseArtifactAlreadyExistsException e )
>> + {
>> + throw new DavException(
>> HttpServletResponse.SC_CONFLICT );
>> + }
>>
>
>
> it might make more sense just to throw this at the source and eliminate the
> exception, since the result is always the same?
agreed :)
>
> // MRM-872 : merge all available metadata
>> // merge metadata only when requested via the repo group
>> - if ( ( repositoryRequest.isMetadata( requestedResource ) || (
>> requestedResource.endsWith( "metadata.xml.sha1" ) ||
>> requestedResource.endsWith( "metadata.xml.md5" ) ) )
>> - && repoGroupConfig != null )
>> + if ( ( repositoryRequest.isMetadata( requestedResource ) || (
>> requestedResource.endsWith( "metadata.xml.sha1" ) ||
>> requestedResource.endsWith( "metadata.xml.md5" ) ) ) &&
>> + repoGroupConfig != null )
>>
>
> Should this use "isSupportFile" like below? That will cover the two
> metadata checksums
.. but it will also get the other non-metadata checksum files so I don't
think we can use isSupportFile(..) here
>
>
> @@ -482,6 +496,35 @@
>>
>> if ( request.getMethod().equals( HTTP_PUT_METHOD ) )
>> {
>> + String resourcePath = logicalResource.getPath();
>> +
>> + // check if target repo is enabled for releases
>> + // we suppose that release-artifacts can deployed only to
>> repos enabled for releases
>> + if ( managedRepository.getRepository().isReleases() &&
>> !repositoryRequest.isMetadata( resourcePath ) &&
>> + !repositoryRequest.isSupportFile( resourcePath ) )
>> + {
>> + ArtifactReference artifact = null;
>> + try
>> + {
>> + artifact = managedRepository.toArtifactReference(
>> resourcePath );
>> + }
>> + catch ( LayoutException e )
>> + {
>> + throw new DavException(
>> HttpServletResponse.SC_BAD_REQUEST, e );
>> + }
>> +
>> + if ( !VersionUtil.isSnapshot( artifact.getVersion() )
>> )
>> + {
>> + // check if artifact already exists
>> + if ( managedRepository.hasContent( artifact ) )
>> + {
>> + log.warn( "Overwriting released artifacts is
>> not allowed." );
>> + throw new
>> ReleaseArtifactAlreadyExistsException( managedRepository.getId(),
>> +
>> "Overwriting released artifacts is not allowed." );
>> + }
>> + }
>> + }
>> +
>>
>
> Is it necessarily a bad request if the reference can't be derived, or
> should the check just be skipped?
>
I don't think this is just a check though but it's for getting the artifact
object and its coordinates. Maybe we could add a fall back for getting the
artifact obj & its coordinates when a LayoutException is thrown instead of
immediately propagating it as a bad request error?
> Also, given this is a point release (1.2.3), I don't think this kind of
> functionality change should be imposed on users - can we offer a
> configuration option?
>
+1
I've left the issue open yesterday since this functionality also needs to be
applied to the web upload.
Thanks,
Deng
> Cheers,
> Brett
>
>
Re: svn commit: r824677 - in /archiva/trunk/archiva-modules/archiva-web/archiva-webdav/src: main/java/org/apache/maven/archiva/webdav/ test/java/org/apache/maven/archiva/webdav/ test/resources/
Posted by Brett Porter <br...@apache.org>.
On 13/10/2009, at 9:36 PM, oching@apache.org wrote:
> - resource =
> - processRepositoryGroup( request,
> archivaLocator, repoGroupConfig.getRepositories(),
> - activePrincipal,
> resourcesInAbsolutePath );
> + try
> + {
> + resource =
> + processRepositoryGroup( request,
> archivaLocator, repoGroupConfig.getRepositories(),
> + activePrincipal,
> resourcesInAbsolutePath );
> + }
> + catch ( ReleaseArtifactAlreadyExistsException e )
> + {
> + throw new DavException
> ( HttpServletResponse.SC_CONFLICT );
> + }
it might make more sense just to throw this at the source and
eliminate the exception, since the result is always the same?
> // MRM-872 : merge all available metadata
> // merge metadata only when requested via the repo group
> - if ( ( repositoryRequest.isMetadata( requestedResource ) ||
> ( requestedResource.endsWith( "metadata.xml.sha1" ) ||
> requestedResource.endsWith( "metadata.xml.md5" ) ) )
> - && repoGroupConfig != null )
> + if ( ( repositoryRequest.isMetadata( requestedResource ) ||
> ( requestedResource.endsWith( "metadata.xml.sha1" ) ||
> requestedResource.endsWith( "metadata.xml.md5" ) ) ) &&
> + repoGroupConfig != null )
Should this use "isSupportFile" like below? That will cover the two
metadata checksums
> @@ -482,6 +496,35 @@
>
> if ( request.getMethod().equals( HTTP_PUT_METHOD ) )
> {
> + String resourcePath = logicalResource.getPath();
> +
> + // check if target repo is enabled for releases
> + // we suppose that release-artifacts can deployed
> only to repos enabled for releases
> + if ( managedRepository.getRepository().isReleases()
> && !repositoryRequest.isMetadata( resourcePath ) &&
> + !repositoryRequest.isSupportFile
> ( resourcePath ) )
> + {
> + ArtifactReference artifact = null;
> + try
> + {
> + artifact =
> managedRepository.toArtifactReference( resourcePath );
> + }
> + catch ( LayoutException e )
> + {
> + throw new DavException
> ( HttpServletResponse.SC_BAD_REQUEST, e );
> + }
> +
> + if ( !VersionUtil.isSnapshot
> ( artifact.getVersion() ) )
> + {
> + // check if artifact already exists
> + if ( managedRepository.hasContent
> ( artifact ) )
> + {
> + log.warn( "Overwriting released
> artifacts is not allowed." );
> + throw new
> ReleaseArtifactAlreadyExistsException( managedRepository.getId(),
> +
> "Overwriting
> released artifacts is not allowed." );
> + }
> + }
> + }
> +
Is it necessarily a bad request if the reference can't be derived, or
should the check just be skipped?
Also, given this is a point release (1.2.3), I don't think this kind
of functionality change should be imposed on users - can we offer a
configuration option?
Cheers,
Brett