[FIX] Administrative functions not secured

• Key: JS1-421
• Type: Improvement
• Status: Resolved
• Resolution: Fixed
• Priority: Major
• Assignee: Mark Orciuch
• Reporter: Olaf Romanski
• Component/s: Security
• Affects Version/s: 1.4b5-dev / CVS
• Fix Version/s: 1.5

Environment

Operating System: Windows NT/2K
Platform: PC

Description

Here is what I do (using nightly build from 09.09.2003):
1. Create a new user (initially has USER role only)
2. Log on to Jetspeed with that user's name
3. Enter one of the following URL's into my browser:

http://localhost:8080/jetspeed/portal/template/Home/template/Home?
action=portlets.PortletUpdateAction&eventSubmit_doDelete=true&portlet_name=portl
et_to_be_deleted

and

http://localhost:8080/jetspeed/portal/template/Home/template/Home?
action=portlets.security.PermissionUpdateAction&eventSubmit_doInsert=true&name=i
nserted_permission_name

Result is:
Having only USER role I deleted portlet 'portlet_to_be_deleted' from portlet
registry and added new permission 'inserted_permission_name'
Should be:
Some message about unauthorized access attempt should be displayed, or at least
protected resources should not be modified.

Comments

[David Sean Taylor]

Yes this is a bad 'feature' of Turbine: no actions are secured.
So if you want to secure your actions, you need to write the security check in your action.
Its a pretty easy security check, but I haven't done it because it 'hard-codes' the security.
What if someone wanted to have another role, say "super-user" who also could access the admin
portlets.

Here's a simple solution:

Add a configurable property to JetspeedSecurity.properties:

services.JetspeedSecurity.admin.roles = admin

And then check for this role in all of the administrative actions

[Jeremy Ford]

How does this sound? We create new abstract actions such as
SecureGenericMVCAction and others as needed (SecureVelocityPortletAction,...).
These actions would override the doPerform/fireEvents methods by first
performing the admin check algorithm mentioned by David. If the security check
passes, then execution proceeds as normal. Otherwise, a message is logged.
From this point on, I'm unsure what course to take. Right now, I'm thinking
that maybe we could set the template to some generic error page, or a specific
template. Ideas and suggestions are welcome.

[Mark Orciuch]

IMO this is important and should be revisited again. I'm fine with Jeremy's
proposal. Anyone else has comments?

[Mark Orciuch]

Just started revisiting this problem again. It seems that the problem could be corrected within PortletActionEvent.executeEvents method. If the portlet cannot be retrieved from context, the event should not occur. If portlet can be retrieved from context, the security can be checked and event can be allowed or disallowed.

The problem I see is that many portlets (including admin portlets)execute the action events from the form rather than allowing the portlet to execute its own actions. If this is the case, Turbine processes the action before Jetspeed gets a chance to validate it. I don't think this the correct behaviour. A form based portlet should not define an action - it should allow Jetspeed to execute the action defined in registry. Otherwise, what is the purpose of declaring portlet action in registry?

I'd like to solve this problem once and for all so any feedback will be greatly appreciated.

[Mark Orciuch]

Well, upon review of what needs to be changed in order to implement my solution, it would be more time than what I'm willing to invest right now :-( All the admin portlets would have to be modified - I thought I could do it all in one place and still keep it backward compatible.

I will go with the solution documented proposed by Jeremy and David (i.e. create a SecureVelocityPortletAction, SecureJspPortletAction and SecureGenericMVCAction). Only people with role specified in the 'services.JetspeedSecurity.admin.roles' property will have to ability to do administration. If the property is empty, access will not be blocked. The default value will be 'admin' as you suggested.

[Mark Orciuch]

Change implemented and committed. Who's going to close this issue?

[Mark Orciuch]

Fix tested and committed.