-
Notifications
You must be signed in to change notification settings - Fork 895
[Feature:System] Add verification of WebSocket pages #11634
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
Conversation
…ing for polls and grading features
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11634 +/- ##
============================================
+ Coverage 21.66% 21.70% +0.03%
- Complexity 9531 9582 +51
============================================
Files 268 268
Lines 36395 36560 +165
Branches 475 475
============================================
+ Hits 7886 7936 +50
- Misses 28038 28153 +115
Partials 471 471
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't approve or request changes since I technically am the author, but one change needs to get made and then it should be looking pretty good.
### Please check if the PR fulfills these requirements: * [ ] Tests for the changes have been added/updated (if possible) * [ ] Documentation has been updated/added if relevant * [ ] Screenshots are attached to Github PR if visual/UI changes were made ### What is the current behavior? When viewing a poll, the authorization logic is handled within the polls controller which makes it inaccessible to anywhere else in the codebase that might need to verify if a user should have access to a poll. ### What is the new behavior? The logic is moved to Access.php under `poll.view` and `poll.view.histogram` ### Other information? <!-- Is this a breaking change? --> <!-- How did you test --> Needed for #11634 --------- Co-authored-by: William Allen <16820599+williamjallen@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should consider adding a method to revoke websocket tokens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR makes multiple improvements to the ease of development of websocket features, and makes them more secure as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks good. Still looking at a couple security aspects. Left a couples comments and may have some more follow up once some questions are answered.
I think the short lived 5 minute tokens work fine but there is a downside to their lifetime. Sometimes the websocket server needs a manual restart (or has even crashed) which unfortunately drops all connections and makes everyone reconnect. With this approach, after 5 minutes of being on the page, they won't be able to reconnect until a page reload which is unfortunate. A good solution to this would be to recognize the expiration time or the auth failure, and request a new websocket token via AJAX. However this can definitely be a future improvement issue as this PR is already quite large.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me. No concerns security wise. Just one minor comment about the testing which can quickly be fixed (and worst case it could be addressed in a future PR). Note this is just a code review and I have not locally tested this.
Please check if the PR fulfills these requirements:
What is the current behavior?
Any WebSocket page can be opened and monitored by anyone with the correct page name and basic authentication.
What is the new behavior?
Connection requests to a given WebSocket page are now entirely authorized through a WebSocket authorization token, managed by the web server. The proposed window for the token is 5 minutes, during which old authorized pages that have not been revisited within their expiration window will eventually be filtered out on the next token refresh request through a sliding window approach.
Other information?
This implementation replaces the original authorization logic with a JWT-based system, significantly reducing authentication time during WebSocket connection setup as we no longer make an external request to the database. The testing data is based on various new WebSocket page creations for new connections and page reload cases.