search

IdentityPython / JWTConnect-Python-OidcMsg

Implementation of OIDC protocol messages
Apache License 2.0
3 stars 10 forks source link

Get AbstractStorageSQLAlchemy storage working #21

Closed davidkhess closed 3 years ago

davidkhess commented 4 years ago

This is a patch that gets AbstractStorageSQLAlchemy working. Note, it probably has some issues that need resolving another way but that is beyond my pay grade because I don't know this library well enough. But I think this is a pretty good cleanup and suits my needs.

Some of the (opinionated) changes:

1) Got rid of the behavior where it automatically always creates the table it was referring to via a call to create all tables. That should be under the control of the surrounding application not a library.

2) Got rid of the import of the sample Table as it was polluting the surrounding application's metadata. This should not be done by a library and is better managed by the application. Note, there could be more cleanup here (like optionally passing in an existing engine instead of creating one) but I wasn't sure how to arrange that.

3) General cleanup and simplification. There were clearly assumptions changed with how the calling code uses a Storage and this code needed fixes for that.

4) Introduced a hack that resolved an issue using this storage back end. When enabled, it interfered with the key jar storage mechanism. The default used for that changes when a db_conf without a default is supplied and it breaks. So, the PlainDict at the top of this PR is a hack around it. See the configuration below which is working for my OP:

            "db_conf": {
                "state": {
                    "handler": "oidcmsg.storage.absqlalchemy.AbstractStorageSQLAlchemy",
                    "url": "postgresql:///application",
                    "params": {
                        "table": "oidc_storage"
                    }
                },
                "default": {
                    "handler": "oidcmsg.storage.absqlalchemy.PlainDict"
                }
            }
rohe commented 3 years ago

Seems like Travis fails because of an error in tox.ini which I think we fixed a while ago. Apart from that I have no problem with this PR.

peppelinux commented 3 years ago

It seems isort validation @davidkhess please fix It, I'll merge It asap.

See Travis log, at the end, it's quite explicit about the real problem