Feature/ogc 3157 current user none crash by Tschuppi81 · Pull Request #2471 · OneGov/onegov-cloud

Tschuppi81 · 2026-05-08T14:06:16Z

Org: Replace assert current_user is not None with a Sentry warning and HTTPForbidden

Adds OrgRequest.get_current_user() which captures a Sentry warning and raises HTTPForbidden when current_user is None despite a valid session. Replaces all bare asserts across feriennet, org, town6 and translator_directory.

Sentry issue: https://seantis-gmbh.sentry.io/issues/7467148569/

TYPE: Feature
LINK: ogc-3157

…er() Adds OrgRequest.get_current_user() which captures a Sentry warning and raises HTTPForbidden when current_user is None despite a valid Redis session. Replaces all bare asserts and inline None checks across feriennet, org, town6 and translator_directory. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Tests that get_current_user() returns the user when present, and raises HTTPForbidden while sending a Sentry warning when current_user is None. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

linear · 2026-05-08T14:06:20Z

OGC-3157

Copilot

Pull request overview

This PR aims to prevent crashes when request.current_user is unexpectedly None by introducing a safer accessor on OrgRequest and replacing a number of assert request.current_user is not None call sites with either request.get_current_user() or guard clauses.

Changes:

Added OrgRequest.get_current_user() which raises HTTPForbidden and attempts to emit a Sentry warning when current_user is unexpectedly missing.
Replaced multiple assert request.current_user is not None usages with request.get_current_user() across org/feriennet/translator_directory views and helpers.
Added unit tests for the new get_current_user() behavior.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
tests/onegov/org/test_request.py	Adds tests for `OrgRequest.get_current_user()` success and failure behavior.
src/onegov/org/request.py	Introduces `get_current_user()` and adds Sentry logging + `HTTPForbidden` on missing user.
src/onegov/org/custom.py	Avoids crashing in global tools ticket section when `current_user` is `None`.
src/onegov/org/utils.py	Switches second-factor lookup to use `get_current_user()` in `can_change_username`.
src/onegov/org/views/ticket.py	Uses `get_current_user()` when building “My” ticket filter.
src/onegov/org/views/resource.py	Uses `get_current_user()` when closing reservation tickets.
src/onegov/org/views/form_registration_window.py	Uses `get_current_user()` when closing tickets during submission cancellation.
src/onegov/org/views/form_definition.py	Uses `get_current_user()` when closing tickets related to submissions.
src/onegov/translator_directory/views/time_report.py	Uses `get_current_user()` when accepting time reports and closing tickets.
src/onegov/translator_directory/generate_docx.py	Uses `get_current_user()` to derive signature lookup criteria.
src/onegov/translator_directory/custom.py	Adds guard to avoid `current_user`-None crash in tickets tools.
src/onegov/town6/custom.py	Adds guard to avoid `current_user`-None crash in chat staff tools.
src/onegov/feriennet/views/occasion.py	Uses `get_current_user()` when recording TOS acceptance.
src/onegov/feriennet/views/invoice.py	Uses `get_current_user()` for invoice/donation flows dependent on the logged-in user.
src/onegov/feriennet/views/booking.py	Uses `get_current_user()` for “my bookings” when not admin.
src/onegov/feriennet/forms/attendee.py	Uses `get_current_user()` for TOS field handling based on user data.
src/onegov/feriennet/custom.py	Adds guard to avoid `current_user`-None crash in personal tools.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

codecov · 2026-05-08T14:12:55Z

Codecov Report

❌ Patch coverage is 93.54839% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.54%. Comparing base (ade1a79) to head (26957b6).
⚠️ Report is 13 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
src/onegov/translator_directory/generate_docx.py	75.00%	1 Missing ⚠️
src/onegov/user/auth/provider.py	0.00%	1 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
src/onegov/core/security/identity_policy.py	`100.00% <100.00%> (ø)`
src/onegov/org/forms/user.py	`89.26% <100.00%> (-0.06%)`	⬇️
src/onegov/org/request.py	`92.54% <100.00%> (+1.09%)`	⬆️
src/onegov/user/auth/core.py	`91.25% <100.00%> (-0.84%)`	⬇️
src/onegov/user/cli.py	`86.09% <100.00%> (-0.06%)`	⬇️
src/onegov/user/models/user.py	`99.29% <100.00%> (+0.01%)`	⬆️
src/onegov/translator_directory/generate_docx.py	`92.00% <75.00%> (-1.16%)`	⬇️
src/onegov/user/auth/provider.py	`50.32% <0.00%> (ø)`

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ade1a79...26957b6. Read the comment docs.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Embed the username directly in the message string instead of using the non-existent extras= parameter. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…up fails Anonymous requests should raise HTTPForbidden silently; the warning is only meaningful when a valid session identity exists but current_user is unexpectedly None. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…ogin links Replace early return guard with an inline condition so only the tickets block is skipped when current_user is None, not the rest of the generator. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Rename get_current_user() result to current_user to distinguish the requester from the user argument being inspected. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Tschuppi81 · 2026-05-08T14:25:37Z

@Daverball Is this a good approach to prevent assertion errors at runtime but try to identify the root cause of current_user beeing None?

Copilot

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.

…user NO_IDENTITY is not None, so the previous guard would trigger a spurious Sentry warning for unauthenticated requests. Use is_logged_in instead, which correctly checks identity is not NO_IDENTITY. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

MagicMock auto-creates truthy attributes, so setting identity=None had no effect on the is_logged_in check. Explicitly set is_logged_in=True/False to reflect the actual condition the code under test checks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Replacing the silent early return with get_current_user() ensures a stale-session case (is_logged_in True but current_user None) is reported to Sentry and handled consistently with the rest of the codebase. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…sions Same fix as town6: replace the silent early return with get_current_user() so a stale-session case is reported to Sentry and handled consistently. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Replace the silent current_user is not None guard with get_current_user() so stale-session inconsistencies are reported to Sentry consistently. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.

The method raises HTTPForbidden for any None current_user, not just the stale-session case. Clarify that anonymous requests are also forbidden, and that Sentry is only notified for the logged-in-but-missing-user case. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

…ning current_username is an email address (PII) and was sent unconditionally, bypassing the should_send_default_pii() gate used elsewhere in the Sentry integration. Switch to identity.uid which is the non-PII Sentry user ID already attached to every event via the event processor. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Asserts can be optimised away with -O and split(' ') would crash on names with more than one space. Use an explicit guard and split(' ', 1) with a length check instead, returning None for missing or malformed names. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Replace the silent current_user None guard in translator_directory/custom.py with get_current_user(), consistent with the other nav fixes. Also add get_current_user() to AgencyApp's DummyRequest test fixture. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

strip() removes leading/trailing whitespace, split(None, 1) handles multiple spaces between first and last name, and the non-empty check on both parts prevents overly broad LIKE '%%' queries. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Daverball · 2026-05-08T16:45:51Z

The root cause of this exception is most likely that changing a translator's e-mail address does not log them out. So they're still logged in with their old e-mail, which no longer has an associated account, so get_current_user will return None.

So I don't think we should make any large sweeping changes to make the application not crash, when this happens. Since after all it is a real problem if we didn't log that user out.

It might be worth adding a change_username method to User which calls self.logout_all_sessions() and then switch to using that instead of manually updating User.username, everywhere it is relevant, so we can't forget to do this step.

Although we probably could change current_user to use the primary key instead of the username in order to retrieve the User object, since we now store both in the identity object and the primary key will never change. But we would still need to detect when the username in the identity no longer matches the username of the User and update the cached username in the session, since otherwise there may still be some odd behavior for code that relies on request.current_username.

Tschuppi81 · 2026-05-22T14:09:23Z

The root cause of this exception is most likely that changing a translator's e-mail address does not log them out

Is that related to LDAP?

Daverball · 2026-05-22T14:31:19Z

Is that related to LDAP?

It shouldn't be an issue during external login, although I'm not sure what happens if they have multiple sessions and their LDAP username gets changed during external login, I'm not sure if we remembered to terminate all the other sessions, so only the one with the new name remains.

But with translators it's also possible for admins to change the name of the translator, which gets mirrored back to the user if I'm not mistaken and we could be forgetting to log the user out there as well. There's also the LDAP synchronization cronjob, although I'm not sure if we have that for translator directory or if it's only running for FSI.

Either way there are quite a few ways the username can change, and we probably didn't remember to log out the active session for some of the ways. That's why adding a change_username method makes sense to me, so it's not as easy to forget. Either way you will have to look at all of the places in the code that update the username and check if they also call logout_all_sessions() and either add the missing call or refactor to change_username().

Daverball · 2026-05-26T09:38:52Z

@@ -37,6 +40,8 @@ def remember(
    ) -> None:
        for key in self.required_keys:
            request.browser_session[key] = getattr(identity, key)
+        if (uid := getattr(identity, 'uid', None)) is not None:
+            request.browser_session['uid'] = uid


Just add uid to required_keys instead. I think I just forgot to do that, when I added uid to Identity.

Daverball

Looks good overall, but we can simplify some of the uid vs userid logic.

Daverball · 2026-05-26T09:42:12Z

+            uid = getattr(self.identity, 'uid', None)
+            if uid:
+                self._current_user = (
+                    self.session.query(User)
+                    .filter(User.id == UUID(uid))
+                    .first()
+                )
+                if (
+                    self._current_user is not None
+                    and self.identity.userid != self._current_user.username
+                ):
+                    # stale username in session — fix it for subsequent
+                    # requests
+                    self.browser_session['userid'] = (
+                        self._current_user.username)
+            else:
+                self._current_user = (
+                    self.session.query(User)
+                    .filter_by(username=self.identity.userid)
+                    .first()
+                )


This branching should not be necessary, valid identities always should have a uid. The only reason it sometimes didn't before, is because we forgot to put it in the cache along with the other identity attributes.

Daverball · 2026-05-26T09:45:55Z

+        user = self.current_user
+        if user is not None:
+            return user.username


I'm not sure I'm entirely comfortable with this change. It is a little bit more robust, but it means we always have to load the entire user object on every request where we access this property, even though we have the username cached and it should always be correct, if we didn't leave an old session hanging around, that should've been terminated.

I think we should try to get away with the old implementation, but we could check if _current_user already exist, when it does we can return _current_user.username instead. But I don't think we should emit an extra database query, unless we definitely need it.

Daverball · 2026-05-26T09:49:32Z

+        uid = getattr(identity, 'uid', None)
+        if uid:
+            user = self.users.by_id(UUID(uid))
+            if user is not None:
+                return user


Once again, this branch should be unnecessary and we should skip the query if identity.uid is None, I think you also don't need to convert to UUID, SQLAlchemy should automatically coerce the value to the correct type.

Daverball · 2026-05-26T09:58:20Z

+def test_user_change_username() -> None:
+    user = User(username='old@example.org')
+
+    with patch('onegov.user.models.user.remembered') as remembered:
+        with patch('onegov.user.models.user.forget') as forget:
+            remembered.return_value = True
+
+            user.save_current_session(DummyRequest('sess1'))  # type: ignore[arg-type]
+            assert user.sessions is not None
+            assert 'sess1' in user.sessions
+
+            user.change_username('new@example.org', DummyRequest('zzz').app)  # type: ignore[arg-type]
+
+            assert call(None, 'sess1') in forget.mock_calls
+            assert user.username == 'new@example.org'


Suggested change

def test_user_change_username() -> None:

user = User(username='old@example.org')

with patch('onegov.user.models.user.remembered') as remembered:

with patch('onegov.user.models.user.forget') as forget:

remembered.return_value = True

user.save_current_session(DummyRequest('sess1')) # type: ignore[arg-type]

assert user.sessions is not None

assert 'sess1' in user.sessions

user.change_username('new@example.org', DummyRequest('zzz').app) # type: ignore[arg-type]

assert call(None, 'sess1') in forget.mock_calls

assert user.username == 'new@example.org'

def test_user_change_username() -> None:

user = User(username='old@example.org')

with (

patch('onegov.user.models.user.remembered') as remembered,

patch('onegov.user.models.user.forget') as forget

):

remembered.return_value = True

user.save_current_session(DummyRequest('sess1')) # type: ignore[arg-type]

assert user.sessions is not None

assert 'sess1' in user.sessions

user.change_username('new@example.org', DummyRequest('zzz').app) # type: ignore[arg-type]

assert call(None, 'sess1') in forget.mock_calls

assert user.username == 'new@example.org'

You can invoke multiple context managers in a single with statement, so you don't need to indent twice.

Tschuppi81 and others added 2 commits May 8, 2026 10:03

OGC-3157: Add unit tests for OrgRequest.get_current_user()

5218b0b

Tests that get_current_user() returns the user when present, and raises HTTPForbidden while sending a Sentry warning when current_user is None. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Tschuppi81 requested a review from Copilot May 8, 2026 14:06

Copilot started reviewing on behalf of Tschuppi81 May 8, 2026 14:07 View session

Copilot AI reviewed May 8, 2026

View reviewed changes

Tschuppi81 and others added 5 commits May 8, 2026 10:19

OGC-3157: Fix invalid extras kwarg in sentry capture_message call

8943e52

Embed the username directly in the message string instead of using the non-existent extras= parameter. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

OGC-3157: Avoid shadowing user parameter in can_change_username

8cef8b8

Rename get_current_user() result to current_user to distinguish the requester from the user argument being inspected. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

OGC-3157: Move pytest import to module scope in test_request.py

7685145

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Tschuppi81 requested a review from Copilot May 8, 2026 14:23

Tschuppi81 requested a review from Daverball May 8, 2026 14:25

Copilot started reviewing on behalf of Tschuppi81 May 8, 2026 14:26 View session

Copilot AI reviewed May 8, 2026

View reviewed changes

Comment thread src/onegov/org/request.py Outdated

Comment thread tests/onegov/org/test_request.py Outdated

Comment thread src/onegov/translator_directory/custom.py

Comment thread src/onegov/town6/custom.py

Comment thread src/onegov/feriennet/custom.py

Comment thread src/onegov/org/custom.py

Tschuppi81 and others added 5 commits May 8, 2026 10:57

Feriennet: Use get_current_user() in personal tools nav for stale ses…

7612350

…sions Same fix as town6: replace the silent early return with get_current_user() so a stale-session case is reported to Sentry and handled consistently. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Org: Use get_current_user() in ticket nav to surface stale sessions

506ea53

Replace the silent current_user is not None guard with get_current_user() so stale-session inconsistencies are reported to Sentry consistently. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Tschuppi81 requested a review from Copilot May 8, 2026 15:06

Copilot started reviewing on behalf of Tschuppi81 May 8, 2026 15:07 View session

Copilot AI reviewed May 8, 2026

View reviewed changes

Comment thread src/onegov/org/request.py Outdated

Comment thread src/onegov/org/request.py Outdated

Comment thread src/onegov/translator_directory/custom.py

Comment thread src/onegov/translator_directory/generate_docx.py Outdated

Tschuppi81 and others added 4 commits May 8, 2026 11:16

Tschuppi81 requested a review from Copilot May 8, 2026 15:24

Copilot started reviewing on behalf of Tschuppi81 May 8, 2026 15:25 View session

Copilot AI reviewed May 8, 2026

View reviewed changes

Comment thread src/onegov/translator_directory/generate_docx.py Outdated

Tschuppi81 added 3 commits May 19, 2026 07:09

Revert the broad use of get_current_user

8756ae9

Merge branch 'master' into feature/ogc-3157-current-user-none-crash

9ee6e45

Delete not needed file

f1cc9b6

Call change_username to ensure logout from all sessions

26957b6

Daverball reviewed May 26, 2026

View reviewed changes

Conversation

Tschuppi81 commented May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linear Bot commented May 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented May 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Tschuppi81 commented May 8, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Daverball commented May 8, 2026

Uh oh!

Tschuppi81 commented May 22, 2026

Uh oh!

Daverball commented May 22, 2026

Uh oh!

Daverball May 26, 2026

Choose a reason for hiding this comment

Uh oh!

Daverball left a comment

Choose a reason for hiding this comment

Uh oh!

Daverball May 26, 2026

Choose a reason for hiding this comment

Uh oh!

Daverball May 26, 2026

Choose a reason for hiding this comment

Uh oh!

Daverball May 26, 2026

Choose a reason for hiding this comment

Uh oh!

Daverball May 26, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Tschuppi81 commented May 8, 2026 •

edited

Loading

codecov Bot commented May 8, 2026 •

edited

Loading