Skip to content

IVS-593 - Users are unable to self report vendor association#209

Merged
Ghesselink merged 3 commits intodevelopmentfrom
IVS-593_Users_are_unable_to_self_report_vendor_association
Jul 31, 2025
Merged

IVS-593 - Users are unable to self report vendor association#209
Ghesselink merged 3 commits intodevelopmentfrom
IVS-593_Users_are_unable_to_self_report_vendor_association

Conversation

@rw-bsi
Copy link
Contributor

@rw-bsi rw-bsi commented Jul 30, 2025

Impacts new users or users who have cleared their cookies
(added Playwright tests that were failing before the fix)

@aothms
Copy link
Collaborator

aothms commented Jul 31, 2025

Isn't the issue simply this single line change: https://github.com/buildingSMART/validate/blame/development/backend/apps/ifc_validation_bff/views_legacy.py#L206 from @ensure_csrf_cookie to @requires_csrf_token? requires_csrf_token is specific to django templates and forms I thought that injects the token there into the namespace.

@rw-bsi
Copy link
Contributor Author

rw-bsi commented Jul 31, 2025

Isn't the issue simply this single line change: https://github.com/buildingSMART/validate/blame/development/backend/apps/ifc_validation_bff/views_legacy.py#L206 from @ensure_csrf_cookie to @requires_csrf_token? requires_csrf_token is specific to django templates and forms I thought that injects the token there into the namespace.

Before this change we had a mix of methods either not checking the token (on POST to /me for example it wasn't checking CSRF), or checking the token or issuing a new cookie. Indeed with templates you don't need to worry about re-setting the CSRF but with React/API they need to be manually controlled afaik.

This PR change now: always sets the cookie with each API request (@ensure_csrf_cookie) and always validates them (@requires_csrf_token) for modifying HTTP verbs (POST, DELETE, PUT etc).

@aothms
Copy link
Collaborator

aothms commented Jul 31, 2025

and always validates them (@requires_csrf_token)

That's exactly my question. In this sense I find the django docs vague and badly named, but according to my long term memory requires_csrf_token does not validate, but only provides the token to the template context. Maybe @csrf_protect does validate.

requires_csrf_token(view)

Normally the csrf_token template tag will not work if CsrfViewMiddleware.process_view or an equivalent like csrf_protect has not run. The view decorator requires_csrf_token can be used to ensure the template tag does work. This decorator works similarly to csrf_protect, but never rejects an incoming request.

Not entirely sure what This decorator works similarly to csrf_protect, but never rejects an incoming request. means, but never rejects an incoming request does not sound like it validates. Not sure why it's similar to csrf_protect then though...

@rw-bsi
Copy link
Contributor Author

rw-bsi commented Jul 31, 2025

That's exactly my question. In this sense I find the django docs vague and badly named, but according to my long term memory requires_csrf_token does not validate, but only provides the token to the template context. Maybe @csrf_protect does validate.

This is my interpretation how it works (and I agree, Django docs are not conclusive)

@ensure_csrf_cookie sets the cookie (in the HTTP response)
We do this for all API methods so any API call will forcibly set the cookie

@requires_csrf_token checks if there is any CSRF token present in the view (in the Django middleware, in HTTP request pipeline)
Checkes either the cookie or the HTTP header (X-CSRF-Token) for templating purposes
In theory only required for POST, DELETE and PUT - not for GET or OPTIONS etc...

@csrf_protect checks if the actual value is correct - seems a better candidate indeed

I updated the PR to be more selective about it and to switch @requires_csrf_token into @csrf_protect; initially I didn't want to have to think about it: always set, always validate, always send back and forth

@rw-bsi rw-bsi requested review from aothms and removed request for civilx64 July 31, 2025 09:18
Copy link
Contributor

@Ghesselink Ghesselink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manual check & e2e tests are all passing

@Ghesselink Ghesselink merged commit beadc3f into development Jul 31, 2025
3 checks passed
@Ghesselink Ghesselink deleted the IVS-593_Users_are_unable_to_self_report_vendor_association branch July 31, 2025 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants