Add SQL authenticator #508

janjagusch · 2022-03-23T11:36:29Z

I've written a basic authenticator, similar to the dict authenticator, that stores credentials as sha256-hashed values in a SQL database. It also ships with CLI tools for CRUD operations on the credentials table (like creating users or updating passwords).

It's built around sqlmodel, so it should work with any kind of SQL backend.

I don't know how you feel about adding more authenticator plugins, so if you feel like I should rather put this in a separate repository, feel free to close this PR.

plugins/quetz_sql_authenticator/README.md

wolfv · 2022-03-23T11:43:01Z

Hey, wow, that's cool!

How do you feel about using passlib (https://passlib.readthedocs.io/) instead of plain sha256?

baszalmstra · 2022-03-23T11:51:08Z

Would it make sense to also add a password salt for extra security?

wolfv · 2022-03-23T11:53:18Z

yeah I was hoping that passlib would do all that :)

janjagusch · 2022-03-23T11:55:33Z

Hey, wow, that's cool!

How do you feel about using passlib (https://passlib.readthedocs.io/) instead of plain sha256?

Glad you like it. 🙂

Thanks for pointing me to passlib, I'll use that instead.

janjagusch · 2022-03-23T12:23:29Z

yeah I was hoping that passlib would do all that :)

Yes, I think so too: https://passlib.readthedocs.io/en/stable/#welcome

janjagusch · 2022-03-23T12:27:24Z

@wolfv, I've added passlib.

do you have a preferred way of testing this? the way I see it is that we'd need a SQL server running in a separate container. alternatively, we could test with sqlite, which might make things easier.

wolfv · 2022-03-23T12:45:56Z

Hey, I didn't check deeply before, but I am just wondering now if you want to use a different database on purpose?

Because you can also create a new table for your plugin and use that table as part of the Quetz SQL database for the storage of the authentication information.

E.g. here is another plugin creating a table: https://github.com/mamba-org/quetz/blob/main/plugins/quetz_conda_suggest/quetz_conda_suggest/db_models.py

wolfv · 2022-03-23T12:46:29Z

You would also have to define a migration but that can be auto-generated: https://github.com/mamba-org/quetz/tree/main/plugins/quetz_conda_suggest/quetz_conda_suggest/migrations/versions

janjagusch · 2022-03-23T13:15:20Z

Hey, I didn't check deeply before, but I am just wondering now if you want to use a different database on purpose?

That's a good question. I was also thinking about using the Quetz database instead but then thought it might be useful to have this decoupled, as you might have central user management that you want to reuse here. However, I don't have a strong opinion on this (in our use case we could easily use the same DB) and hence wouldn't oppose changing this.

E.g. here is another plugin creating a table: https://github.com/mamba-org/quetz/blob/main/plugins/quetz_conda_suggest/quetz_conda_suggest/db_models.py

Nice, thanks. I'll check this out later.

plugins/quetz_sql_authenticator/quetz_sql_authenticator/__init__.py

plugins/quetz_sql_authenticator/quetz_sql_authenticator/cli.py

jonashaag · 2022-03-25T15:07:47Z

plugins/quetz_sql_authenticator/quetz_sql_authenticator/cli.py

+        credentials = list(session.exec(statement))
+    click.echo(f"WARNING: Resetting the table will delete {len(credentials)} users.")
+    while (
+        reset_database := input("Are you sure you want to reset the table? [Y/n]")


There is click.confirm IIRC

Or click.prompt?

nice, thanks for the pointer.

janjagusch · 2022-04-05T16:18:47Z

Sorry, this PR has gotten a bit stale. I will try to get back to it the week after next week.

janjagusch · 2022-04-23T08:55:10Z

You would also have to define a migration but that can be auto-generated: https://github.com/mamba-org/quetz/tree/main/plugins/quetz_conda_suggest/quetz_conda_suggest/migrations/versions

hi @wolfv! the credentials table for the SQL authenticator is now part of the quetz database and written in sqlalchemy instead of sqlmodel. I'm not entirely sure how to create the migration with alembic, though (never used it before). I guess what's confusing me the most is that we have the main migrations directory under quetz/migrations but then quetz_conda_suggest has its own migrations directory, which doesn't contain an env.py or a script.py.mako.

Could you provide me with some more instructions on how to make the migration work, please? Once I have that part working, I'd start building some basic tests.

wolfv · 2022-10-10T12:54:04Z

Sorry for ignoring the questions regarding the database migrations. I believe the quetz command line utility has some helper functions to generate initial migrations from the sqlalchemy schema (even for plugins):

(base) ➜  quetz git:(main) ✗ quetz make-migrations --help

 Usage: quetz make-migrations [OPTIONS] [PATH]

 make database migrations for quetz or a plugin

╭─ Arguments ──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│   path      [PATH]  The path of the deployment [default: None]                                                                                                                                           │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
╭─ Options ────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ --message                          TEXT  revision message [default: None]                                                                                                                                │
│ --plugin                           TEXT  head or heads or plugin name [default: quetz]                                                                                                                   │
│ --initialize    --no-initialize          initialize migrations [default: no-initialize]                                                                                                                  │
│ --help                                   Show this message and exit.                                                                                                                                     │
╰──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯

This should populate the right files that are then used by quetz init-db to execute the schema migrations.

simonbohnen · 2022-10-10T12:58:56Z

Hey Wolf! Thanks for the hint, that looks exactly right!

I've been talking to @janjagusch and will work on finishing up this PR.

wolfv · 2022-10-10T13:08:03Z

great!

…quetz into add-sqlalchemy-authenticator

plugins/quetz_sql_authenticator/README.md

plugins/quetz_sql_authenticator/quetz_sql_authenticator/api.py

janjagusch · 2022-10-25T12:30:06Z

quetz/authentication/base.py

@@ -109,7 +109,7 @@ async def authorize(
        user_dict = await self._authenticate(request, dao, config)

        if user_dict is None:
-            return Response("login failed")
+            return Response("login failed", status_code=401)


@wolfv, this change affects all authenticators, I believe. I think previously, it would always return 200, even when the login failed. This is not strictly necessary in this PR, so if you want to can remove it/discuss it in a separate PR.

I'd suggest leaving this out for now and opening a separate PR for it. Will probably make it easier to get this PR merged.

…quetz into add-sqlalchemy-authenticator

…janjagusch/quetz into add-sqlalchemy-authenticator

plugins/quetz_sql_authenticator/README.md

plugins/quetz_sql_authenticator/tests/test_quetz_sql_authenticator.py

janjagusch

awesome job, @simonbohnen!

…quetz into add-sqlalchemy-authenticator

simonbohnen · 2022-10-25T14:41:55Z

I'm unable to resolve the conversations, but that shouldn't matter I guess. @wolfv do you have any feedback on the current state? Jan's comment might be relevant.

janjagusch

I'm the official PR author, but it was mostly @simonbohnen's work.
Hence, I will give this an unofficial approval and proceed to merge.

Awesome work, @simonbohnen!

add SQL authenticator

4405916

janjagusch commented Mar 23, 2022

View reviewed changes

plugins/quetz_sql_authenticator/README.md Outdated Show resolved Hide resolved

Update plugins/quetz_sql_authenticator/README.md

ffb8109

janjagusch added 3 commits March 23, 2022 13:04

lint

d12b04a

don't hash username

5f96309

use passlib instead of hashlib

bc6b071

jonashaag reviewed Mar 25, 2022

View reviewed changes

janjagusch added 2 commits March 28, 2022 15:42

return when username can't be found

fb22bf7

raise hard when error occurs

b5ea425

add credentials table to quetz db

132940a

wolfv added the enhancement New feature or request label Apr 23, 2022

rename utils to db_models

3283f93

simonbohnen added 2 commits October 10, 2022 09:29

start replacing sqlmodel with sqlalchemy

55a09f3

Stop using sqlmodel

240bdbc

simonbohnen added 2 commits October 10, 2022 15:06

add migrations to setup.py

81e706d

create alembic migration

002cceb

simonbohnen and others added 3 commits October 25, 2022 12:00

Clean up debugging

6a1ae42

Merge branch 'add-sqlalchemy-authenticator' of github.com:janjagusch/…

148e09e

…quetz into add-sqlalchemy-authenticator

Trigger CI.

1e609c2

janjagusch commented Oct 25, 2022

View reviewed changes

plugins/quetz_sql_authenticator/README.md Outdated Show resolved Hide resolved

janjagusch commented Oct 25, 2022

View reviewed changes

plugins/quetz_sql_authenticator/quetz_sql_authenticator/api.py Outdated Show resolved Hide resolved

janjagusch commented Oct 25, 2022

View reviewed changes

plugins/quetz_sql_authenticator/quetz_sql_authenticator/api.py Outdated Show resolved Hide resolved

janjagusch commented Oct 25, 2022

View reviewed changes

plugins/quetz_sql_authenticator/quetz_sql_authenticator/api.py Outdated Show resolved Hide resolved

janjagusch commented Oct 25, 2022

View reviewed changes

plugins/quetz_sql_authenticator/quetz_sql_authenticator/api.py Outdated Show resolved Hide resolved

janjagusch commented Oct 25, 2022

View reviewed changes

simonbohnen and others added 7 commits October 25, 2022 12:55

API tests

ed1e444

Merge branch 'add-sqlalchemy-authenticator' of github.com:janjagusch/…

cc14a87

…quetz into add-sqlalchemy-authenticator

Remove CLI

3ec92d2

Update README

4fb4577

type hints

9ecf3b9

Trigger CI.

d12fe39

Merge branch 'add-sqlalchemy-authenticator' of github.com-janjagusch:…

0c3a0c3

…janjagusch/quetz into add-sqlalchemy-authenticator

janjagusch commented Oct 25, 2022

View reviewed changes

plugins/quetz_sql_authenticator/README.md Outdated Show resolved Hide resolved

janjagusch commented Oct 25, 2022

View reviewed changes

plugins/quetz_sql_authenticator/README.md Show resolved Hide resolved

janjagusch commented Oct 25, 2022

View reviewed changes

plugins/quetz_sql_authenticator/tests/test_quetz_sql_authenticator.py Outdated Show resolved Hide resolved

janjagusch commented Oct 25, 2022

View reviewed changes

plugins/quetz_sql_authenticator/tests/test_quetz_sql_authenticator.py Outdated Show resolved Hide resolved

janjagusch commented Oct 25, 2022

View reviewed changes

plugins/quetz_sql_authenticator/tests/test_quetz_sql_authenticator.py Outdated Show resolved Hide resolved

janjagusch commented Oct 25, 2022

View reviewed changes

simonbohnen added 4 commits October 25, 2022 13:34

remove reset route and add get_all route

3879f1e

Merge branch 'add-sqlalchemy-authenticator' of github.com:janjagusch/…

4b8d3a3

…quetz into add-sqlalchemy-authenticator

Add tests for unauthorized case

2a452bc

Add new alembic migration

022fdbe

Revert 401 auth status

b5ad2b1

janjagusch commented Oct 26, 2022

View reviewed changes

janjagusch merged commit 9399645 into mamba-org:main Oct 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SQL authenticator #508

Add SQL authenticator #508

janjagusch commented Mar 23, 2022

wolfv commented Mar 23, 2022

baszalmstra commented Mar 23, 2022

wolfv commented Mar 23, 2022

janjagusch commented Mar 23, 2022

janjagusch commented Mar 23, 2022

janjagusch commented Mar 23, 2022

wolfv commented Mar 23, 2022

wolfv commented Mar 23, 2022

janjagusch commented Mar 23, 2022

jonashaag Mar 25, 2022

jonashaag Mar 25, 2022

janjagusch Mar 25, 2022

janjagusch commented Apr 5, 2022

janjagusch commented Apr 23, 2022

wolfv commented Oct 10, 2022

simonbohnen commented Oct 10, 2022

wolfv commented Oct 10, 2022

janjagusch Oct 25, 2022

janjagusch Oct 25, 2022 •

edited

Loading

janjagusch left a comment

simonbohnen commented Oct 25, 2022

janjagusch left a comment

Add SQL authenticator #508

Add SQL authenticator #508

Conversation

janjagusch commented Mar 23, 2022

wolfv commented Mar 23, 2022

baszalmstra commented Mar 23, 2022

wolfv commented Mar 23, 2022

janjagusch commented Mar 23, 2022

janjagusch commented Mar 23, 2022

janjagusch commented Mar 23, 2022

wolfv commented Mar 23, 2022

wolfv commented Mar 23, 2022

janjagusch commented Mar 23, 2022

jonashaag Mar 25, 2022

Choose a reason for hiding this comment

jonashaag Mar 25, 2022

Choose a reason for hiding this comment

janjagusch Mar 25, 2022

Choose a reason for hiding this comment

janjagusch commented Apr 5, 2022

janjagusch commented Apr 23, 2022

wolfv commented Oct 10, 2022

simonbohnen commented Oct 10, 2022

wolfv commented Oct 10, 2022

janjagusch Oct 25, 2022

Choose a reason for hiding this comment

janjagusch Oct 25, 2022 • edited Loading

Choose a reason for hiding this comment

janjagusch left a comment

Choose a reason for hiding this comment

simonbohnen commented Oct 25, 2022

janjagusch left a comment

Choose a reason for hiding this comment

janjagusch Oct 25, 2022 •

edited

Loading