Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix role checking when using websocket push #20679

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

mcollovati
Copy link
Collaborator

Description

When using PUSH with websocket transport, the atmosphere wrapped request can be a no-op implementation whose isUserInRole method alwasy returns false, causing, for example, wrong access checking during navigation. This change falls back to Spring Securty for role checking when PUSH transport is websocket.
It also fixes some tests in order to propagate the Spring Security context when starting Thread that perform UI operations.

References psi#123
Part of #11026

Type of change

  • Bugfix
  • Feature

Checklist

  • I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
  • I have added a description following the guideline.
  • The issue is created in the corresponding repository and I have referenced it.
  • I have added tests to ensure my change is effective and works as intended.
  • New and existing tests are passing locally with my change.
  • I have performed self-review and corrected misspellings.

Additional for Feature type of change

  • Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

When using PUSH with websocket transport, the atmosphere wrapped request
can be a no-op implementation whose isUserInRole method alwasy returns
false, causing, for example, wrong access checking during navigation.
This change falls back to Spring Securty for role checking when PUSH
transport is websocket.
It also fixes some tests in order to propagate the Spring Security context
when starting Thread that perform UI operations.

References psi#123
Part of #11026
Copy link

Test Results

1 159 files  ± 0  1 159 suites  ±0   1h 32m 34s ⏱️ - 1m 59s
7 547 tests + 1  7 493 ✅ + 1  54 💤 ±0  0 ❌ ±0 
7 862 runs   - 55  7 800 ✅  - 54  62 💤  - 1  0 ❌ ±0 

Results for commit 9161b37. ± Comparison against base commit c7a0714.

This pull request removes 5 and adds 6 tests. Note that renamed tests count towards both.
com.vaadin.flow.spring.flowsecurity.AppViewIT ‑ logout_via_doLogin_redirects_to_logout
com.vaadin.flow.spring.flowsecuritycontextpath.AppViewIT ‑ logout_via_doLogin_redirects_to_logout
com.vaadin.flow.spring.flowsecurityreverseproxy.AppViewIT ‑ logout_via_doLogin_redirects_to_logout
com.vaadin.flow.spring.flowsecurityurlmapping.AppViewIT ‑ logout_via_doLogin_redirects_to_logout
com.vaadin.flow.spring.flowsecuritywebsocket.AppViewIT ‑ logout_via_doLogin_redirects_to_logout
com.vaadin.flow.spring.flowsecurity.AppViewIT ‑ logout_via_doLogoutURL_redirects_to_logout
com.vaadin.flow.spring.flowsecuritycontextpath.AppViewIT ‑ logout_via_doLogoutURL_redirects_to_logout
com.vaadin.flow.spring.flowsecurityreverseproxy.AppViewIT ‑ logout_via_doLogoutURL_redirects_to_logout
com.vaadin.flow.spring.flowsecurityurlmapping.AppViewIT ‑ logout_via_doLogoutURL_redirects_to_logout
com.vaadin.flow.spring.flowsecuritywebsocket.AppViewIT ‑ logout_via_doLogoutURL_redirects_to_logout
com.vaadin.flow.spring.flowsecuritywebsocket.AppViewIT ‑ websocket_roles_checked_correctly_during_navigation
This pull request removes 1 skipped test and adds 1 skipped test. Note that renamed tests count towards both.
com.vaadin.flow.spring.flowsecuritywebsocket.AppViewIT ‑ logout_via_doLogin_redirects_to_logout
com.vaadin.flow.spring.flowsecuritywebsocket.AppViewIT ‑ logout_via_doLogoutURL_redirects_to_logout

@mshabarov mshabarov merged commit 5ff8cf5 into main Dec 13, 2024
26 checks passed
@mshabarov mshabarov deleted the psi/123-spring-security-websocket-role-checker branch December 13, 2024 14:54
@vaadin-bot
Copy link
Collaborator

Hi @mcollovati and @mshabarov, when i performed cherry-pick to this commit to 24.6, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 5ff8cf5
error: could not apply 5ff8cf5... fix: fix role checking when using websocket push (#20679)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

@vaadin-bot
Copy link
Collaborator

Hi @mcollovati and @mshabarov, when i performed cherry-pick to this commit to 24.5, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 5ff8cf5
error: could not apply 5ff8cf5... fix: fix role checking when using websocket push (#20679)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

@vaadin-bot
Copy link
Collaborator

Hi @mcollovati and @mshabarov, when i performed cherry-pick to this commit to 24.4, i have encountered the following issue. Can you take a look and pick it manually?
Error Message:
Error: Command failed: git cherry-pick 5ff8cf5
error: could not apply 5ff8cf5... fix: fix role checking when using websocket push (#20679)
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".

mcollovati added a commit that referenced this pull request Dec 13, 2024
When using PUSH with websocket transport, the atmosphere wrapped request
can be a no-op implementation whose isUserInRole method alwasy returns
false, causing, for example, wrong access checking during navigation.
This change falls back to Spring Securty for role checking when PUSH
transport is websocket.
It also fixes some tests in order to propagate the Spring Security context
when starting Thread that perform UI operations.

References psi#123
Part of #11026
mcollovati added a commit that referenced this pull request Dec 13, 2024
When using PUSH with websocket transport, the atmosphere wrapped request
can be a no-op implementation whose isUserInRole method alwasy returns
false, causing, for example, wrong access checking during navigation.
This change falls back to Spring Securty for role checking when PUSH
transport is websocket.
It also fixes some tests in order to propagate the Spring Security context
when starting Thread that perform UI operations.

References psi#123
Part of #11026
mcollovati added a commit that referenced this pull request Dec 13, 2024
When using PUSH with websocket transport, the atmosphere wrapped request
can be a no-op implementation whose isUserInRole method alwasy returns
false, causing, for example, wrong access checking during navigation.
This change falls back to Spring Securty for role checking when PUSH
transport is websocket.
It also fixes some tests in order to propagate the Spring Security context
when starting Thread that perform UI operations.

References psi#123
Part of #11026
mcollovati added a commit that referenced this pull request Dec 13, 2024
When using PUSH with websocket transport, the atmosphere wrapped request
can be a no-op implementation whose isUserInRole method alwasy returns
false, causing, for example, wrong access checking during navigation.
This change falls back to Spring Securty for role checking when PUSH
transport is websocket.
It also fixes some tests in order to propagate the Spring Security context
when starting Thread that perform UI operations.

References psi#123
Part of #11026
mcollovati added a commit that referenced this pull request Dec 16, 2024
When using PUSH with websocket transport, the atmosphere wrapped request
can be a no-op implementation whose isUserInRole method alwasy returns
false, causing, for example, wrong access checking during navigation.
This change falls back to Spring Securty for role checking when PUSH
transport is websocket.
It also fixes some tests in order to propagate the Spring Security context
when starting Thread that perform UI operations.

References psi#123
Part of #11026
mcollovati added a commit that referenced this pull request Dec 16, 2024
When using PUSH with websocket transport, the atmosphere wrapped request
can be a no-op implementation whose isUserInRole method alwasy returns
false, causing, for example, wrong access checking during navigation.
This change falls back to Spring Securty for role checking when PUSH
transport is websocket.
It also fixes some tests in order to propagate the Spring Security context
when starting Thread that perform UI operations.

References psi#123
Part of #11026
mcollovati added a commit that referenced this pull request Dec 16, 2024
When using PUSH with websocket transport, the atmosphere wrapped request
can be a no-op implementation whose isUserInRole method alwasy returns
false, causing, for example, wrong access checking during navigation.
This change falls back to Spring Securty for role checking when PUSH
transport is websocket.
It also fixes some tests in order to propagate the Spring Security context
when starting Thread that perform UI operations.

References psi#123
Part of #11026
mcollovati added a commit that referenced this pull request Dec 16, 2024
When using PUSH with websocket transport, the atmosphere wrapped request
can be a no-op implementation whose isUserInRole method alwasy returns
false, causing, for example, wrong access checking during navigation.
This change falls back to Spring Securty for role checking when PUSH
transport is websocket.
It also fixes some tests in order to propagate the Spring Security context
when starting Thread that perform UI operations.

References psi#123
Part of #11026
mcollovati added a commit that referenced this pull request Dec 16, 2024
When using PUSH with websocket transport, the atmosphere wrapped request
can be a no-op implementation whose isUserInRole method alwasy returns
false, causing, for example, wrong access checking during navigation.
This change falls back to Spring Securty for role checking when PUSH
transport is websocket.
It also fixes some tests in order to propagate the Spring Security context
when starting Thread that perform UI operations.

References psi#123
Part of #11026
mcollovati added a commit that referenced this pull request Dec 16, 2024
When using PUSH with websocket transport, the atmosphere wrapped request
can be a no-op implementation whose isUserInRole method alwasy returns
false, causing, for example, wrong access checking during navigation.
This change falls back to Spring Securty for role checking when PUSH
transport is websocket.
It also fixes some tests in order to propagate the Spring Security context
when starting Thread that perform UI operations.

References psi#123
Part of #11026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants