[auth][api] QgsAuthConfigurationStorage classes and tests #57992

elpaso · 2024-07-05T07:03:06Z

Abstract authentication credentials storage

Implementation of qgis/QGIS-Enhancement-Proposals#248

Donors

This work was financed with funds from:

Gis3W https://gis3w.it/
QGIS Grant program
Hytech-imaging, spectral imaging for environmental mapping and monitoring in civil and defence applications https://hytech-imaging.fr/
QTIBIA ENGINEERING https://qtibia.com/
QGIS-DE - Anwendergruppe Deutschland https://www.qgis.de/

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)

src/core/auth/qgsauthconfigurationstorage.h

src/core/CMakeLists.txt

nyalldawson · 2024-07-09T02:48:13Z

src/core/auth/qgsauthconfigurationstorage.h

+ *  Abstract class that defines the interface for all authentication configuration storage implementations.
+ *  \since QGIS 3.40
+ */
+class CORE_EXPORT QgsAuthConfigurationStorage: public QObject SIP_ABSTRACT


Suggested change

class CORE_EXPORT QgsAuthConfigurationStorage: public QObject SIP_ABSTRACT

class CORE_EXPORT QgsAbstractAuthConfigurationStorage: public QObject SIP_ABSTRACT

I'd argue that "storage" in this class name is needlessly restrictive. Perhaps QgsAbstractAuthBackend is a better choice?

src/core/auth/storage/qgsauthconfigurationstoragedb.cpp

nyalldawson · 2024-07-09T03:02:57Z

src/core/auth/storage/qgsauthconfigurationstoragedb.cpp

+
+bool QgsAuthConfigurationStorageDb::authDbOpen() const
+{
+  QMutexLocker locker( &mMutex );


Are all these implementations 1:1 copies of the current code? Or are there modifications I should look over specifically?

They are mostly 1:1 copies but I did not just cut & paste. I'd say that you can defer the review of the CRUD methods after finish writing the tests, I will probably intercept most issues while testing.

src/core/auth/qgsauthconfigurationstorage.h

src/core/qgis.h

github-actions · 2024-07-09T11:25:06Z

🪟 Windows builds ready!

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

(Built from commit fdb2874)

src/core/auth/qgsauthconfigurationstorage.h

src/core/auth/qgsauthconfigurationstorageregistry.h

src/core/auth/qgsauthconfigurationstorageregistry.cpp

src/core/qgis.h

nyalldawson · 2024-07-29T00:22:26Z

src/core/auth/qgsauthconfigurationstoragedb.cpp

+  }
+
+  QSqlQuery query( authDatabaseConnection() );
+


More of a design question than a code question -- but I'm curious how this method will work in a multi-user setup? Eg a number of plugins I've come across (and some I maintain) use this method to securely store API tokens after a user authenticates with a service. So I'm wondering what would happen if there's a central auth db setup (say on postgres), and then a plugin attempts to store something inherently user-specific like a API token? Either the db will be locked down and prevent this, or they'll clobber their work colleagues' token. Both situations are bad, and would ultimately break the plugin. 😱 (Or is the intention here that a centrally managed auth db is siloed into completely separate user "buckets" via db-level handling? )

Unless this situation is gracefully handled I can see this being a blocker for real-world use cases.

Let's make a step back: the original goal was to provide a way to store credentials in a real client-server DB to remove the filesystem sqlite DB which is causing a lot of issues with QGIS server deployments in the cloud.

For the desktop there is an enterprise use case which I think is interesting (and I spotted in the wild when working with big government orgs in the past): centrally managed shared credentials for company's services.

With the new API you can have read/only storages and you can have multiple storages, so you can imagine using a read/write local storage for user overrides.

Also, you can subclass QgsAuthConfigurationStorage (if not SIP_ABSTRACT) or QgsAuthConfigurationStorageDb and create your own accessors to the DB with filters for the user.

I thought about adding a 'user' field or something similar to the tables but I thought it was too much specific and it wouldn't ever cover all use cases.

Coming back to your use case (if I get this right): if a plugin uses the auth system for its own secure storage it would probably be capable of managing its own keys naming convention to avoid clashes with other users.

elpaso · 2024-08-13T13:43:31Z

@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 · 2024-08-13T13:55:37Z

@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 · 2024-08-13T14:31:44Z

@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 · 2024-08-13T23:46:16Z

@elpaso sorry about that, the fix is in #58372

nyalldawson · 2024-08-13T23:49:08Z

@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 · 2024-08-23T05:54:16Z

@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 · 2024-08-26T04:59:21Z

@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 · 2024-09-01T09:19:34Z

@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.

jef-n · 2024-09-13T08:52:21Z

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

elpaso · 2024-09-13T08:57:43Z

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

Maybe fixed by #58719 ?

Implementation of QEP Authentication System: allow Database storage for authentication DB qgis/QGIS-Enhancement-Proposals#248

elpaso added API API improvement only, no visible user interface changes Authentication Related to the QGIS Authentication subsystem or user/password handling NOT FOR MERGE Don't merge! labels Jul 5, 2024

elpaso marked this pull request as draft July 5, 2024 07:03

github-actions bot added this to the 3.40.0 milestone Jul 5, 2024

nyalldawson reviewed Jul 9, 2024

View reviewed changes

elpaso force-pushed the authmanager-storage-api branch from 54117f8 to 40ddba2 Compare July 9, 2024 09:10

elpaso force-pushed the authmanager-storage-api branch 5 times, most recently from 50b2786 to d93a8c2 Compare July 15, 2024 14:03

elpaso force-pushed the authmanager-storage-api branch 2 times, most recently from 04b6f9b to da447d3 Compare July 18, 2024 10:32

elpaso marked this pull request as ready for review July 18, 2024 17:29

elpaso removed the NOT FOR MERGE Don't merge! label Jul 18, 2024

nyalldawson reviewed Jul 29, 2024

View reviewed changes

elpaso force-pushed the authmanager-storage-api branch from ea9842d to a9961a1 Compare August 5, 2024 08:37

elpaso force-pushed the authmanager-storage-api branch 2 times, most recently from ed7845a to 77b4b83 Compare August 13, 2024 13:40

elpaso force-pushed the authmanager-storage-api branch from 77b4b83 to 6bbfb40 Compare August 23, 2024 06:03

elpaso added 25 commits September 12, 2024 09:13

Remove same thread protection and add a mutex

7301e78

Better error messages in case capability is not on

dada517

Fix DB URI and schema

339408a

Auh DB uri: integration and tests

1bb5169

Address PR comments

3a61628

First methods

1999c54

Check caps for read-only

2af639b

Manage read-only default DB storage

c7e3167

class map

b499cd4

Remove custom model

ac084c2

Fix capabilities update after read-only change

d68ab6f

sipify

12ecd75

sipify

d03850e

sipify

0b9ac78

Throw exceptions on unsupported calls

85f2795

sipify

5d78c45

sipify

af4e4c6

layout

2ee6e9a

sipify

a615df4

Fix id and add test for RO update

b083cb7

sipify

de6c6bf

Fix build and test

7256a7a

Skip test

4332aac

SIP_CONCAT_PARTS 25

0c12429

sipify

fdb2874

elpaso force-pushed the authmanager-storage-api branch from 7241a0e to fdb2874 Compare September 12, 2024 07:16

elpaso merged commit 987c38e into qgis:master Sep 12, 2024
24 checks passed

velle pushed a commit to velle/QGIS that referenced this pull request Sep 17, 2024

[auth][api] QgsAuthConfigurationStorage classes and tests (qgis#57992)

0f1533e

Implementation of QEP Authentication System: allow Database storage for authentication DB qgis/QGIS-Enhancement-Proposals#248

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[auth][api] QgsAuthConfigurationStorage classes and tests #57992

[auth][api] QgsAuthConfigurationStorage classes and tests #57992

elpaso commented Jul 5, 2024 •

edited

Loading

nyalldawson Jul 9, 2024

nyalldawson Jul 9, 2024

elpaso Jul 9, 2024

github-actions bot commented Jul 9, 2024 •

edited

Loading

nyalldawson Jul 29, 2024

elpaso Jul 29, 2024

elpaso commented Aug 13, 2024

elpaso commented Aug 13, 2024

3nids commented Aug 13, 2024

nyalldawson commented Aug 13, 2024

nyalldawson commented Aug 13, 2024

elpaso commented Aug 23, 2024

nyalldawson commented Aug 26, 2024

elpaso commented Sep 1, 2024

jef-n commented Sep 13, 2024

elpaso commented Sep 13, 2024

	class CORE_EXPORT QgsAuthConfigurationStorage: public QObject SIP_ABSTRACT
	class CORE_EXPORT QgsAbstractAuthConfigurationStorage: public QObject SIP_ABSTRACT

[auth][api] QgsAuthConfigurationStorage classes and tests #57992

[auth][api] QgsAuthConfigurationStorage classes and tests #57992

Conversation

elpaso commented Jul 5, 2024 • edited Loading

Abstract authentication credentials storage

Donors

Goal

Additional features:

nyalldawson Jul 9, 2024

Choose a reason for hiding this comment

nyalldawson Jul 9, 2024

Choose a reason for hiding this comment

elpaso Jul 9, 2024

Choose a reason for hiding this comment

github-actions bot commented Jul 9, 2024 • edited Loading

🪟 Windows builds ready!

nyalldawson Jul 29, 2024

Choose a reason for hiding this comment

elpaso Jul 29, 2024

Choose a reason for hiding this comment

elpaso commented Aug 13, 2024

elpaso commented Aug 13, 2024

3nids commented Aug 13, 2024

nyalldawson commented Aug 13, 2024

nyalldawson commented Aug 13, 2024

elpaso commented Aug 23, 2024

nyalldawson commented Aug 26, 2024

elpaso commented Sep 1, 2024

jef-n commented Sep 13, 2024

elpaso commented Sep 13, 2024

elpaso commented Jul 5, 2024 •

edited

Loading

github-actions bot commented Jul 9, 2024 •

edited

Loading