[auth][api] QgsAuthConfigurationStorage classes and tests

[elpaso]

Abstract authentication credentials storage

Implementation of https://github.com/qgis/QGIS-Enhancement-Proposals/issues/248

Goal

Provide an abstract API to manage authentication configurations storage. The new API supports any database supported by QSqlDatabase (https://doc.qt.io/qt-5/qsqldatabase.html) including SQLite.

Additional features:

• support non-DB sources through Python plugins or new C++ implementations
• support multiple sources through a new QgsAuthConfigurationStorageRegistry that holds the (ordered) list of authentication storages available to the system
• granular CRUD capabilities to allow storages to support only a subset of the auth assets that are currently supported by the QGIS auth system
• support for both encrypted and unencrypted storages (for external third party storages that handle encryption themselves)

[github-actions[bot]]

🪟 Windows builds ready!

Windows builds of this PR are available for testing here. Debug symbols for this build are available here.

(Built from commit fdb28748cad62aa4cd120c8d1f300aa81cd1a81b)

[elpaso]

@nyalldawson I have applied the recommended changes: if the default storage is not local (SQLITE) it is added read-only. I have also added some more tests for these cases and made sure the GUI behaves correctly with a read-only storage.

Managing multiple storages from the GUI is out of scope for now but the API supports multiple as well as custom storages.

[elpaso]

@3nids do you have any idea what's happening with sipify? It is driving me mad, first the map yaml files and now it is not even running locally, it gives multiple errors like

Traceback (most recent call last):
  File "/home/ale/dev/QGIS/./scripts/sipify.py", line 2214, in <module>
    CONTEXT.current_line = fix_annotations(CONTEXT.current_line)
  File "/home/ale/dev/QGIS/./scripts/sipify.py", line 1061, in fix_annotations
    line = re.sub(
  File "/usr/lib/python3.10/re.py", line 209, in sub
    return _compile(pattern, flags).sub(repl, string, count)
  File "/usr/lib/python3.10/re.py", line 303, in _compile
    p = sre_compile.compile(pattern, flags)
  File "/usr/lib/python3.10/sre_compile.py", line 788, in compile
    p = sre_parse.parse(p, flags)
  File "/usr/lib/python3.10/sre_parse.py", line 955, in parse
    p = _parse_sub(source, state, flags & SRE_FLAG_VERBOSE, 0)
  File "/usr/lib/python3.10/sre_parse.py", line 444, in _parse_sub
    itemsappend(_parse(source, state, verbose, nested + 1,
  File "/usr/lib/python3.10/sre_parse.py", line 841, in _parse
    p = _parse_sub(source, state, sub_verbose, nested + 1)
  File "/usr/lib/python3.10/sre_parse.py", line 444, in _parse_sub
    itemsappend(_parse(source, state, verbose, nested + 1,
  File "/usr/lib/python3.10/sre_parse.py", line 841, in _parse
    p = _parse_sub(source, state, sub_verbose, nested + 1)
  File "/usr/lib/python3.10/sre_parse.py", line 444, in _parse_sub
    itemsappend(_parse(source, state, verbose, nested + 1,
  File "/usr/lib/python3.10/sre_parse.py", line 841, in _parse
    p = _parse_sub(source, state, sub_verbose, nested + 1)
  File "/usr/lib/python3.10/sre_parse.py", line 444, in _parse_sub
    itemsappend(_parse(source, state, verbose, nested + 1,
  File "/usr/lib/python3.10/sre_parse.py", line 672, in _parse
    raise source.error("multiple repeat",
re.error: multiple repeat at position 124

[3nids]

@elpaso Nyall has moved sipify from Perl to Python, there might some remaining issues. Regarding the map files, no need to update them from the PR.

[nyalldawson]

@elpaso sorry about that, the fix is in https://github.com/qgis/QGIS/pull/58372

[nyalldawson]

@elpaso what about the explicit exceptions we discussed when storeAuthSetting etc are called on a non-user specific store?

If I recall correctly we agreed that these would raise an exception with the error QgsNotSupportedException: "This method is not supported by your authentication configuration. Please contact your local QGIS administrator for support."

[elpaso]

@elpaso what about the explicit exceptions we discussed when storeAuthSetting etc are called on a non-user specific store?

If I recall correctly we agreed that these would raise an exception with the error QgsNotSupportedException: "This method is not supported by your authentication configuration. Please contact your local QGIS administrator for support."

Yes, I did not forget it but decided it wasn't appropriate: all the methods return False in case of storage failure and the last error is set to indicate the reason, I have also expanded the support for read-only storages (that because are fully supported do not need throw any exception in case of writing failure).

Also, the DB storages based non-filesystem based DBs are added read-only by default.

[nyalldawson]

@elpaso

Yes, I did not forget it but decided it wasn't appropriate: all the methods return False in case of storage failure and the last error is set to indicate the reason, I have also expanded the support for read-only storages (that because are fully supported do not need throw any exception in case of writing failure).

I'm disappointed by that. I think we have an opportunity here to make the PyQGIS API more friendly to developers, and instead we're opting for an opaque, unfriendly API. Returning false isn't very helpful at all -- it's going to cost someone hours in debugging time to realise what's happening there, and then they'll be forced to dig into the QGIS source to work out why it's returning false in the first place. Vs adding the exception, which is a minor change here, and which INSTANTLY gives the PyQGIS developer helpful feedback about what's gone wrong and how they can handle this...

Also, the DB storages based non-filesystem based DBs are added read-only by default.

In that case IMO all DB storages should raise an explicit "QgsAuthenticationException" whenever a write method is called.

[elpaso]

@elpaso

Yes, I did not forget it but decided it wasn't appropriate: all the methods return False in case of storage failure and the last error is set to indicate the reason, I have also expanded the support for read-only storages (that because are fully supported do not need throw any exception in case of writing failure).

I'm disappointed by that. I think we have an opportunity here to make the PyQGIS API more friendly to developers, and instead we're opting for an opaque, unfriendly API. Returning false isn't very helpful at all -- it's going to cost someone hours in debugging time to realise what's happening there, and then they'll be forced to dig into the QGIS source to work out why it's returning false in the first place. Vs adding the exception, which is a minor change here, and which INSTANTLY gives the PyQGIS developer helpful feedback about what's gone wrong and how they can handle this...

Also, the DB storages based non-filesystem based DBs are added read-only by default.

In that case IMO all DB storages should raise an explicit "QgsAuthenticationException" whenever a write method is called.

@nyalldawson Before I change all the methods can you please clarify whether you want the methods to return void and raise whenever the operation did not succeed (regardless of the reason) or you want the methods to still return bool and only raise when the attempted operation was not allowed/permitted.

These are two different situations: image that the storage is a local sqlite with RW (and delete) capabilities but a delete operation fails because the item to be deleted could not be found in the storage: do you want that to raise and exception or just return false? On the other hand, an attempt to write on a RO storage should raise an exception?

If you opt for return void and always raise I think we would need different exceptions for the different cases: unsupported operation vs. other errors.

[nyalldawson]

@elpaso

Before I change all the methods can you please clarify whether you want the methods to return void and raise whenever the operation did not succeed (regardless of the reason) or you want the methods to still return bool and only raise when the attempted operation was not allowed/permitted.

I'd lean toward exceptions whenever they didn't succeed, regardless of the reason. But I'd be happy with exceptions for not supported methods + return code for the semi-expected failures. Whatever's easiest.

These are two different situations: image that the storage is a local sqlite with RW (and delete) capabilities but a delete operation fails because the item to be deleted could not be found in the storage: do you want that to raise and exception or just return false? On the other hand, an attempt to write on a RO storage should raise an exception?

If you opt for return void and always raise I think we would need different exceptions for the different cases: unsupported operation vs. other errors.

What about we use the existing QgsNotSupportedException for whenever the backend explicitly denies an operation -- I think that's a good fit. (Including calling a write method on a read only backend). If we opt for exceptions on other failures then I think we should introduce a new QgsAuthenticationException for those.

[elpaso]

@nyalldawson the implementation now raises on unsupported operations, can you please make another review?

[jef-n]

@elpaso https://cdash.orfeo-toolbox.org/viewBuildError.php?buildid=26100

[elpaso]

@elpaso https://cdash.orfeo-toolbox.org/viewBuildError.php?buildid=26100

Maybe fixed by https://github.com/qgis/QGIS/pull/58719 ?