peholmst / vaadin4spring

Vaadin integration for Spring and Spring Boot
Apache License 2.0
266 stars 131 forks source link

Add secured single-UI support #176

Closed gjrtimmer closed 9 years ago

gjrtimmer commented 9 years ago

Add support for a secured single-UI application.

Possible solution

Provide a SecuredNavigator like the sample of markoradinovic to support a single-ui security application which is secured with spring-security.

gjrtimmer commented 9 years ago

Progress 50% DONE.

gjrtimmer commented 9 years ago

Progress slowing down, need to rewrite the SpringViewProvider to allow more pre-checks for secured views so the new SecuredNavigator can handle the view request accordingly.

gjrtimmer commented 9 years ago

Hit a wall, breaking it down slowly.

gjrtimmer commented 9 years ago

Done

gjrtimmer commented 9 years ago

@peholmst could you please checkout branch dev/security and look at the new security-sample-single and look at the changes I've made.

Checkout the massive security upgrade, I've also made some changes to the SpringViewProvider to work with the new SecuredNavigator and the new SecuredUI.

Hope you like them.

gjrtimmer commented 9 years ago

TODO Add SpringErrorHandler to SecuredUI to handle AccessDeniedException

peholmst commented 9 years ago

@GJRTimmer I've now have had a look at your security changes and especially your sample applications. You've certainly put some effort into this, which is highly appreciated. That said, I do have some comments.

The problem I see with the single-UI approach, is that it seems to rely a lot on views and the Navigation API, and forces the user to use them in a way that at least I myself have never done.

In a single-UI application, this is how we do security manually:

Personally, I'd like to be able to code like this with Vaadin4Spring and still have good security integration.

As for the multi-UI example, in a multi-UI application, the login UI would not need to rely on the navigation API at all since it makes the application unnecessarily complex. The login UI can set its content to the login form UI directly and then redirect to the main UI once authentication is complete.

I think we still need to think a little bit about what is the recommended way of using security in a typical Vaadin4Spring application. I feel we're on the right track, but we're not really there yet. Once that is done, we need to think about how we can make this recommended way as easy to use as possible.

gjrtimmer commented 9 years ago

@peholmst You are right, and I agree with you. The single-ui setup I've created might not be the best way to go. As you said earlier we need to think about what the best direction is we should pursue.

What I think we definitely need is some way to detect and handle the Spring AccessDeniedException. Within markoradinovic he has done it by creating a SpringErrorHandler to catch the AccessDeniedHandler. This errorhandler is set as the UI ErrorHandler.

We somehow need to create something similar which does not only catch the exception but also allows some kind of easy handling of it, for example you want to direct the user to either a login page or a page which shows forbidden or access denied.

Any suggestion ?

peholmst commented 9 years ago

I need to think about this. We need to approach it from both the POV of a view based application and the POV of a single view application (that does not use the navigation API at all). We also need to decide how much we are going to provide out of the box and how much the developers are required to set up themselves.

peholmst commented 9 years ago

So the situations where we might end up with an AccessDeniedException are when we invoke a protected service method in the backend, or try to navigate to a protected view? Are there any other cases?

gjrtimmer commented 9 years ago

Both cases are correct, when you try to access a protected service / method on the backend and navigating to a protected view.

Other cases are:

peholmst commented 9 years ago

If we are building a single UI application, all session problems should result in the UI being reloaded, since that's the only way to recover as far as I know. As for the Remember Me token, I assume this is something that should also be checked by the UI's init method, where the UI decides whether to show the login screen or the main screen.

IMO, if you try to access a protected service and get an access denied message, you should not be redirected anywhere. Instead, a message should pop up, informing the user what has happened. In a well written application, this should never happen since the developer should make sure that forbidden actions are not even available in the UI in the first place.

As for protected views: what do you think is better - inform the user that the view exists, but that access has been denied, or behave like the view did not exist in the first place?

gjrtimmer commented 9 years ago

About rememberme: When spring finds a valid rememberme token, it authenticates the user based upon the token. You should indeed check if the user is authenticated within the init method and then redirect.

You said that when accessing a protected service you should not be redirected, I disagree, when a user loads a URL from his bookmarks but is no longer authenticated, then he should be redirected to the login. After successful login the user should be redirected to this original requested URL. Like i've made with the multi-UI example with the SavedRequestAware filter.

About protected views, I like the idea that the page does exist but is forbidden and the user sess / knows this, at least the user knows he has the correct url while if you maintain the concept of the current SpringViewProvider and on access denied you pretend the view does not exist at all, you might give the end user the idea that he is dealing with a malfunctioning webapplication.

peholmst commented 9 years ago

As for the redirection upon access denied when accessing a service, I'd like the application to know the difference between access denied as in "you have not authenticated and need to login" and as in "you are authenticated, but your permissions are not sufficient to access this service".

In the first case, you should be redirected to the login screen and I think this should happen automatically since the UI's init() method needs to be called anyway. In the second case, you should receive an error message.

gjrtimmer commented 9 years ago

Agreed.

In the first case you should be redirected to the login, after which to return to your original request, and in the second case you should be given an error, because even while signing in you cannot magically get more access right.

peholmst commented 9 years ago

I think we should build a single UI application that showcases all these features, and then try to make the framework support it.

gjrtimmer commented 9 years ago

Good plan, do you want to use the dev/security branch as a initial setup or start from scratch, I think maybe starting from scratch is not a bad idea.

shall we create a new branch for this ?

peholmst commented 9 years ago

Let's create a new branch, and start with the demo applications.

gjrtimmer commented 9 years ago

Good plan,

If I could make a suggestion. Please install nodeJs, npm, bower, and create a empty JHister project. I always borrow a lot from them, and their empty project is I think a good sample of what we should be aiming for concerning our demo applications. The integrate very good with Spring and have a lot a default spring class we could use.

peholmst commented 9 years ago

I'll have a look. I'm currently busy with some other projects, so if you find the time, feel free to create the branch and start on the demo app yourself (but put a comment about it here so that we don't do overlapping work in case I also find some free time).

gjrtimmer commented 9 years ago

branch feature/demo created

peholmst commented 9 years ago

Thanks.

gjrtimmer commented 9 years ago

@peholmst Full demo application framework provided within branch feature/demo, project setup based upon official vaadin multi-module project creation.

I have not created security and database configuration; this we add later I think maybe you can create a nice mainview or maybe you have something lying around which we can reuse ?

Also some of the classes come from the JHipster project. Don't forget to read the README about how to start it, I probably don't have to tell you because you and your team have written the README of the official project plugin.

peholmst commented 9 years ago

@GJRTimmer The demo project setup looks like it could be very useful in a real world application, but for the sake of demonstrating Vaadin4Spring it is IMO a huge overkill. There are so much stuff in there that does not have anything to do with Vaadin, that you risk getting lost in the details. IMO the single UI demo application should consist of:

At some point it would probably be a good idea to build a bigger demo application, but I think that would be a completely different project, in a different repository.

peholmst commented 9 years ago

As for the view provider, I suggest the following:

This way there is no need to customize the Navigator itself.

gjrtimmer commented 9 years ago

@peholmst Good idea, I will look into it.

peholmst commented 9 years ago

@GJRTimmer I'm currently working on the Access Denied view. Should have it done later today if no problems show up.

peholmst commented 9 years ago

@GJRTimmer Support for Access Denied view added in commit [9830d6f].

gjrtimmer commented 9 years ago

I've finally got the full rememberme functionality working, including with a AuditEvent handling which stores the token into a database to allow rememberme with application restart.

It's fully working, however changing my setup to a sample which can be used within the project samples will take time. It;s a full Vaadin+Spring-Security+Spring-LDAP+MySQL project. So it could take more time then I currently have available.

I will try to push a working remember sample as soon as possible.

About the remember me and the UI init(). Currently I've used the fully functionality of SpringSecurityFilterChain. This means that when the RememberMeAuthenticationFilter detects a remember me cookie. It will authenticate the user, populates the SecurityContext and simply continues the chain. This all happens before the UI init() is called.

So we have to think about how to implement the RememberMe functionality.

peholmst commented 9 years ago

Isn't the remember me authentication supposed to happen before UI.init()? Then, in UI.init(), you'd have something like this:

if (mySecurityHelper.isAuthenticated()) {
  setContent(myMainScreen);
} else {
  setContent(myLoginScreen);
}
gjrtimmer commented 9 years ago

The Remembe Me authentication is supposed to happen before you reach the init() the attached rememberme token with the request can be expired, or maybe there was a theft attack on the request.

The RememberMeAuthenticationFilter is then one that should be responsible for the rememberme authentication. When all is correct the rememberme authentication filter populates the SecurityContext so that the user will be authenticated.

RememberMe is attached to the request by a cookie in default, I don't think from a security point a view it's something you want to take over with the UI init(). You open a can of worms, handling sessions attacks, session expiration, cookie expiration, incorrect token(s) invalidated tokens etc..

When you finally reach the the init() method, and call .isAuthenticated() if the rememberme authentication went successful, it will return true. If it failed you can redirect the user to a login screen or have the SecurityFilterChain redirect the user to a login screen, like your provided sample.

I will try to strip the demo from all unnecessary classes and strip it down to a simple demo, which we can use to create a single application demo.

peholmst commented 9 years ago

Now I don't really follow you, why would you even want to take over RememberMe in the UI.init()? I've never suggested anything like that.

gjrtimmer commented 9 years ago

@peholmst Sorry wrong interpretation on my end. I suggest we try to get a working demo first.

peholmst commented 9 years ago

No worries. Getting a working demo sounds like a plan.