Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

Add support for conditional access based on roles. Part one. #6034

Merged
merged 1 commit into from
Oct 6, 2018

Conversation

splatch
Copy link
Contributor

@splatch splatch commented Aug 13, 2018

This commit addresses issue #579. It does not solve it fully, but creates a base for security mechanism which covers main entrypoints to Eclipse SmartHome - meaning REST and other servlet based interactions such icons and charts.

While this is just beginning there is lots of things to be reviewed and validated. Initial review of code and its execution have been done by @svilenvul with positive feedback. Issues found by him were addressed in fork.

@splatch splatch force-pushed the ESH-579 branch 3 times, most recently from 13a3556 to d712fab Compare August 14, 2018 16:15
@splatch
Copy link
Contributor Author

splatch commented Aug 14, 2018

FYI - I've signed CLA long time ago using lukasz@dywicki.pl mail, however ip-validation bot does not pick up signed off tag value.

@kaikreuzer
Copy link
Contributor

No worries. I have manually checked your ECA and can confirm that it is valid. This PR will anyhow have to go through IP team approval, so just ignore the IP validation failure here.

Copy link
Contributor

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Many thanks @splatch, this is an excellent work and Iimho a wonderful basis to build on!

I very much like the chosen architecture and the functional split, I would merely try to slightly reduce the number of bundles where it doesn't hurt, see some comments below.

I couldn't get the code to work (in a sense that I'd been able to login), but afaics, this is simply not yet fully implemented, correct?

So what's your plan for the next steps? I think I can (at least from my perspective) give you green light for what you have done so far and the direction you are heading - I'd therefore love to see a working scenario with Basic Auth very soon and one with JWT shortly after :-)

@@ -10,7 +10,9 @@
*
* SPDX-License-Identifier: EPL-2.0
*/
package org.eclipse.smarthome.core.auth;
package org.eclipse.smarthome.auth.password;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it really worth to create a new bundle for this small class?
Do you expect many more classes being added to that bundle, which might have dependencies that we would not want to have in smarthome.core? Otherwise, I'd think that we could leave it where it was.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this away as I can think of use cases where password authentication is not one supported by default. Also moving credential to separate packages causes that extractors working with password based access are free of dependency to smarthome core.

Copy link
Contributor

Choose a reason for hiding this comment

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

Defining that there is such a thing as password credentials does not mean that there is support for it - the support only comes by including the http.auth.basic bundle.

are free of dependency to smarthome core.

Well, smarthome.core.auth contains the basic building blocks, and it is expected to be the root dependency for every auth feature - I don't think this is therefore an argument.

import org.osgi.service.http.HttpContext;

/**
* Http context which does nothing bug lets delegate to do it's job.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: bug -> but
typo: it's -> its
reformulate: -> but lets the delegate do its job

@@ -14,4 +14,10 @@
<packaging>eclipse-plugin</packaging>

<name>Eclipse SmartHome REST JAX-RS Optimizations</name>

<properties>
Copy link
Contributor

Choose a reason for hiding this comment

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

you can remove those, we usually do not add these properties anymore

Copy link
Contributor

Choose a reason for hiding this comment

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

That's right. See #6060.

import org.slf4j.LoggerFactory;

/**
* Base class for HTTP servlets developed in Eclipse Smart Home.
Copy link
Contributor

Choose a reason for hiding this comment

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

remove "developed in Eclipse Smart Home."


protected void activate(String alias, HttpContext httpContext) {
try {
logger.debug("Starting up chart servlet at " + alias);
Copy link
Contributor

Choose a reason for hiding this comment

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

chart servlet?
Please use parameterized logging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You got me. This is copy paste.

@@ -0,0 +1,33 @@
<?xml version="1.0" encoding="UTF-8"?>
<projectDescription>
<name>org.eclipse.smarthome.io.http.core</name>
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have a org.eclipse.smarthome.io.http bundle, so this somehow duplicates it.
I think we could easily move the classes of this bundle to a package org.eclipse.smarthome.io.http.servlet in the org.eclipse.smarthome.io.http bundle and remove this one here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I messed up dot files (I have global git ignore to not commit them by default) and then I been adding them manually on other machine. Will fix.

@Component
public class LoginService {

private static final String LOGIN_FORM_ADDRESS = "/login/form";
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be "/login", so that the page is available as /login/index.html.
It might even be more elegant to register a servlet under /login, which simply serves the page.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed that the ?process is anyhow not processed by the form, so I assume you'll have to make this a servlet anyhow, so that you can set the Basic Auth HTTP header, right?

<body>

<div id="login_form">
<form name="login-form" method="post" action="/login/form/index.html?process" id="f1">
Copy link
Contributor

Choose a reason for hiding this comment

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

Better don't use an absolute, but a relative url here - index.html?process should just be fine.


// configuration properties
private boolean enabled = true;
private String loginUri = "/login";
Copy link
Contributor

Choose a reason for hiding this comment

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

Use a url that works in ESH out of the box without requiring any configuration - e.g. "/login/index.html", if this is where we'd register the form.


if (error instanceof AuthenticationException) {
// force client redirect
response.setHeader("Location", loginUri);
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't you pass the called url as a parameter, so that the form can actually redirect back once the user authenticated successfully?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, will update code to support such scenario.

@splatch
Copy link
Contributor Author

splatch commented Aug 15, 2018

@kaikreuzer Thank you very much for review and kind words.

I couldn't get the code to work (in a sense that I'd been able to login), but afaics, this is simply not yet fully implemented, correct?

I found that sometimes I had to refresh or restart some bundles in order to get all services working together. Code which is currently implemented offers only very thin protection layer, namely a guard, and does not populate nor keep session. This means that even if you pass authentication with basic credentials - you still might get redirected to login page on next click. This is because there is no session, and no cookie returned to client so auth result is simply lost over time. In order to keep login operation result somewhat more persistent we need to co come with local JWT representing Authentication. If we will have that we could use a cookie to store a token until all UIs support authentication of out the box.
I didn't reach this place yet because it touches a bit of authorization planned for second phase. To start this we need to define JWT claim names (or just single name we want to use for keeping role) and their values.

So what's your plan for the next steps? I think I can (at least from my perspective) give you green light for what you have done so far and the direction you are heading - I'd therefore love to see a working scenario with Basic Auth very soon and one with JWT shortly after :-)

I used so far form and basic login to verify if entire idea works. My main intention was to provide html form for human interactions (to not rely on basic auth) and leave HTTP basic for calls from various shell scripts. Both basic and form credential extractors could have additional configuration to restrict what client addresses they do accept. By this a access based on HTTP basic, which is extremely easy to tap, could be restricted just to localhost.
Basic auth should work already. I didn't plan yet to work on JWT due to reasons listed above.

I tested entire thing with openhab distro and it have worked. I will update code, retest and update PR.

@kaikreuzer
Copy link
Contributor

I tested entire thing with openhab distro and it have worked.

Well, we probably have a different definition of "working" then :-)
Yes, all bundles resolved and services started and all hooks worked indeed - so what is there is working as expected, I agree.
It is just that the parts do not yet interact in a way that it brings any working feature to the user: The redirect to the login form fails due to a wrong default url, the submission of form data is not at all processed and thus the user is never authenticated, i.e. cannot access any page. And as you say, even if that would work, due to a lacking session, it wouldn't be usable.

I agree that BasicAuth is not the way to go, but as long as we are working on JWT support, it might be an idea to change the login form to ask the browser to do BasicAuth (which it will keep sending in further requests, if I am not mistaken). This way, we could at least have a working prototype. Wdyt?

Wrt JWT claim names: I have no experience in how to choose them and what makes sense there. Do you have some references and could come up with a suggestion? @JochenHiller Do you have any wishes/suggestions here?

<properties>
<bundle.symbolicName>org.eclipse.smarthome.io.http.auth.basic</bundle.symbolicName>
<bundle.namespace>org.eclipse.smarthome.io.http.auth.basic</bundle.namespace>
</properties>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove these properties. See #6060.

<properties>
<bundle.symbolicName>org.eclipse.smarthome.io.http.auth.form</bundle.symbolicName>
<bundle.namespace>org.eclipse.smarthome.io.http.auth.form</bundle.namespace>
</properties>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove these properties. See #6060.

<properties>
<bundle.symbolicName>org.eclipse.smarthome.io.http.auth.jwt</bundle.symbolicName>
<bundle.namespace>org.eclipse.smarthome.io.http.auth.jwt</bundle.namespace>
</properties>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove these properties. See #6060.

<properties>
<bundle.symbolicName>org.eclipse.smarthome.io.http.auth</bundle.symbolicName>
<bundle.namespace>org.eclipse.smarthome.io.http.auth</bundle.namespace>
</properties>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove these properties. See #6060.

<properties>
<bundle.symbolicName>org.eclipse.smarthome.io.http.core</bundle.symbolicName>
<bundle.namespace>org.eclipse.smarthome.io.http.core</bundle.namespace>
</properties>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove these properties. See #6060.

<properties>
<bundle.symbolicName>org.eclipse.smarthome.io.http.test</bundle.symbolicName>
<bundle.namespace>org.eclipse.smarthome.io.http.test</bundle.namespace>
</properties>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove these properties. See #6060.

<properties>
<bundle.symbolicName>org.eclipse.smarthome.io.http</bundle.symbolicName>
<bundle.namespace>org.eclipse.smarthome.io.http</bundle.namespace>
</properties>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove these properties. See #6060.

@@ -14,4 +14,10 @@
<packaging>eclipse-plugin</packaging>

<name>Eclipse SmartHome REST JAX-RS Optimizations</name>

<properties>
Copy link
Contributor

Choose a reason for hiding this comment

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

That's right. See #6060.

<properties>
<bundle.symbolicName>org.eclipse.smarthome.ui.login</bundle.symbolicName>
<bundle.namespace>org.eclipse.smarthome.ui.login</bundle.namespace>
</properties>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove these properties. See #6060.

@@ -27,10 +27,14 @@
/**
* Base type for handling authentication and authorization delegated from rest/jaxrs service layer.
*
* @deprecated Since introduction of security on lower layer (servlets), this extension which is
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be ok with completely removing this class - I doubt that anyone is using this as it never fully worked.

@splatch
Copy link
Contributor Author

splatch commented Aug 17, 2018

tested entire thing with openhab distro and it have worked.

Well, we probably have a different definition of "working" then :-)

First goal which was a must was a system wide protection switch which will enable security on all servlets and that was my primary goal in submitting this pull request. I agree that it still has little or no value for end user, yet going further requires touching bits of JWT which was decided to be used as session mechanism.

Yes, all bundles resolved and services started and all hooks worked indeed - so what is there is working as expected, I agree.
It is just that the parts do not yet interact in a way that it brings any working feature to the user: The redirect to the login form fails due to a wrong default url, the submission of form data is not at all processed and thus the user is never authenticated, i.e. cannot access any page. And as you say, even if that would work, due to a lacking session, it wouldn't be usable.

I updated code to satisfy this requirement - once user enters under protected resource it is redirected to login method selection where he can just select a form. After successful authentication he is redirected back to place where he started whole process, see RedirectHandler which has been added in one of last commits. Still, even if user is properly authenticated he is forwarded back to login because authentication result is not persisted.

I agree that BasicAuth is not the way to go, but as long as we are working on JWT support, it might be an idea to change the login form to ask the browser to do BasicAuth (which it will keep sending in further requests, if I am not mistaken). This way, we could at least have a working prototype. Wdyt?

Yes, this gonna solve a cyclic redirect. I will add a basic prompt for handling that.

Wrt JWT claim names: I have no experience in how to choose them and what makes sense there. Do you have some references and could come up with a suggestion? @JochenHiller Do you have any wishes/suggestions here?

Based on my own knowledge of JWT spec I can't find any. We know it need to be short, yet unique so it doesn't conflict with any standard claim. We can use esh_role containing a JSON list for now.
Main reason why I am asking about this is because there is more work on that part to be made. We need to generate (or reuse) key pair, create token with required claims and also validate it once client sends it back. Because implementing even fragment of JWT specification requires significant amount of work I usually used 3rd party library for that. I am not sure what is your take on that since it will be an external dependency.

@openhab-bot
Copy link
Contributor

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

https://community.openhab.org/t/apache2-sitemap-protection/49658/2

Copy link
Contributor

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Thanks for the update - please find some additional feedback.

@@ -10,9 +10,11 @@ Bundle-Version: 0.10.0.qualifier
Export-Package: org.eclipse.smarthome.io.rest.auth
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not exist anymore.

@@ -10,9 +10,11 @@ Bundle-Version: 0.10.0.qualifier
Export-Package: org.eclipse.smarthome.io.rest.auth
Import-Package:
com.eclipsesource.jaxrs.provider.security,
com.eclipsesource.jaxrs.publisher;version="5.3.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the version

@@ -10,9 +10,11 @@ Bundle-Version: 0.10.0.qualifier
Export-Package: org.eclipse.smarthome.io.rest.auth
Import-Package:
com.eclipsesource.jaxrs.provider.security,
Copy link
Contributor

Choose a reason for hiding this comment

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

remove, not needed anymore

@@ -10,9 +10,11 @@ Bundle-Version: 0.10.0.qualifier
Export-Package: org.eclipse.smarthome.io.rest.auth
Import-Package:
com.eclipsesource.jaxrs.provider.security,
com.eclipsesource.jaxrs.publisher;version="5.3.1",
javax.ws.rs.container,
org.eclipse.jdt.annotation;resolution:=optional,
org.eclipse.smarthome.core.auth,
org.eclipse.smarthome.io.rest.auth,
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

public class LoginMethodsServlet extends SmartHomeServlet {

private static final String REDIRECT_PARAM_NAME = "redirect";
private static final String LOGIN_ADDRESS = "/login";
Copy link
Contributor

Choose a reason for hiding this comment

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

I get a 403 when accessing this URI...

@Component
public class FormService {

static final String LOGIN_FORM_ADDRESS = "/login/form";
Copy link
Contributor

Choose a reason for hiding this comment

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

You already register the servlet under /login - doesn't this cause conflicts here?

@@ -0,0 +1,29 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

please set these xmls to ignore

@@ -0,0 +1,8 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, please let us try to keep the number of additional bundles small: This one does not have any too specific dependencies and consists out of a single class only - this should imho be moved to the smarthome.ui bundle.

@kaikreuzer
Copy link
Contributor

Because implementing even fragment of JWT specification requires significant amount of work I usually used 3rd party library for that. I am not sure what is your take on that since it will be an external dependency.

I agree that we should not reimplement such stuff ourselves. I'd hope that there are some lean libs available, do you have any specific one in mind already?

@splatch
Copy link
Contributor Author

splatch commented Aug 20, 2018

I agree that we should not reimplement such stuff ourselves. I'd hope that there are some lean libs available, do you have any specific one in mind already?

I've used nimbusds so far and it worked quite fine with most of use cases. @antoniosv in antoniosv#1 used jose4j for that which might be also a good alternative.

@kaikreuzer
Copy link
Contributor

Both look fine to me wrt size and license.

org.eclipse.jdt.core.compiler.problem.assertIdentifier=error
org.eclipse.jdt.core.compiler.problem.enumIdentifier=error
org.eclipse.jdt.core.compiler.problem.forbiddenReference=warning
org.eclipse.jdt.core.compiler.source=1.8
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this file because it causes the default Eclipse project settings to be overridden. See also #6106.

resolveWorkspaceProjects=true
resourceFilterGoals=process-resources resources\:testResources
skipCompilerPlugin=true
version=1
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this file because it causes the default Eclipse project settings to be overridden. See also #6106.

org.eclipse.jdt.core.compiler.problem.assertIdentifier=error
org.eclipse.jdt.core.compiler.problem.enumIdentifier=error
org.eclipse.jdt.core.compiler.problem.forbiddenReference=warning
org.eclipse.jdt.core.compiler.source=1.8
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this file because it causes the default Eclipse project settings to be overridden. See also #6106.

resolveWorkspaceProjects=true
resourceFilterGoals=process-resources resources\:testResources
skipCompilerPlugin=true
version=1
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this file because it causes the default Eclipse project settings to be overridden. See also #6106.

org.eclipse.jdt.core.compiler.problem.assertIdentifier=error
org.eclipse.jdt.core.compiler.problem.enumIdentifier=error
org.eclipse.jdt.core.compiler.problem.forbiddenReference=warning
org.eclipse.jdt.core.compiler.source=1.8
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this file because it causes the default Eclipse project settings to be overridden. See also #6106.

resolveWorkspaceProjects=true
resourceFilterGoals=process-resources resources\:testResources
skipCompilerPlugin=true
version=1
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this file because it causes the default Eclipse project settings to be overridden. See also #6106.

org.eclipse.jdt.core.compiler.problem.assertIdentifier=error
org.eclipse.jdt.core.compiler.problem.enumIdentifier=error
org.eclipse.jdt.core.compiler.problem.forbiddenReference=warning
org.eclipse.jdt.core.compiler.source=1.8
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this file because it causes the default Eclipse project settings to be overridden. See also #6106.

resolveWorkspaceProjects=true
resourceFilterGoals=process-resources resources\:testResources
skipCompilerPlugin=true
version=1
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this file because it causes the default Eclipse project settings to be overridden. See also #6106.

org.eclipse.jdt.core.compiler.problem.assertIdentifier=error
org.eclipse.jdt.core.compiler.problem.enumIdentifier=error
org.eclipse.jdt.core.compiler.problem.forbiddenReference=warning
org.eclipse.jdt.core.compiler.source=1.8
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this file because it causes the default Eclipse project settings to be overridden. See also #6106.

resolveWorkspaceProjects=true
resourceFilterGoals=process-resources resources\:testResources
skipCompilerPlugin=true
version=1
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this file because it causes the default Eclipse project settings to be overridden. See also #6106.

@kaikreuzer
Copy link
Contributor

Hey @splatch, time to update this PR and get the Basic Auth working - would love to see this get merged as a first step soon 😎 .

@splatch
Copy link
Contributor Author

splatch commented Aug 30, 2018

Hey Kai, I got changes to support basic challenge, however ran into troubles with build and rebase. I'm going to update PR as soon as I'm able to confirm it works end to end.

@splatch splatch force-pushed the ESH-579 branch 2 times, most recently from f91eab7 to 94719f3 Compare September 9, 2018 16:44
@splatch
Copy link
Contributor Author

splatch commented Sep 9, 2018

I moved whole thing a little bit ahead - authentication part works, however AudioServlet tests are failing due to missing HttpService and HttpContext references. I need someones tip on how to solve that. PDE/Tycho test infrastructure remains unknown to me and I have no clue how to bring these two services alive for the test other than registering mocks.

@splatch splatch force-pushed the ESH-579 branch 4 times, most recently from 19304a9 to 68bad99 Compare September 9, 2018 23:37
@kaikreuzer
Copy link
Contributor

however AudioServlet tests are failing

Travis is happy, so what do you want more? :-)

@kaikreuzer
Copy link
Contributor

What is missing is your Signed-off-by footer on the commit message, though.

Copy link
Contributor

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, @splatch!
Looks pretty good altogether already and Basic Auth works for the servlets.

What I noticed though is that the REST API requests are not authenticated/challenged and thus JAX-RS rejects them as being not permitted. Does this work for you?


@Override
public void handleError(HttpServletRequest request, HttpServletResponse response, HandlerContext context) {
response.setHeader("WWW-Authenticate", "Basic realm=\"Please enter user name and password to access OpenHab\"");
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove " to access OpenHab"

@@ -41,6 +45,7 @@
* @author Łukasz Dywicki - Initial contribution and API
* @author Kai Kreuzer - Removed ManagedService and used DS configuration instead
*/
@Component(configurationPid = "org.eclipse.smarthome.auth.jaas")
Copy link
Contributor

Choose a reason for hiding this comment

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

please change to "org.eclipse.smarthome.jaas"

/**
* A thread local binder.
*
* TODO verify if this is really needed.
Copy link
Contributor

Choose a reason for hiding this comment

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

As the current code does not yet use it, better remove it from this PR.

*
* @author Łukasz Dywicki - initial contribution.
*/
@Component(configurationPid = "org.eclipse.smarthome.io.http.auth")
Copy link
Contributor

Choose a reason for hiding this comment

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

change to "org.eclipse.smarthome.auth"

return false;
}

// TODO add decision logic so not all URIs gets secured but only these which are told to be
Copy link
Contributor

@kaikreuzer kaikreuzer Sep 10, 2018

Choose a reason for hiding this comment

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

Any comment? (see #6034 (comment))

this.authenticationManager = authenticationManager;
}

public void unsetAuthneticationManager(AuthenticationManager authenticationManager) {
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: unsetAuthenticationManager

import org.osgi.service.component.annotations.Component;

/**
* Handler located after authentication which redirect client to page from which he started authentication process..
Copy link
Contributor

Choose a reason for hiding this comment

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

remove duplicate dot at the end

private WrappingHttpContext httpContext;

@Override
public HttpContext createDefaultHttpContext(Bundle bundle) {
Copy link
Contributor

@kaikreuzer kaikreuzer Sep 10, 2018

Choose a reason for hiding this comment

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

Comments? (see #6034 (comment))

*
* @author Łukasz Dywicki - Initial contribution and API
*/
public abstract class BaseSmartHomeServlet extends HttpServlet {
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Instead of providing abstract base classes that need to be extended by everyone, wouldn't it be nicer to follow the whiteboard pattern and only have those implement a "SmartHomeServlet" interface, while the framework provides a service that collects those and registers them on the http service? From the past we have some bad experiences with using base classes as an API and thus would want to go for proper interfaces whereever possible. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Difficulty with whiteboard is marrying everything together - you register things using SmartHomeServlet interface. In order to bind it with http service you need to make explicit cast to HttpServlet. It would be much better if we would have SmartHomeServletProvider, because then we avoid casts, however this interface would be a factory and updates of it will become harder.

We will still need to do binding of http context, the only difference is that we will do that centrally in whiteboard extender. Beyond that we will need to fetch servlet path (probably one of methods to be added in interface).
I understand your argumentation and motivation, however I wouldn't like to push that change over with this PR to do not mix things. It is definitely an improvement to above base class, however if we want to stick with this approach we need to decide what goes first - auth or whiteboard.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, let's not let the scope explode and leave the whiteboard refactoring for later. It just didn't look nice to have so many places in the code now extending a new abstract class although we do not want to use this pattern.

Import-Package:
com.eclipsesource.jaxrs.provider.security,
com.eclipsesource.jaxrs.publisher;version="5.3.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

remove version constraint

@kaikreuzer kaikreuzer added the CQ label Oct 2, 2018
@marziman
Copy link

marziman commented Oct 2, 2018

If this is merged, I want a big coffee from @kaikreuzer and @splatch for always pushing it forward 😁

@kirantpatil
Copy link

@marziman, please let me know where to buy one for you.

@kaikreuzer
Copy link
Contributor

I want a big coffee from @kaikreuzer and @splatch for always pushing it forward

@marziman First time you appear here on this PR and you want kudos? I think you should rather spend a round of beer for Lukasz and myself, who spent endless nights on coding, reviewing and testing!

@marziman
Copy link

marziman commented Oct 3, 2018

Hmm @kaikreuzer, I think you missestimate the efforts behind the scenes. That things go forward and becomes a contribution. We already have this feature running for a longer time and it would stuck without our push. So no beer this time..

I had endless discussions and decissions, which are the most difficult part. Implementing goes than much easier, same for testing. But I think you should know that..

@marziman
Copy link

marziman commented Oct 3, 2018

I dont see it as a fair argument either, cause all the meetings and discussions in the RBAC issue are blended out.But ask @splatch he knows how much dnergy went in from my side. So his kudos are sufficient for us.

PS:
Our team took the branch when it was initial and tested it, to give Lukasz the confidence to create the updated PR. Meaning we have tested the impl. as one of the first and made some feedbacks to him. At that time you didnt know that there is any activity at all 😉

@splatch
Copy link
Contributor Author

splatch commented Oct 3, 2018

Gentleman, there is no need to make such arguments. All of you did outstanding job on various fronts to let this happen. Kudos belongs to all of you who supported this work.

I believe that Kai wanted to bring some fun with his comment and get a free beer as well (who wouldn't like to get one!?).

I confirm that there was an unofficial review and test phase in July. It happened two weeks before submitting this PR and was provided by @svilenvul with Mehmet push for it. Svilen reported several issues (see Code-House#1, Code-House#2, Code-House#3, Code-House#4, Code-House#5) which I've fixed in early August.

There was no big calls with all parties, as there was very little to debate. Plumbing work had to be done anyway, and it could be verified and discussed during review as it happened. In terms of credits we also need to mention @antoniosv who worked with me earlier this year, provided some code, and tested several things related to JWT. His help and testing also moved us forward.

On bright sides, for sure there is much more work on this topic in front of us!

Cheers,
Lukasz

@lolodomo
Copy link
Contributor

lolodomo commented Oct 3, 2018

Could we have a short description of what feature this PR brings to the final user ?
Does it introduce an authentication to connect to ESH/openHAB ? In this case, how and where do we define the authorized accounts ?

@splatch
Copy link
Contributor Author

splatch commented Oct 3, 2018

Could we have a short description of what feature this PR brings to the final user ?

Within few bundles/features provided within this PR you will be able to use HTTP Basic authentication to secure your installation. There is no authorization support yet (that's why this PR is called part one). In later steps we need to provide JWT handling on both - server and client side.

Does it introduce an authentication to connect to ESH/openHAB ? In this case, how and where do we define the authorized accounts ?

Without diving to much into details - you need to:

  • property realmName=karaf inside org.eclipse.smarthome.jaas configuration to use the same user/password as you do to access openhab/esh console.
  • property authentication.enabled=true inside org.eclipse.smarthome.auth configuration
  • install feature name esh-io-transport-http-auth-basic

First step defines how you validate user/password information.
Second step enables security.
Third one adds support to extract user/pw from incoming http request via basic auth (browser prompt).

Once authenticated you will have access to everything, otherwise you should see nothing.

Kind regards,
Lukasz

@kirantpatil
Copy link

@splatch, do we have web interface to create, modify and delete users ?

@splatch
Copy link
Contributor Author

splatch commented Oct 3, 2018

@splatch, do we have web interface to create, modify and delete users ?

Not really, unless you write your own AuthenticationProvider. For openhab installations once this change get merged and configuration applied - you can use users.properties file.

@kaikreuzer
Copy link
Contributor

kaikreuzer commented Oct 3, 2018

I believe that Kai wanted to bring some fun with his comment and get a free beer as well

Definitely. I didn't mean to offend anyone, nor to downgrade any contribution.

Nonetheless, I'd like to remind everyone of the rules of open source engagement: Contributions and discussions have to be public within this project, that's all that counts when it comes to attribution.

I confirm that there was an unofficial review and test phase in July.

The much better way would have been a WIP PR at that time already - @svilenvul could have provided his feedback as a review on that PR.

Svilen reported several issues which I've fixed in early August.

Sorry, but that's a good example how NOT to do open source: Those issues have no comments, they do not directly reference any code/commit, they are still open and their status is completely unknown. Again, please do WIP PRs in future instead, that would make much more sense for everyone.

Could we have a short description of what feature this PR brings to the final user ?

@lolodomo In short, it does not bring anything to the final user as it is only a framework feature that can potentially be used and integrated into ESH solutions. So once this is merged, a next step for me would be the integration in openHAB - I briefly tested this already, but still had issues, so it can still take a while before anything becomes available to openHAB users. But for this, let us open an issue at openhab-distro instead for further discussion.

@marziman
Copy link

marziman commented Oct 3, 2018

@kaikreuzer,

I think the people know well how OSS works. The things we implement is not intended to be opensource atm. So went the right way as we made issues in the github of @splatch, where the current state was (visible to the public also).

So I think it is not about anyone getting kudos. It is about accepting the fact that it is more than just submitting a PR.
Again, we pushed this stuff and that fact shouldnt be made smaller as it was.

I think instead of trying OSS process dogma, lets stay at the core of that discussion: Respecting all involved peoples work.

BR Mehmet

@kaikreuzer
Copy link
Contributor

The things we implement is not intended to be opensource atm.

Is this PR a part of the work that "we" (i.e. you, whoever you exactly refer to here) are working on?

Just to be clear: If there is anyone but @splatch that had a significant share of the contribution of this PR, I will have to withdraw the CQ immediately as this isn't correctly stated.

@splatch
Copy link
Contributor Author

splatch commented Oct 3, 2018

Just to be clear: If there is anyone but @splatch that had a significant share of the contribution of this PR, I will have to withdraw the CQ immediately as this isn't correctly stated.

Entire code contained within this PR was written by me and no one else. In terms of code there are no contributions from other people.

@kaikreuzer kaikreuzer removed the CQ label Oct 6, 2018
@kaikreuzer
Copy link
Contributor

Good news, everyone, the CQ has already been approved for check-in!
I'll do a new ESH stable soon, so we can then see how to get this working in openHAB for a start.
At the same time, we should start working on the "part 2", i.e. OAuth2 & JWT support as discussed.

@kaikreuzer kaikreuzer merged commit 145701a into eclipse-archived:master Oct 6, 2018
@kaikreuzer
Copy link
Contributor

so we can then see how to get this working in openHAB for a start.

I have created openhab/openhab-core#412 for this.

htreu pushed a commit to htreu/smarthome that referenced this pull request Oct 11, 2018
The successor of this bundle is `org.eclipse.smarthome.io.http.auth.basic`.

Related to eclipse-archived#6034.

Signed-off-by: Henning Treu <henning.treu@telekom.de>
kaikreuzer pushed a commit that referenced this pull request Oct 11, 2018
The successor of this bundle is `org.eclipse.smarthome.io.http.auth.basic`.

Related to #6034.

Signed-off-by: Henning Treu <henning.treu@telekom.de>
@htreu htreu added this to the 0.10.0 milestone Oct 30, 2018
@kirantpatil
Copy link

May I know how to access basic auth using API ?

@splatch
Copy link
Contributor Author

splatch commented Jan 1, 2019

May I know how to access basic auth using API ?

@kirantpatil You need to install appropriate feature and then turn on authentication (org.eclipse.smarthome.auth.cfg with authentication.enabled=true). Second step is enabling of AuthenticationProvider who will verify username/password combination - this is yet another step which is specific to product you use.
If you are interested in this topic in context of openhab, please start forum thread at http://community.openhab.org/.

Cheers,
Łukasz

@lblabr
Copy link

lblabr commented Jul 13, 2019

is there already a thread in community, i can't find one related to this PR, could somebody give me a hint?

thanks in advance

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants