Do not allow duplicate role memberships#3815
Conversation
This was an oversight and should have always called `super().setUp()` instead of `super().setUpClass()`. This didn’t fail when running tests for the entire file (`pytest test_sessions_api.py`), but it did always fail when running individual tests (`pytest test_sessions_api.py -k oauth_callback`).
This likely isn't the root cause for #3811, but it will help with debugging in the future.
While this doesn't fix the root cause for #3811, it ensures that this error doesn't result in inconsistent data that have previously permanently blocked users from sign-in. Having sensible constraints on tables is a good idea anyway.
stchris
left a comment
stchris
left a comment
There was a problem hiding this comment.
I'm ok with this implementation, but left a few side questions.
| """Adds an existing role as a membership of a group.""" | ||
| self.roles.append(role) | ||
| db.session.add(role) | ||
| if role not in self.roles: |
There was a problem hiding this comment.
I wonder if catching https://docs.sqlalchemy.org/en/20/core/exceptions.html#sqlalchemy.exc.IntegrityError here and reacting on it (log.warn) would be more efficient than what I assume is a SELECT followed by an INSERT?
There was a problem hiding this comment.
This operates on the ORM object only, i.e., no requests are made at this time. Only after flushing the SQLAlchemy session and comitting the transaction statements to update the role memberships would be executed (and any errors would be raised only then).
There was a problem hiding this comment.
The primary goal of this log message was to confirm that there is no logic flaw that can insert duplicate role memberships within a single request. I’m pretty sure there is no such flaw (instead the issue is a race condition with parallel requests) and thus we should never see this log message in practice. But I’m not 100% sure.
|
|
||
| @contextmanager | ||
| def mock_oauth_token_exchange(name: str, email: str): | ||
| def mock_oauth_token_exchange(name: str, email: str, groups: List[str] = []): |
There was a problem hiding this comment.
High praise for adding type hints 🏅
| assert query["status"] == ["error"] | ||
| assert query["code"] == ["403"] | ||
|
|
||
| def test_oauth_callback_sync_groups(self): |
There was a problem hiding this comment.
The test case looks good, thanks for making it so straight-forward!
|
I suggest we keep this PR open and merge it when we are ready to release/deploy the changes (after a stable 4.0 release), to ensure we do not forget about the breaking changes/manual checks required. |
2 participants
Aleph users can be part of user groups. Users and groups are both modeled as "roles" in Aleph. To add a user to a group, a row is inserted into the
role_membershiptable.We’ve occasionally had issues with duplicate rows in this table, likely due to a race condition when Aleph handles parallel OAuth requests for the same user (#3811 (comment)), but we haven’t been able to reproduce or pinpoint the exact root cause.
The effect of a duplicate role membership is that users are permanently blocked from authenticating until the duplicates are removed.
This PR adds a primary key constraint to the
role_membershiptable. While this does not address the root cause of the problem, it prevents duplicates from being inserted into the table. In practice, this means that in rare cases, OAuth requests may fail when the constraint is violated, but users can simply retry the authentication in that case.In addition, I’ve implemented two related changes:
Important
This PR contains a migration that adds a primary key constraint to an existing table. The migration will fail if the existing table violates the constraint. It is necessary to manually check and remove whether duplicates exist in this table before applying the migration. See #3811 (comment) for a query that returns duplicates.