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.

Sign up for GitHub

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

Jump to bottom

[websocket] Allow registering websocket adapters #3622

Merged
merged 10 commits into from
Jun 10, 2023
Merged

[websocket] Allow registering websocket adapters #3622

merged 10 commits into from
Jun 10, 2023

Conversation

GiviMAD
Copy link
Member

@GiviMAD GiviMAD commented May 21, 2023 •
edited
Loading

Hi openHAB community, I would like to merge this PR which allows to register multiple WebSocket handlers in to a common servlet which handles the authentication.

I have refactored the already present implementation into a separate handler (EventWebSocketHandler) that is served in the root path '/ws' so I think this is not a breaking change.

Also I have made some methods in the AuthFilter (package org.openhab.core.io.rest.auth) public so they can be reused here, so the "Trusted Network" and "Allow Basic Auth" rest api options apply also to the WebSocket and this can be accessed with the same credentials than the rest api (before the commit it allowed user tokens and basic auth but no JWT tokens).

Hope everything is in place. Best regards!

GiviMAD added 2 commits May 21, 2023 12:25
@GiviMAD
[WebSocket] Allow register websocket handlers
fa412d6 
Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
@GiviMAD
fix static code analysis
496a823 
Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
@GiviMAD GiviMAD requested a review from a team as a code owner May 21, 2023 10:43
@GiviMAD
made AuthFilter and AnonymousUserSecurityContext non internal
6083369 
Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
@GiviMAD
Copy link
Member Author

GiviMAD commented May 21, 2023

Seems like these changes break the core karaf feature, I'm trying to understand why, I don't have experience with these definitions. If someone can give me some some help will be appreciated.

@GiviMAD
fix core karaf feature install
f01f0e6 
Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
@GiviMAD
Copy link
Member Author

GiviMAD commented May 21, 2023

Luckily i was able to fix it :)

splatch
splatch reviewed May 27, 2023
View reviewed changes
Copy link
Contributor

@splatch splatch left a comment

Choose a reason for hiding this comment

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

I have some remarks on direction in which use of security goes. Please consider them. Driving further adoption of rest filter as a main access control mechanism will make it difficult to adopt different authentication mechanisms requested in #3305.

...e.io.rest.auth/src/main/java/org/openhab/core/io/rest/auth/AnonymousUserSecurityContext.java Show resolved Hide resolved
...es/org.openhab.core.io.rest.auth/src/main/java/org/openhab/core/io/rest/auth/AuthFilter.java Show resolved Hide resolved
...es/org.openhab.core.io.rest.auth/src/main/java/org/openhab/core/io/rest/auth/AuthFilter.java Outdated Show resolved Hide resolved
bundles/org.openhab.core.io.websocket/pom.xml Show resolved Hide resolved
...ab.core.io.websocket/src/main/java/org/openhab/core/io/websocket/CommonWebSocketServlet.java Show resolved Hide resolved
...ab.core.io.websocket/src/main/java/org/openhab/core/io/websocket/CommonWebSocketServlet.java Outdated Show resolved Hide resolved
...hab.core.io.websocket/src/main/java/org/openhab/core/io/websocket/EventWebSocketHandler.java Outdated Show resolved Hide resolved
features/karaf/openhab-core/src/main/feature/feature.xml Show resolved Hide resolved
@GiviMAD
apply pr review
66159e5 
Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
@J-N-K J-N-K added the enhancement An enhancement or new feature of the Core label May 27, 2023
@GiviMAD GiviMAD requested a review from splatch May 27, 2023 13:28
GiviMAD added 3 commits May 27, 2023 15:29
@GiviMAD
apply spotless
46a3521 
Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
@GiviMAD
fix test
8caa3dc 
Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
@GiviMAD
remove unnecesary nullable annotation
d02d269 
Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
@GiviMAD
Copy link
Member Author

GiviMAD commented May 27, 2023

Tests failure seems unrelated to the PR.

@GiviMAD
apply pr review
8123a77 
Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
@GiviMAD GiviMAD requested a review from J-N-K June 9, 2023 14:47
@GiviMAD
Copy link
Member Author

GiviMAD commented Jun 9, 2023 •
edited
Loading

I'm still not so happy with the AuthFilter changes, but unfortunately I don't have a much better idea at the moment. So I would say we could merge that.

I have an idea, I'm not sure if it's the best way to do it, but I'll leave it here in case you think it's a good idea or it might inspire something better.

That will be:

  • Creating a new class APISecurity in org.openhab.core.auth and handle there the api-security configuration and move there also all the code related to network verification and authentication that is currently on the AuthFilter class.
  • Leaving the AutFilter class just as a ContainerRequestFilter that delegate its logic to the APISecurity class.
  • The websocket security can be also delegated to the APISecurity class removing the AuthFilter dependency.
  • This will involve moving also some other classes from org.openhab.core.io.rest.auth.internal to org.openhab.core.auth (or org.openhab.core.auth.internal) like the JwtHelper, JwtSecurityContext, and others.

@J-N-K
Copy link
Member

J-N-K commented Jun 9, 2023

I think that's the idea of the AuthenticationManager. But we should probably move that to another PR.

J-N-K
J-N-K requested changes Jun 9, 2023
View reviewed changes
Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

....openhab.core.io.websocket/src/main/java/org/openhab/core/io/websocket/WebSocketHandler.java Outdated Show resolved Hide resolved
@GiviMAD
apply pr review
babd5a6 
Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
@GiviMAD GiviMAD requested a review from J-N-K June 10, 2023 11:10
J-N-K
J-N-K approved these changes Jun 10, 2023
View reviewed changes
Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

Thanks!

@J-N-K J-N-K added this to the 4.0 milestone Jun 10, 2023
@J-N-K J-N-K merged commit e3396c9 into openhab:main Jun 10, 2023
@J-N-K J-N-K changed the title [WebSocket] Allow register websocket handlers [websocket] Allow registering websocket adapters Jun 10, 2023
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/oh4-sign-in-fails-repeteadly-on-snapshot/147207/3

@jlaur
Copy link
Contributor

jlaur commented Jun 11, 2023

@GiviMAD - do you think this could be related?
https://community.openhab.org/t/oh4-sign-in-fails-repeteadly-on-snapshot/147207

@J-N-K J-N-K mentioned this pull request Jun 11, 2023
Merged
@GiviMAD
Copy link
Member Author

GiviMAD commented Jun 11, 2023

@GiviMAD - do you think this could be related?
https://community.openhab.org/t/oh4-sign-in-fails-repeteadly-on-snapshot/147207

Seems like @J-N-K just found the problem, thank you for handled it and sorry.

splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
@GiviMAD @splatch
[websocket] Allow registering websocket adapters (openhab#3622)
8bd410e 
* [WebSocket] Allow register websocket handlers

Signed-off-by: Miguel Álvarez <miguelwork92@gmail.com>
GitOrigin-RevId: e3396c9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@J-N-K J-N-K J-N-K approved these changes

@splatch splatch Awaiting requested review from splatch

Assignees
No one assigned
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
5 participants
@GiviMAD @J-N-K @openhab-bot @jlaur @splatch