Change API to be able to assign roles to namespaces #607

nkaretnikov · 2023-10-03T13:09:55Z

Fixes #491.

netlify · 2023-10-03T13:09:59Z

✅ Deploy Preview for kaleidoscopic-dango-0cf31d canceled.

Name	Link
🔨 Latest commit	`70c20f3`
🔍 Latest deploy log	https://app.netlify.com/sites/kaleidoscopic-dango-0cf31d/deploys/65685d3a6572db00080f4891

conda-store-server/conda_store_server/api.py

tests/test_api.py

conda-store-server/conda_store_server/orm.py

conda-store-server/conda_store_server/server/auth.py

...-server/conda_store_server/alembic/versions/46bdf428642d_change_namespacerolemapping_and_.py

conda-store-server/tests/test_db_api.py

conda-store-server/conda_store_server/api.py

conda-store-server/conda_store_server/schema.py

conda-store-server/conda_store_server/server/auth.py

Otherwise, nothing is printed when `role_mappings_version` is set incorrectly. This ``` c.RBACAuthorizationBackend.role_mappings_version = 42 ``` now prints ``` c.RBACAuthorizationBackend.role_mappings_version: invalid role mappings version: 42, expected: (1, 2) ```

nkaretnikov · 2023-12-01T15:18:07Z

Migrating data from v1 to v2 role mappings will be done in a separate PR because it can cause issues during migration, which might require manual intervention. So a standalone function will be provided for those who need it. #681

nkaretnikov · 2023-12-03T15:38:32Z

conda-store-server/conda_store_server/server/app.py

+        @app.exception_handler(Exception)
+        async def exception_handler(request, exc):
+            print(exc)
+            return await http_exception_handler(request, exc)


Added this to print an exception to the terminal when an invalid role_mappings_version value is used. Previously, nothing was printed except "Invalid server error".

nkaretnikov · 2023-12-03T15:50:19Z

conda-store-server/conda_store_server/server/auth.py

+            for row in raw_role_mappings:
+                db_role_mappings[row["entity"]].add(row["role"])
+
+        return db_role_mappings


This code used to be in database_role_bindings. I moved it to the start of the class to use it in _role_mappings_versions.

nkaretnikov · 2023-12-03T18:56:54Z

conda-store-server/tests/test_auth.py

@@ -140,36 +141,191 @@ def test_authorization(conda_store, entity_bindings, arn, permissions, authorize
    assert authorized == authorization.authorize(entity, arn, permissions)


-def test_end_to_end_auth_flow(conda_store):
+@pytest.mark.parametrize("role_mappings_version", [1, 2])


Updated this test to match the newly added test_end_to_end_auth_flow_v2. The v1 test calls HTTP APIs that use the v1 role mappings table. The v2 test does the same for the v2 table. Both tests check that auth works after inserting roles into the corresponding DB table.

nkaretnikov · 2023-12-03T19:19:55Z

conda-store-server/tests/test_auth.py

+    response.raise_for_status()
+
+    # Should fail again
+    assert authorize() is False


Note that in the v2 table the relationship between namespaces is reversed compared to v1. That's why this performs API requests to other_namespace and sets other_namespace to namespace in post. This is intended.

The v1 table says: "I give myself access to this namespace"
The v2 table says: "I allow this namespace access"

nkaretnikov · 2023-12-03T19:26:00Z

conda-store-server/tests/test_auth.py

+            Permissions.SETTING_READ,
+            Permissions.SETTING_UPDATE,
+        },
+    )


Nit: the above block should be indented.

nkaretnikov · 2023-12-03T21:47:03Z

I've reviewed the code, not planning to make any changes. Ready for review.

nkaretnikov · 2023-12-03T22:03:51Z

For reviewers: this is a huge PR, I suggest you start with conda-store-server/tests/test_auth.py and docs/administration.md. The doc explains how this is supposed to be used. The auth test provides end-to-end examples for v1 and v2 and shows how this affects authorization.

dcmcand

The comments in this review are mostly stylistic and are not blocking for approval.

...-server/conda_store_server/alembic/versions/46bdf428642d_change_namespacerolemapping_and_.py

conda-store-server/conda_store_server/alembic/versions/771180018e1b_add_v2_role_mappings.py

dcmcand · 2023-12-04T21:01:21Z

conda-store-server/conda_store_server/api.py

+    if namespace is None:
+        raise ValueError(f"Namespace='{name}' not found")
+
+    nrm = aliased(orm.NamespaceRoleMappingV2)


Can we please have more descriptive variable names here? Readability is part of maintainability.

In general, I agree. But not here. I deliberately use short names so that queries don't get split up over multiple lines by the linter, which would be hard to read. All these DB functions are very small, so there's no concern of this clashing with something else causing confusion. I couldn't think of a short name for NamespaceRoleMapping other than using the first letters.

conda-store-server/conda_store_server/api.py

dcmcand · 2023-12-04T21:12:17Z

conda-store-server/conda_store_server/orm.py

+    namespace = relationship(Namespace, foreign_keys=[namespace_id])
+
+    # ... for other namespace
+    other_namespace_id = Column(Integer, ForeignKey("namespace.id"), nullable=False)


As mentioned above, these names don't make it clear which namespace has permissions to which.

Addressed in another comment above.

conda-store-server/conda_store_server/server/auth.py

dcmcand · 2023-12-05T14:02:23Z

One thought that occurred to me is that introducing the version to our routes actually is breaking if anyone has previously had naked routes. I think adding a router for naked routes that redirects to v1 of the route and documenting this would handle that issue. This could be a seperate issue/pr if that is simpler, but without it this will be a breaking change.

nkaretnikov · 2023-12-05T14:04:46Z

One thought that occurred to me is that introducing the version to our routes actually is breaking if anyone has previously had naked routes.

Did you mean to reply to the router_ui thread? This PR doesn't make changes to existing routes.

dcmcand · 2023-12-05T14:25:19Z

One thought that occurred to me is that introducing the version to our routes actually is breaking if anyone has previously had naked routes.

Did you mean to reply to the router_ui thread? This PR doesn't make changes to existing routes.

No, i meant in regards to the api. I thought that we had discussed making the default v1, but if this doesn't touch existing routes then you can disregard this comment.

nkaretnikov · 2023-12-05T14:27:25Z

Yeah, no action is needed here. I'll mention this for whoever is reading these later. The API I'm using is already versioned:

router_api = APIRouter(
    tags=["api"],
    prefix="/api/v1",
)

costrouc · 2023-12-05T16:57:35Z

conda-store-server/tests/test_db_api.py

@@ -71,6 +71,108 @@ def test_namespace_role_mapping(db):
    NamespaceRoleMapping(namespace=namespace, namespace_id=namespace.id, entity="*/*")


+def test_namespace_role_mapping_v2(db):


Like the detailed tests on this feature

dcmcand · 2023-12-05T18:15:00Z

conda-store-server/tests/test_auth.py

-            },
+            primary_namespace=namespace,
+            # No default roles
+            role_bindings={},
        )
    )

    token_model = authentication.authenticate(token)

    authorization = RBACAuthorizationBackend(


Pulling all this auth stuff that is setup for the auth tests into a helper function would help readability of these tests imo. This won't block approval, but would help maintainability I think.

dcmcand · 2023-12-05T18:17:28Z

conda-store-server/tests/test_db_api.py

+    other_namespace_name3 = "pytest-other-namespace3"
+
+    # Starts with no namespaces
+    assert len(api.list_namespaces(db).all()) == 0


It might be simpler to just clear out the namespaces like you do in the auth tests than to assert here.

dcmcand · 2023-12-05T18:22:38Z

conda-store-server/tests/test_db_api.py

+    assert len(roles) == 1
+    roles = api.get_other_namespace_roles(db, namespace_name)
+    db.commit()
+    assert len(roles) == 0


In general, relying on having a pure db and checking lengths has led to flaky tests for me in the past, especially if you ever try to run tests in parallel. I always prefer to check for concrete records than for lengths.

This comment was marked as outdated.

Sign in to view

nkaretnikov commented Oct 4, 2023

View reviewed changes

conda-store-server/conda_store_server/api.py Outdated Show resolved Hide resolved

nkaretnikov force-pushed the role-mappings-api-491 branch from 2c3ba98 to 9df4c35 Compare October 9, 2023 00:14

nkaretnikov commented Oct 9, 2023

View reviewed changes

tests/test_api.py Outdated Show resolved Hide resolved

nkaretnikov force-pushed the role-mappings-api-491 branch from 9df4c35 to 6a2f985 Compare October 9, 2023 00:42

nkaretnikov commented Oct 9, 2023

View reviewed changes

conda-store-server/conda_store_server/orm.py Outdated Show resolved Hide resolved

nkaretnikov commented Oct 9, 2023

View reviewed changes

conda-store-server/conda_store_server/server/auth.py Outdated Show resolved Hide resolved

nkaretnikov commented Oct 9, 2023

View reviewed changes

...-server/conda_store_server/alembic/versions/46bdf428642d_change_namespacerolemapping_and_.py Outdated Show resolved Hide resolved

nkaretnikov force-pushed the role-mappings-api-491 branch from 6a2f985 to e96a709 Compare October 15, 2023 19:44

nkaretnikov commented Oct 15, 2023

View reviewed changes

...-server/conda_store_server/alembic/versions/46bdf428642d_change_namespacerolemapping_and_.py Outdated Show resolved Hide resolved

nkaretnikov force-pushed the role-mappings-api-491 branch 3 times, most recently from 67c271e to 0b19764 Compare October 16, 2023 18:06

nkaretnikov commented Oct 16, 2023

View reviewed changes

conda-store-server/tests/test_db_api.py Outdated Show resolved Hide resolved

nkaretnikov commented Oct 16, 2023

View reviewed changes

conda-store-server/tests/test_db_api.py Outdated Show resolved Hide resolved

nkaretnikov force-pushed the role-mappings-api-491 branch 2 times, most recently from 552dda1 to bb3b131 Compare October 17, 2023 12:43

trallard added the status: in progress 🏗 label Oct 17, 2023

nkaretnikov force-pushed the role-mappings-api-491 branch from 6fa9c98 to cee8495 Compare October 18, 2023 09:48

nkaretnikov mentioned this pull request Oct 18, 2023

[BUG] - Flaky CI tests: libmamba.so.2: undefined symbol: solver_ruleinfo2str, version SOLV_1.0 #632

Closed

nkaretnikov force-pushed the role-mappings-api-491 branch 3 times, most recently from ed7384c to 9ac936b Compare October 19, 2023 15:27