add detection of explicit vsi-prefix in URI-address #59586

notguiltyspark · 2024-11-25T17:03:13Z

Description

As of now, process of adding vector layer via URI-address with already-present prefix does not detect this prefix, which results in its prepending to the final string, which leads to the error in query.

This PR fixes it, scanning input uri-address beforehand and checking if it can detect prefixes from the usual protocols list.

github-actions · 2024-11-25T17:27:47Z

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit 7686a6a)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 7686a6a)

rouault · 2024-11-25T18:16:37Z

src/gui/ogr/qgsgdalguiutils.cpp

    {
-      uri.prepend( QStringLiteral( "http://" ) );
+      // If no protocol is provided in the URL, default to HTTP
+      if ( !uri.startsWith( "http://" ) && !uri.startsWith( "https://" ) && !uri.startsWith( "ftp://" ) )


Suggested change

if ( !uri.startsWith( "http://" ) && !uri.startsWith( "https://" ) && !uri.startsWith( "ftp://" ) )

if ( !uri.startsWith( QLatin1String("http://") ) && !uri.startsWith( QLatin1String("https://") ) && !uri.startsWith( QLatin1String("ftp://") ) )

nyalldawson · 2024-11-29T00:28:43Z

Can you add some tests for this too please?

notguiltyspark · 2024-12-11T10:34:14Z

Added tests

nyalldawson · 2024-12-12T01:02:02Z

tests/src/gui/testqgsgdalguiutils.cpp

+  cleanupTempDir();
+
+  // make QGIS_AUTH_DB_DIR_PATH temp dir for qgis-auth.db and master password file
+  const QDir tmpDir = QDir::temp();
+  QVERIFY2( tmpDir.mkpath( mTempDir ), "Couldn't make temp directory" );
+  qputenv( "QGIS_AUTH_DB_DIR_PATH", mTempDir.toLatin1() );
+
+  // init app and auth manager
+  QgsApplication::init();
+  QgsApplication::initQgis();
+  QVERIFY2( !QgsApplication::authManager()->isDisabled(), "Authentication system is DISABLED" );
+
+  // verify QGIS_AUTH_DB_DIR_PATH (temp auth db path) worked
+  Q_NOWARN_DEPRECATED_PUSH
+  const QString db1( QFileInfo( QgsApplication::authManager()->authenticationDatabasePath() ).canonicalFilePath() );
+  Q_NOWARN_DEPRECATED_POP
+  const QString db2( QFileInfo( mTempDir + "/qgis-auth.db" ).canonicalFilePath() );
+  QCOMPARE( db1, db2 );
+
+  // verify master pass can be set manually
+  // (this also creates a fresh password hash in the new temp database)
+  QVERIFY2( QgsApplication::authManager()->setMasterPassword( mPass, true ), "Master password could not be set" );
+  QVERIFY2( QgsApplication::authManager()->masterPasswordIsSet(), "Auth master password not set from passed string" );
+
+  // create QGIS_AUTH_PASSWORD_FILE file
+  const QString passfilepath = mTempDir + "/passfile";
+  QFile passfile( passfilepath );
+  if ( passfile.open( QIODevice::WriteOnly | QIODevice::Text | QIODevice::Truncate ) )
+  {
+    QTextStream fout( &passfile );
+    fout << QString( mPass ) << "\r\n";
+    passfile.close();
+    qputenv( "QGIS_AUTH_PASSWORD_FILE", passfilepath.toLatin1() );
+  }
+  // qDebug( "QGIS_AUTH_PASSWORD_FILE=%s", qgetenv( "QGIS_AUTH_PASSWORD_FILE" ).constData() );
+
+  // re-init app and auth manager
+  QgsApplication::quit();
+  // QTest::qSleep( 3000 );


None of this is required, there's no tests which are actually taking advantage of the auth db

Suggested change

cleanupTempDir();

// make QGIS_AUTH_DB_DIR_PATH temp dir for qgis-auth.db and master password file

const QDir tmpDir = QDir::temp();

QVERIFY2( tmpDir.mkpath( mTempDir ), "Couldn't make temp directory" );

qputenv( "QGIS_AUTH_DB_DIR_PATH", mTempDir.toLatin1() );

// init app and auth manager

QgsApplication::init();

QgsApplication::initQgis();

QVERIFY2( !QgsApplication::authManager()->isDisabled(), "Authentication system is DISABLED" );

// verify QGIS_AUTH_DB_DIR_PATH (temp auth db path) worked

Q_NOWARN_DEPRECATED_PUSH

const QString db1( QFileInfo( QgsApplication::authManager()->authenticationDatabasePath() ).canonicalFilePath() );

Q_NOWARN_DEPRECATED_POP

const QString db2( QFileInfo( mTempDir + "/qgis-auth.db" ).canonicalFilePath() );

QCOMPARE( db1, db2 );

// verify master pass can be set manually

// (this also creates a fresh password hash in the new temp database)

QVERIFY2( QgsApplication::authManager()->setMasterPassword( mPass, true ), "Master password could not be set" );

QVERIFY2( QgsApplication::authManager()->masterPasswordIsSet(), "Auth master password not set from passed string" );

// create QGIS_AUTH_PASSWORD_FILE file

const QString passfilepath = mTempDir + "/passfile";

QFile passfile( passfilepath );

if ( passfile.open( QIODevice::WriteOnly | QIODevice::Text | QIODevice::Truncate ) )

{

QTextStream fout( &passfile );

fout << QString( mPass ) << "\r\n";

passfile.close();

qputenv( "QGIS_AUTH_PASSWORD_FILE", passfilepath.toLatin1() );

}

// qDebug( "QGIS_AUTH_PASSWORD_FILE=%s", qgetenv( "QGIS_AUTH_PASSWORD_FILE" ).constData() );

// re-init app and auth manager

QgsApplication::quit();

// QTest::qSleep( 3000 );

None of this is required, there's no tests which are actually taking advantage of the auth db

If we set flag expandAuthConfig and set some config-string to configId we enter this block in QgsGdalGuiUtils::createProtocolURI:

// Update URI with authentication information if ( !configId.isEmpty() ) { if ( expandAuthConfig ) { QStringList connectionItems; connectionItems << uri; if ( QgsApplication::authManager()->updateDataSourceUriItems( connectionItems, configId, QStringLiteral( "ogr" ) ) ) { uri = connectionItems.join( QString() ); } } else { uri += QStringLiteral( " authcfg='%1'" ).arg( configId ); } }

So there seems to be a possibility or combination of circumstances in which we need authentication manager?

If so - these tests are prerequisites/preparations to initializing and using it, if i understand correctly, since it is not self-contained (i.e. couchDB test was failing until authentication manager was initialized correctly with authentication method and master setting of masterpassword due to incorrect auth-db creation and reading).

If not - i will remove them according to your suggestions, just making sure that i understand correctly

I'm referring to the fact that none of the tests introduced here actually require the auth configuration. Theres '// Test with configId and expandAuthConfig = false', but that isn't requiring the auth.

Then there's QgsGdalGuiUtils::createProtocolURI( QLatin1String( "CouchDB" ), QLatin1String( "mydb" ), QLatin1String( "ab34567" ), QString(), QString(), true );, but that's not actually doing any replacement either.

I see, i updated tests according to suggestions

nyalldawson · 2024-12-12T01:02:12Z

tests/src/gui/testqgsgdalguiutils.cpp

+  QVERIFY2( !QgsApplication::authManager()->isDisabled(), "Authentication system is DISABLED" );
+
+  // verify QGIS_AUTH_PASSWORD_FILE worked, when compared against hash in db
+  QVERIFY2( QgsApplication::authManager()->masterPasswordIsSet(), "Auth master password not set from QGIS_AUTH_PASSWORD_FILE" );
+
+  // all tests should now have a valid qgis-auth.db and stored/set master password


Suggested change

QVERIFY2( !QgsApplication::authManager()->isDisabled(), "Authentication system is DISABLED" );

// verify QGIS_AUTH_PASSWORD_FILE worked, when compared against hash in db

QVERIFY2( QgsApplication::authManager()->masterPasswordIsSet(), "Auth master password not set from QGIS_AUTH_PASSWORD_FILE" );

// all tests should now have a valid qgis-auth.db and stored/set master password

nyalldawson · 2024-12-12T01:02:25Z

tests/src/gui/testqgsgdalguiutils.cpp

+  QList<QgsAuthMethodConfig> lAuthConfigs = registerAuthConfigs();
+  for ( auto &authcfg : lAuthConfigs )
+  {
+    QgsApplication::instance()->authManager()->storeAuthenticationConfig( authcfg, true );
+  }
+


Suggested change

QList<QgsAuthMethodConfig> lAuthConfigs = registerAuthConfigs();

for ( auto &authcfg : lAuthConfigs )

{

QgsApplication::instance()->authManager()->storeAuthenticationConfig( authcfg, true );

}

nyalldawson · 2024-12-12T01:04:55Z

tests/src/gui/testqgsgdalguiutils.cpp

+  cleanupTempDir();
+}
+
+void TestQgsGdalGuiUtils::cleanupTempDir()
+{
+  QString mTempDir = QDir::tempPath() + "/auth";
+
+  QDir tmpDir = QDir( mTempDir );
+  if ( tmpDir.exists() )
+  {
+    for ( const QString &tf : tmpDir.entryList( QDir::NoDotAndDotDot | QDir::Files ) )
+    {
+      QVERIFY2( tmpDir.remove( mTempDir + '/' + tf ), qPrintable( "Could not remove " + mTempDir + '/' + tf ) );
+    }
+    QVERIFY2( tmpDir.rmdir( mTempDir ), qPrintable( "Could not remove directory " + mTempDir ) );
+  }
+}
+
+QList<QgsAuthMethodConfig> TestQgsGdalGuiUtils::registerAuthConfigs()
+{
+  QList<QgsAuthMethodConfig> configs;
+
+  // Basic
+  QgsAuthMethodConfig b_config;
+  b_config.setName( QStringLiteral( "Basic" ) );
+  b_config.setMethod( QStringLiteral( "Basic" ) );
+  b_config.setUri( QStringLiteral( "http://example.com" ) );
+  b_config.setConfig( QStringLiteral( "username" ), QStringLiteral( "username" ) );
+  b_config.setConfig( QStringLiteral( "password" ), QStringLiteral( "password" ) );
+  b_config.setConfig( QStringLiteral( "realm" ), QStringLiteral( "Realm" ) );
+  b_config.setId( QStringLiteral( "ab34567" ) );
+  if ( !b_config.isValid() )
+  {
+    return configs;
+  }
+
+  configs << b_config;
+  return configs;


Suggested change

cleanupTempDir();

}

void TestQgsGdalGuiUtils::cleanupTempDir()

{

QString mTempDir = QDir::tempPath() + "/auth";

QDir tmpDir = QDir( mTempDir );

if ( tmpDir.exists() )

{

for ( const QString &tf : tmpDir.entryList( QDir::NoDotAndDotDot | QDir::Files ) )

{

QVERIFY2( tmpDir.remove( mTempDir + '/' + tf ), qPrintable( "Could not remove " + mTempDir + '/' + tf ) );

}

QVERIFY2( tmpDir.rmdir( mTempDir ), qPrintable( "Could not remove directory " + mTempDir ) );

}

}

QList<QgsAuthMethodConfig> TestQgsGdalGuiUtils::registerAuthConfigs()

{

QList<QgsAuthMethodConfig> configs;

// Basic

QgsAuthMethodConfig b_config;

b_config.setName( QStringLiteral( "Basic" ) );

b_config.setMethod( QStringLiteral( "Basic" ) );

b_config.setUri( QStringLiteral( "http://example.com" ) );

b_config.setConfig( QStringLiteral( "username" ), QStringLiteral( "username" ) );

b_config.setConfig( QStringLiteral( "password" ), QStringLiteral( "password" ) );

b_config.setConfig( QStringLiteral( "realm" ), QStringLiteral( "Realm" ) );

b_config.setId( QStringLiteral( "ab34567" ) );

if ( !b_config.isValid() )

{

return configs;

}

configs << b_config;

return configs;

github-actions · 2024-12-30T02:47:58Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

github-actions · 2025-01-16T02:46:00Z

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

that all unit tests are passing
that all comments by reviewers have been addressed
that there is enough information for reviewers, in particular
- link to any issues which this pull request fixes
- add a description of workflows which this pull request fixes
- add screenshots if applicable
that you have written unit tests where possible
In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
If there is no further activity on this pull request, it will be closed in a week.

github-actions bot added this to the 3.42.0 milestone Nov 25, 2024

rouault reviewed Nov 25, 2024

View reviewed changes

nyalldawson added the Requires Tests! Waiting on the submitter to add unit tests before eligible for merging label Dec 2, 2024

notguiltyspark force-pushed the fix_explicit_vsicurl_prefix branch from 7f7b6d5 to 297fb7b Compare December 11, 2024 10:17

notguiltyspark force-pushed the fix_explicit_vsicurl_prefix branch 3 times, most recently from edfa0ba to 3ca74e5 Compare December 11, 2024 10:52

nyalldawson reviewed Dec 12, 2024

View reviewed changes

github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Dec 30, 2024

notguiltyspark force-pushed the fix_explicit_vsicurl_prefix branch from 3ca74e5 to 5ac18f0 Compare January 1, 2025 21:29

github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jan 1, 2025

github-actions bot added the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jan 16, 2025

notguiltyspark force-pushed the fix_explicit_vsicurl_prefix branch from 5ac18f0 to 5da878b Compare January 19, 2025 19:30

github-actions bot removed the stale Uh oh! Seems this work is abandoned, and the PR is about to close. label Jan 19, 2025

notguiltyspark force-pushed the fix_explicit_vsicurl_prefix branch from 5da878b to f4c58c3 Compare January 19, 2025 19:40

fix explicit vsicurl prefix

7686a6a

notguiltyspark force-pushed the fix_explicit_vsicurl_prefix branch from f4c58c3 to 7686a6a Compare January 19, 2025 21:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add detection of explicit vsi-prefix in URI-address #59586

add detection of explicit vsi-prefix in URI-address #59586

notguiltyspark commented Nov 25, 2024

github-actions bot commented Nov 25, 2024 •

edited

Loading

rouault Nov 25, 2024

nyalldawson commented Nov 29, 2024

notguiltyspark commented Dec 11, 2024

nyalldawson Dec 12, 2024

notguiltyspark Dec 12, 2024 •

edited

Loading

nyalldawson Dec 16, 2024

notguiltyspark Jan 19, 2025

nyalldawson Dec 12, 2024

nyalldawson Dec 12, 2024

nyalldawson Dec 12, 2024

github-actions bot commented Dec 30, 2024

github-actions bot commented Jan 16, 2025

	if ( !uri.startsWith( "http://" ) && !uri.startsWith( "https://" ) && !uri.startsWith( "ftp://" ) )
	if ( !uri.startsWith( QLatin1String("http://") ) && !uri.startsWith( QLatin1String("https://") ) && !uri.startsWith( QLatin1String("ftp://") ) )

add detection of explicit vsi-prefix in URI-address #59586

Are you sure you want to change the base?

add detection of explicit vsi-prefix in URI-address #59586

Conversation

notguiltyspark commented Nov 25, 2024

Description

github-actions bot commented Nov 25, 2024 • edited Loading

🪟 Windows builds

🪟 Windows Qt6 builds

rouault Nov 25, 2024

Choose a reason for hiding this comment

nyalldawson commented Nov 29, 2024

notguiltyspark commented Dec 11, 2024

nyalldawson Dec 12, 2024

Choose a reason for hiding this comment

notguiltyspark Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

nyalldawson Dec 16, 2024

Choose a reason for hiding this comment

notguiltyspark Jan 19, 2025

Choose a reason for hiding this comment

nyalldawson Dec 12, 2024

Choose a reason for hiding this comment

nyalldawson Dec 12, 2024

Choose a reason for hiding this comment

nyalldawson Dec 12, 2024

Choose a reason for hiding this comment

github-actions bot commented Dec 30, 2024

github-actions bot commented Jan 16, 2025

github-actions bot commented Nov 25, 2024 •

edited

Loading

notguiltyspark Dec 12, 2024 •

edited

Loading