Parameter validation

[astappiev]

In GitLab by @PhilippKemkes on Jun 23, 2020, 16:49

All pages must be checked to handle invalid parameters correctly.
That means showing appropriate error messages for missing and invalid parameters. Parameters can be invalid e.g. if they are empty, contain characters instead of numbers, or refer to the id of an deleted item.

The f:viewParam required="true" attribute adds a FacesMessage when the parameter is missing. But this message isn't shown any more. Hence it shall be replaced by validation checks in the Bean.

To be discussed: Check also access permissions and remove hasAccessPermission template paramater through an BeanAssert call.

The task is devided by folders:

• admin
• dashboard
• eumade4all
• group
• myhome
• public
• survey
• user
• your_information
• ROOT

[astappiev]

In GitLab by @PhilippKemkes on Jun 23, 2020, 16:53

Two known special cases:

The group/resource page is used also as users private resources page.

I propose to use templating to create different pages for these two purposes. Then the group_id parameters shall be required on the group/resource page and be ignored on the myhome/resource page.

A similar problem exists for the admin/organisation page which is also used by moderators.

For this page I propse to add the organization id explicitly for all moderator links to this page.

[astappiev]

In GitLab by @PhilippKemkes on Jun 23, 2020, 16:54

mentioned in commit 928772929756dace1e8a619d902432f3ddf50159

[astappiev]

Check also access permissions and remove hasAccessPermission template paramater through an BeanAssert call.

I'm now yet sure about it, I like the idea that all pages are requiring authorization by default and the fact that access level is controlled via a single line.
Let's wait for some time, maybe a better idea will come.

[astappiev]

In GitLab by @PhilippKemkes on Jun 29, 2020, 13:15

access level is controlled via a single line.

I thought this too. But most pages will anyway require more fine grained permissions. Which are not easy to check in the bean. At the moment mostly public and moderator pages use the attribute. For consistency I think it is better to remove it and check permissions only in the beans.

I'm also asking my self if error 400 is appropriate how we use it. It is more common to use error 404. Even if this isn't correct for most cases.

I propose:

• resource_id=abc -> error 400
• resource_id=1000000000 (to big not used yet) -> error 404
• resource_id=12345 (resource deleted) -> error 410

The managers' get method could throw EntryNotFound and EntryDeleted exceptions. Then there would be no need the check this in the beans' onload methods. But this way we can't use specific error messages for resources/groups/....

What do you think?

[astappiev]

I thought this too. But most pages will anyway require more fine grained permissions. Which are not easy to check in the bean. At the moment mostly public and moderator pages use the attribute. For consistency I think it is better to remove it and check permissions only in the beans.

I'm fine with it, but old approach when all pages by default required authorization, I liked more (no one forgets to add this check).
But on the other side, we do not add new pages every day and it can be controlled.

As migration solution, we can use a method from userBean (example below) instead of hasAccessPermission param, so we don't need to create new beans when they not needed:

<ui:define name="metadata">
   <f:metadata>
      <f:viewAction action="#{userBean.checkAccessPermission(userBean.moderator)}" />
   </f:metadata>
</ui:define>

I also wanted to finish implementation of maintenance mode, when it is enabled we should throw an error and show error page accordingly. The method which checks if maintenance is active should be added to all pages (it also can check if user is !admin and more). This can be combined with hasAccessPermission, and checks for unauthorized users.

[astappiev]

I'm also asking my self if error 400 is appropriate how we use it. It is more common to use error 404. Even if this isn't correct for most cases.

This is how it suppose to be, I'm glad to implement that.

The managers' get method could throw EntryNotFound and EntryDeleted exceptions. Then there would be no need the check this in the beans' onload methods.

Sure, good idea.

But this way we can't use specific error messages for resources/groups/....

Why not? We can define different messages for eceptions thrown in GroupManager, ResourceManager, etc.

[astappiev]

In GitLab by @PhilippKemkes on Jun 29, 2020, 15:16

I also wanted to finish implementation of maintenance mode, when it is enabled we should throw an error and show error page accordingly. The method which checks if maintenance is active should be added to all pages (it also can check if user is !admin and more). This can be combined with hasAccessPermission, and checks for unauthorized users.

Very few pages like the refactored YourInformation page use no bean. For those this solution is fine.
I think all other pages can perform the permission check in their constructor if they have no onLoad method.

Why not? We can define different messages for eceptions thrown in GroupManager, ResourceManager, etc.

I thought this wouldn't be ideal. It should be human readable for developers like all exception messages. But optionally it should be translated in the frontend.
We would end up with something like

new EntryNotFoundException("Resource not found"); // do not change the message because it is used as key for the language bundle

But it's better than nothing.

[astappiev]

You don't care when you see keys in xhtml files, why do you care to see them in Java?

I'm totally fine to have new EntryNotFoundException("error.not_found_resource_description"); somewhere in ResourceManager.

I can ctrl+click to see the translation if I need.

[astappiev]

In GitLab by @PhilippKemkes on Jun 30, 2020, 16:12

https://www.ibm.com/developerworks/library/j-dao/

• DAO methods should throw meaningful exceptions.
• DAO methods should not throw java.lang.Exception. A java.lang.Exception is too generic. It does not convey any information about the underlying problem.
• DAO methods should not throw java.sql.SQLException. SQLException is a low-level JDBC exception. A DAO should strive to encapsulate JDBC rather than expose JDBC to the rest of the application.
• Methods in the DAO interface should throw checked exceptions only if the caller can reasonably be expected to handle the exception. If the caller won't be able to handle the exception in a meaningful way, consider throwing an unchecked (run-time) exception.
• Use chained exceptions to translate low-level exceptions into high-level ones.
• Consider defining standard DAO exception classes. The Spring Framework (see Related topics) provides an excellent set of predefined DAO exception classes.

They prefer RuntimeExceptions for DAOs.

Besides the user logs I can't imagine a case when we ever request a deleted resource intentionally. There we would need to catch our own NotFoundException. Everywhere else we could ignore it.

Wrapping SqlExceptions into our own runtimeException would also avoid forwarding all the SqlExceptions .

That would be fine with me.

[astappiev]

Some details how it is implemented in Spring Framework: https://www.baeldung.com/spring-response-status-exception

They have ResponseStatusException(HttpStatus status, java.lang.String reason) which is RuntimeException similar to what I already added.
They also have an option to assign error code to any user-created exception.

[astappiev]

The java.lang.Exception should never be thrown, I agree.
Regarding SQLException, we can wrap it into something else (what will help to remove many exceptions from beans), but generally, if it is thrown and not caught, we will display 500 error page (which is also fine).

[astappiev]

In GitLab by @PhilippKemkes on Jun 30, 2020, 21:21

Ok then please go ahead. I've assigned the issue to you.

Have in mind that I hard-delete soft-deleted resources from time to time.

Hence if a resource can't be found in the database. We have to distinguish by ID if the resource was deleted (GONE) or didn't existed yet (NOT-FOUND)

select auto_increment from information_schema.TABLES where TABLE_NAME ='lw_resource' and TABLE_SCHEMA='learnweb_main'