Feature Requests item #1438745, was opened at 2006-02-25 21:16
Message generated for change (Comment added) made by eelco12
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=684978&aid=1438745&group_id=119783
Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: 1.2
Status: Closed
Priority: 5
Submitted By: Gili Tzabari (cowwoc)
Assigned to: Nobody/Anonymous (nobody)
Summary: New auth-roles-class library with Class-based roles
Initial Comment:
Since Wicket plans to migrate to Java5 (and in fact
most of the roles package uses Java5 code, doesn't it?)
I suggest dumping
wicket.authorization.strategies.role.Roles in favor of:
1) enum Roles (defined by the user)
For example:
enum Roles
{
USER,
ADMIN
}
2) Set<Roles> to replace Wicket's Roles class
Set already defines contains() and containsAll() which
fully cover all the functionality provided by the
Wicket Roles class -- but in a type-safe manner.
I would recommend simply showing an example of this in
the wicket-auth-roles-examples but not having any
formal code in wicket-auth-roles. I think that Wicket
should not predefine roles such as USER, ADMIN as this
is very subjective and belongs completely in the user
code. For example, I have:
enum Roles
{
GUEST,
BASIC_USER,
PREMIUM_USER,
ADMINISTRATOR
}
----------------------------------------------------------------------
Comment By: Eelco Hillenius (eelco12)
Date: 2006-02-27 01:18
Message:
Logged In: YES
user_id=820266
Just a plain 'NO'. You are welcome to start your own project
- in which case you don't have to open a feature request.
----------------------------------------------------------------------
Comment By: Gili Tzabari (cowwoc)
Date: 2006-02-27 01:15
Message:
Logged In: YES
user_id=80628
What about my last post? What about CGLIB and/or a new
library side-by-side the existing one?
If you're using Spring, you have CGLIB. And anyway I don't
see anyone using property files to store
username/password/role in the long run. I would expect this
sort of stuff to be stored in the database.
----------------------------------------------------------------------
Comment By: Jonathan Locke (jonathanlocke)
Date: 2006-02-27 01:10
Message:
Logged In: YES
user_id=486414
Ah, so it was just me who missed that... Your decision
makes sense. I have no interest in taking this on. It just
doesn't matter that much...
----------------------------------------------------------------------
Comment By: Gili Tzabari (cowwoc)
Date: 2006-02-27 01:09
Message:
Logged In: YES
user_id=80628
Jon,
You are right. I have talked with Igor and I already have
some changes planned. My previous implementation mistakely
had User implementing Role. My next update will use
User.getRole() instead -- so EverybodyWithId would go away
altogether. If you want, we could rename EVERYONE to
something else. It is simply a name I borrowed from Windows'
security model. It represents the role a user gets if he has
not yet logged in.
I need Role to be an interface in order to allow multiple
inheritance of interfaces. So you could have interface
AdminRole extends User1Role, User2Role. AbstractRole is just
the abstract class a concrete Role implementation would extend.
Eelco,
I believe we can make it work for Strings using CBLIB. Would
that solve your objection? Otherwise, we can redefine this
RFE against a new wicket auth-roles package (side-by-side to
your own) so we don't have to keep on closing it. I think
there is interest in this kind of library (Igor for one).
----------------------------------------------------------------------
Comment By: Eelco Hillenius (eelco12)
Date: 2006-02-27 01:00
Message:
Logged In: YES
user_id=820266
I didn't miss that classes can be used in annotations. I
just think it is a bad idea. Classes are inflexible and a
bad match for getting these things from configuration/ database.
I am again closing this request. People are welcome to start
their own projects to implement this idea. Or if Jonathan/
Igor want to take this project on you that's fine too. I'm
not going to spend one more minute on it.
----------------------------------------------------------------------
Comment By: Jonathan Locke (jonathanlocke)
Date: 2006-02-27 00:54
Message:
Logged In: YES
user_id=486414
I missed that classes could be used in annotations somehow
and so did Eelco. That does seem a lot better. However,
you're going to need to make multiple classes possible since
you can authorize more than one role to instantiate a class.
The interface sounds like a mistake to me and I'm unsure
why you have AbstractRole AND Role. Also EVERYONE is not a
role. It is a set of users. ADMIN is a role. USER is a
role. I haven't looked at it, but EveryoneWithId sounds
like a mistake or misnaming. User would be the generic
role. Everyone is meaningless since it is not a role.
----------------------------------------------------------------------
Comment By: Eelco Hillenius (eelco12)
Date: 2006-02-27 00:51
Message:
Logged In: YES
user_id=820266
I'm closing this one. Because:
- I started that project primarily to be a simple
implementation with the primary goal of having *some*
implementation that shows Wicket's authorization works and
is sufficiently flexible enough, and to have a handle for
users who want to implement their own implementations.
- Basing authorization on a class hierarchy would never be
my prefered way. It would be the first time I'd actually saw
that opposed to something like a generic User/ Role class,
and compared to strings it is way less flexible.
- Strings are a good match for configurations. You probably
gonna load your roles from a database or configuration file,
whether that is Spring, JAAS or roles in web.xml; it's all
string based.
- I spent way more time on this already. I state again: the
aim of this project is not to be the complete, ultimate
solution, but rather an example and good-for-basic-stuff
implementation. Sometimes it is okay to be happy enough with
something and move on to other areas that are in more need
of attention.
----------------------------------------------------------------------
Comment By: Gili Tzabari (cowwoc)
Date: 2006-02-26 23:05
Message:
Logged In: YES
user_id=80628
Patch version 3:
- Role.EVERYONE is now an interface. You can now do cool
things like:
interface UserType1 extends Role.EVERYONE{};
interface UserType2 extends Role.EVERYONE{};
interface ADMIN extends UserType1, UserType2{};
- Added serialVersionUID to all the Role interfaces.
- Fixed bug in EveryoneWithId.toString()
----------------------------------------------------------------------
Comment By: Gili Tzabari (cowwoc)
Date: 2006-02-26 22:44
Message:
Logged In: YES
user_id=80628
I've uploaded gili2.patch. There are a few new files which
are not bundled in the patch file. These are:
wicket-auth-roles/src/main/java/wicket/authorization/strategies/role/AbstractRole.java
wicket-auth-roles/src/main/java/wicket/authorization/strategies/role/Role.java
wicket-auth-roles-examples/src/main/java/wicket/authorization/strategies/role/example/Admin.java
wicket-auth-roles-examples/src/main/java/wicket/authorization/strategies/role/example/EveryoneWithId.java
you can grab a copy out of gili2.zip
----------------------------------------------------------------------
Comment By: Gili Tzabari (cowwoc)
Date: 2006-02-26 22:39
Message:
Logged In: YES
user_id=80628
I don't understand some of the points you mentioned:
1) What do you mean by "no package in annotations"? I see no
mention of this in the documentation:
http://java.sun.com/j2se/1.5.0/docs/guide/language/annotations.html
2) Users will not have to say:
@AuthorizeInstantiation("com.whatever.foo.MyRole")
instead they will say:
@AuthorizeInstantiation(MyRole.class)
where MyRole.class is in the import list at the top of the
Java file. It's very easy thanks to the import wizard in
your favorite IDE :)
3) In the third design (which I'm uploading now) there is no
need to register your roles and there is no chance of a
namespace collision because as I mentioned before a Role is
a Class and you can't have two Classes with the same name in
the same namespace (thank you Mr. compiler).
Anyway, I really hope you get a chance to check out the
updated examples because it looks quite amazing and really
gets the point across. I'm attaching gili2.zip which is a
drop-in replacement/snapshot of the old code and gili2.patch
which is a patch for that same snapshot should you wish to
commit my changes.
----------------------------------------------------------------------
Comment By: Jonathan Locke (jonathanlocke)
Date: 2006-02-26 21:53
Message:
Logged In: YES
user_id=486414
Okay, could work. But I don't have time to review. A few
quick thoughts:
- Make Role an immutable, final, singleton token that is
essentially a stand-in for String with the inheritance
feature you suggested, but don't close the door to adding
functionality to the object in the future.
- Since roles will be named but not namespaced (no package)
in annotations, you will need to register names with
Application settings. Maybe ISecuritySettings.getRoles()
would give you a Roles object containing all the roles in
the application. To add a role,
getApplication().getSecuritySettings().getRoles().add(role)
- Role.equals should compare the short name of the class
and not the fully qualified name (since annotations won't
use the fully qualified name... nobody wants to say
@AuthorizeInstantiation("com.whatever.foo.MyRole") when they
could say just "MyRole". Also consider making roles
case-insensitive to make annotations more resilient. "user"
ought to be equivalent to "User" in annotations.
- Have a method on Roles that causes the role set to throw
an exception if more than one Role is added with the same
name. Call this method with true on the security settings
object to prevent multiple roles with the same short name
being added to the application. Whatever you do, do NOT
store Roles in a static map.
- In role annotations code, do
ISecuritySettings.getRoles().forName(String).
----------------------------------------------------------------------
Comment By: Gili Tzabari (cowwoc)
Date: 2006-02-26 21:29
Message:
Logged In: YES
user_id=80628
Jon,
Had you read my second post you would have seen I dropped
the idea of using Enums. Please take a look at the second
design. Secondly, I have a third revision coming up which
will be even more intuitive to use with inheritable roles.
For example:
Role Admin extends Role User
such that Admin.hasRole(User) returns true. If you give me a
couple of mins I will upload the third design and you can
review it yourself. Take a look at the updated examples
application to see what the new code looks like.
----------------------------------------------------------------------
Comment By: Jonathan Locke (jonathanlocke)
Date: 2006-02-26 21:22
Message:
Logged In: YES
user_id=486414
Yuck. Enum is not extensible. Roles already extends
HashSet. Roles MUST be implemented as it is because
annotations only support primitive types. We've already
been through ALL of this!!
----------------------------------------------------------------------
Comment By: Gili Tzabari (cowwoc)
Date: 2006-02-26 00:48
Message:
Logged In: YES
user_id=80628
Ok, I've replaced the type of a user role from String to
Class. There are a few benefits here:
1) We will get compile-time errors if we try referring to
non-existant roles (prevents typos).
2) Class is resolved relative to the Java namespace, which
means that we'll get compile-time errors if there is a
namespace collision. With String there was nothing to
prevent different role constants from mistakenly using the
same String.
I've also defined Roles.EVERYONE (the name is borrowed from
Windows' security groups) and Roles.NO_ONE which define a
Role which everyone and no-one has, respectively. Finally
I've also updated the wicket-auth-roles-examples.
I look forward to your feedback.
----------------------------------------------------------------------
Comment By: Gili Tzabari (cowwoc)
Date: 2006-02-25 23:25
Message:
Logged In: YES
user_id=80628
I also had a perfect design ready to upload, then I
discovered that one cannot extend enum... doh!
Anyway, I believe this means we cannot use enums (because
users can't add their own roles) but I'm working on an
alternative now. You can just me in IRC to discuss this
further. The next time I reply to this issue I will
hopefully either close it as INVALID (can't change anything)
or upload a patch of my proposed changes.
----------------------------------------------------------------------
Comment By: Johan Compagner (joco01)
Date: 2006-02-25 23:08
Message:
Logged In: YES
user_id=379231
i can be mistaken but aren't we then tied to those enums?
Currently you can do what every you want create any role you
want.
The user and admin strings are just handy i guess, don't
have to use.
----------------------------------------------------------------------
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=684978&aid=1438745&group_id=119783
