|
 Re: Continuation of previous note
(broadening the audience to include gallery-devel -- this is a continuation of a discussion that Tim and I are having around the branch to fix ticket #1273).
----
Tim,
I notice that you're not copying the edit permissions in the upgrader. Just to verify: we decide that was ok because "edit" is a superset of "edit_xxx" right? In this diagram:
IIRC, "edit_album" is the equivalent of what you have as "edit" right now. I wonder if we should actually rename the permission to be "Edit album" to avoid confusion there.
_copy_permissions is a PHP based O(N*M) operation where N==the number of rows and M== the number of groups. That's going to be really inefficient. Let's get that down to O(M) using the db builder. It'd be something like:
foreach ($groups as $group) { ... db::build() ->from($table) ->set($to_column, db::expr($from_column)) ->execute();
}
is your wrapper around _get_all_groups() really necessary? You're only calling that from the upgrader and when we get to that point there had better be an identity provider active!
More below. On Sat, Apr 30, 2011 at 6:11 PM, Tim Almdal <tnalmdal@...> wrote:
Just a little bit more information, I went and used user1 to add a photo into user2's album (the waterfall). So now there is a picture that user2 can't do anything about.
The other interesting thing is that user2 can now get the organize dialog and edit permission dialogs for their directory. Not sure of this is what we want.
How about we restrict organize to users who have the "edit" permission? That should be unambiguous.
My basic approach was rather than put all sorts of stuff all over the place to check edit permissions, I just changed access.php so that the interface accepts access::can("edit"...) and it figures out whether the user has edit, edit_all_photos or edit_my_photos.
I initially had a negative reaction to this idea, but I thought it through and it makes sense. There's an inconsistency that bothers me, though. Right now you have 3 perms "edit", "edit_all_photos" and "edit_my_photos". If you do access::can("edit") it's not obvious to the caller what's going on there because there is an edit permission. If you rename "edit" to "edit_album" (which is consistent) then "edit" doesn't directly map to a column and we can consider it an alias permission. We had those in G2, but they were heavyweight (using bit vectors, etc). This will just be a simple alias that you cover in access::can.
So then we can try to map this to the diagram we came up with:
For the remainder of this mail I'm going to assume that we rename edit->edit_album and use edit as an alias.
Going through the diagram, I'm convinced that your code is right for the root album (A1) and for the photos (P1, P2). However, I'm not sure that we have A2 correct. Right now your equality for A2 is:
edit(A2) == access::can("edit_album", A2) || (
access::can("edit_all_photos", A1) ||
( is_owner(A2) &&
access::can("edit_my_photos", A1)
) )
I'm not sure if that's right. If I grant a user the ability to create A2 under A1, I would imagine that they would start off with the edit_album permission -- but I could see that they wouldn't get that permission because they don't have edit_album on the parent so they wouldn't inherit it. But we still want them to be able to edit the album they just created, right? I guess that means that we really do need to treat sub-albums the same as sub-photos in that case.
Is that logic correct? If so, then we should rename:
- edit --> edit_album
- edit_all_photos --> edit_all
- edit_my_photos -> edit_mine
This makes our logic easier because we stop differentiating child albums vs. photos. To close the loop, I'm going to write it in pseudocode below so that I'm sure I have it right.
access::can("edit", X) ==
C1 access::can("edit_album", X) || /* X != album? it comes from parent */ C2 access::can("edit_all", X->parent) ||
( C3 access::can("edit_mine", X->parent) &&
is_owner(X) )
So that means the equalities are:
edit(A2) == edit_album(A1) || edit_all(A1) || (edit_mine(A1) && is_owner(A2)) edit(P2) == edit_album(A1) || edit_all(A1) || (edit_mine(A1) && is_owner(P2))
Scenarios:
S1: album creation: - Admin user grants only "view" and "add_album" to groups G1 (containing U1), G2 (U2) at the root album (A0)
- U1 creates album A1, which inherits permissions from root album. A1's permissions are now "view" and "add_album" for G1/G2. U1 cannot edit A1 even though they own it.
- Admin grants "edit_mine" to A0
- Now U1 can edit A1 because of permission clause C3 above.
- U2 creates album A2 and can immediately edit A2 because of step 3 and C3.
S2: photo addition (continues from scenario S1): - U1 creates photo P1A inside A1.
- U1 can edit P1A because of C1
- U2 cannot add photos to A1
- U1 grants "add_photo" to G2 on A1
- U2 now adds photo P1B to A2
- U2 cannot edit P1B because he has no edit permissions
- U1 grants "edit_mine" to G2 on A1
- U2 can now edit P1B but not P1A (** we're not talking about delete yet)
- U1 revokes "edit_mine" from G2 on A1
- U2 can no longer edit P1B
- U1 grants "edit_all" to G2 on A1
- U2 can now edit P1A (which he does not own) and P1B (which he owns)
Ok. I think I'm satisfied with that. One thing I noticed is that you did not implement the aliasing for group_can(). I don't think that we need to do that currently, but right now because you haven't changed the name of the "edit" permission anybody who calls group_can("edit") is going to get an unexpected result -- they're essentially getting group_can("edit_album"), not the alias. Once you rename the permission, it means that they'll get an error, which I think is a good thing since it'll force them to figure out what to do. A quick grep around gallery3 and gallery3-contrib indicates that nobody does this outside of the core code now, so this is a good time to do this.
So to sum up, here's what I think we need to do: - Rename edit -> edit_album
- Rename edit_all_photos -> edit_all
- Rename edit_my_photos -> edit_mine
-
Replace the _user_can_edit() logic with what I have above, and (I suggest) you inline it into user_can()
- Write up all the above scenarios as unit tests (!!!)
Whew! ------------------------------------------------------------------------------
WhatsUp Gold - Download Free Network Management Software
The most intuitive, comprehensive, and cost-effective network
management toolset available today. Delivers lowest initial
acquisition cost and overall TCO of any competing solution.
http://p.sf.net/sfu/whatsupgold-sd__[ g a l l e r y - d e v e l ]_________________________
[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]
|
|
Re: Continuation of previous note
And one more thing I forgot. Deleting the "add" permission is not backwards compatible. Anybody who calls access::can("add", $item) after this change is going to bomb. Looks like all of our themes do that as a way to put up the "Add some photos to this empty album!" message.
Possible solutions: - Let 'em break
- Create add_album, but leave add around in the code as an alias for add_photo
- Create add_album and add_photo and make add an alias for either
I'm leaning towards #2 above. Thoughts? ------------------------------------------------------------------------------
WhatsUp Gold - Download Free Network Management Software
The most intuitive, comprehensive, and cost-effective network
management toolset available today. Delivers lowest initial
acquisition cost and overall TCO of any competing solution.
http://p.sf.net/sfu/whatsupgold-sd__[ g a l l e r y - d e v e l ]_________________________
[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]
|
|
Re: Continuation of previous note
I'll give your suggestions a try.
I notice that you're not copying the edit permissions in the
upgrader. Just to verify: we decide that was ok because "edit" is a
superset of "edit_xxx" right?
No, it was my understanding that we didn't copy edit
permissions because I thought we had finally distilled everything
down to the bottom half of this http://gallery.menalto.com/gallery/doc-images/gcon2011/IMG_0162.JPG.html.
and that we felt that edit would work just as it did now. If the
administrator wanted to use the new permissions that they would
have to explicitly chose to enable them.
is your wrapper around _get_all_groups() really necessary?
using the helper _get_all_groups() was just from copying
code... your right i should be able to just call identity::groups.
How about we restrict organize to users who have the "edit"
permission? That should be unambiguous.
works for me
For the remainder of this mail I'm going to assume that we rename
edit->edit_album and use edit as an alias.
Not a safe assumption, at this point, as mentioned
above, I was not coding to (http://gallery.menalto.com/gallery/doc-images/gcon2011/IMG_0161.JPG.html)
but to http://gallery.menalto.com/gallery/doc-images/gcon2011/IMG_0162.JPG.html.
As you can see from that image, we had crossed out the rename of
edit. What's in a name, so it doesn't matter to me whether we
rename it or not.
I think that trying to extend or convert "edit/edit album"
permission on albums that are added by users is a fools errand and
destined to make things complicated. (We might have to mess
around with the inheritance portion of our existing permission
code). I think the way to do this is force users to have one of
("edit/edit album", edit_all, or edit_mine) permissions in
addition to the add_album or add_photo permissions. That way our
existing inheritance model works naturally. I really don't like
the idea (which I think you are suggesting) of when an album is
added we then create ("edit/edit album") permission for the album.
That's a lot of work and breaks the natural inheritance that we
have now.
What's currently in the code, allows inheritance of the
"add_photo" and "add_album" permissions without regard to the edit
permissions. For example you could create an album and I could
add sub-albums and photo's to your album even if I can't edit them
because the add_* permissions where inherited w/o regard to what
my edit permissions are. Tying them together with the add_*
permissions a sub-permission of the edit* permissions prevents
that blank inheritance.
This makes our logic easier because we stop
differentiating child albums vs. photos. To close the loop, I'm
going to write it in pseudocode below so that I'm sure I have it
right.
access::can("edit",
X) ==
C1
access::can("edit_album", X) || /* X != album? it comes from
parent */
C2 access::can("edit_all", X->parent) ||
(
C3 access::can("edit_mine", X->parent)
&&
is_owner(X)
)
That's what I believe I have implemented.
So in my mind,
1) Decide which diagram we are using (I'm assuming 162)
2) Clean up the installer code especially the _copy_permission code
as you suggested
3) Change the add_* permissions to also require an edit permission
(bury that check in access.php so no code has to change external to
access.php to enforce that)
4) Write some unit tests.
5) Generate the install SQL
And we should be good to go
Tim
On 5/1/2011 12:54 PM, Bharat Mediratta wrote:
(broadening the audience to include gallery-devel -- this is
a continuation of a discussion that Tim and I are having around
the branch to fix ticket #1273).
----
Tim,
I notice that you're not copying the edit permissions in the
upgrader. Just to verify: we decide that was ok because "edit"
is a superset of "edit_xxx" right? In this diagram:
IIRC, "edit_album" is the equivalent of what you have as
"edit" right now.. I wonder if we should actually rename the
permission to be "Edit album" to avoid confusion there.
_copy_permissions is a PHP based O(N*M) operation where
N==the number of rows and M== the number of groups. That's going
to be really inefficient. Let's get that down to O(M) using the
db builder. It'd be something like:
foreach ($groups as $group) {
...
db::build()
->from($table)
->set($to_column, db::expr($from_column))
->execute();
}
is your wrapper around _get_all_groups() really necessary?
You're only calling that from the upgrader and when we get to
that point there had better be an identity provider active!
More below.
On Sat, Apr 30, 2011 at 6:11 PM, Tim
Almdal <tnalmdal@...>
wrote:
Just a little bit more information, I went and used user1 to
add a photo into user2's album (the waterfall). So now there
is a picture that user2 can't do anything about.
The other interesting thing is that user2 can now get the
organize dialog and edit permission dialogs for their
directory. Not sure of this is what we want.
How about we restrict organize to users who have the "edit"
permission? That should be unambiguous.
My basic approach was rather than put all sorts of stuff all
over the place to check edit permissions, I just changed
access.php so that the interface accepts
access::can("edit"...) and it figures out whether the user has
edit, edit_all_photos or edit_my_photos.
I initially had a negative reaction to this idea, but I
thought it through and it makes sense. There's an
inconsistency that bothers me, though. Right now you have 3
perms "edit", "edit_all_photos" and "edit_my_photos". If you
do access::can("edit") it's not obvious to the caller what's
going on there because there is an edit permission.
If you rename "edit" to "edit_album" (which is consistent)
then "edit" doesn't directly map to a column and we can
consider it an alias permission. We had those in G2,
but they were heavyweight (using bit vectors, etc). This will
just be a simple alias that you cover in access::can.
So then we can try to map this to the diagram we came up
with:
For the remainder of this mail I'm going to assume that
we rename edit->edit_album and use edit as an
alias.
Going through the diagram, I'm convinced that your code is
right for the root album (A1) and for the photos (P1, P2).
However, I'm not sure that we have A2 correct. Right now
your equality for A2 is:
edit(A2) == access::can("edit_album", A2) ||
(
access::can("edit_all_photos",
A1) ||
(
is_owner(A2) &&
access::can("edit_my_photos",
A1)
)
)
I'm not sure if that's right.. If I grant a
user the ability to create A2 under A1, I would imagine that
they would start off with the edit_album permission -- but I
could see that they wouldn't get that permission because
they don't have edit_album on the parent so they wouldn't
inherit it. But we still want them to be able to edit the
album they just created, right? I guess that means that we
really do need to treat sub-albums the same as sub-photos in
that case.
Is that logic correct? If so, then we should
rename:
- edit --> edit_album
- edit_all_photos -->
edit_all
- edit_my_photos ->
edit_mine
This makes our logic easier because we stop
differentiating child albums vs. photos. To close the
loop, I'm going to write it in pseudocode below so that
I'm sure I have it right.
access::can("edit", X) ==
C1 access::can("edit_album", X) || /* X !=
album? it comes from parent */
C2 access::can("edit_all", X->parent)
||
(
C3 access::can("edit_mine",
X->parent) &&
is_owner(X)
)
So that means the equalities are:
edit(A2) == edit_album(A1) || edit_all(A1) ||
(edit_mine(A1) && is_owner(A2))
edit(P2) == edit_album(A1) || edit_all(A1) ||
(edit_mine(A1) && is_owner(P2))
Scenarios:
S1: album creation:
- Admin user grants only "view" and "add_album" to groups
G1 (containing U1), G2 (U2) at the root album (A0)
- U1 creates album A1, which inherits permissions from
root album. A1's permissions are now "view" and
"add_album" for G1/G2. U1 cannot edit A1 even
though they own it.
- Admin grants "edit_mine" to A0
- Now U1 can edit A1 because of permission clause C3
above.
- U2 creates album A2 and can immediately edit A2 because
of step 3 and C3.
S2: photo addition (continues from scenario S1):
- U1 creates photo P1A inside A1.
- U1 can edit P1A because of C1
- U2 cannot add photos to A1
- U1 grants "add_photo" to G2 on A1
- U2 now adds photo P1B to A2
- U2 cannot edit P1B because he has no edit permissions
- U1 grants "edit_mine" to G2 on A1
- U2 can now edit P1B but not P1A (** we're not talking
about delete yet)
- U1 revokes "edit_mine" from G2 on A1
- U2 can no longer edit P1B
- U1 grants "edit_all" to G2 on A1
- U2 can now edit P1A (which he does not own) and P1B
(which he owns)
Ok. I think I'm satisfied with that. One thing I noticed
is that you did not implement the aliasing for group_can(). I
don't think that we need to do that currently, but right now
because you haven't changed the name of the "edit" permission
anybody who calls group_can("edit") is going to get an
unexpected result -- they're essentially getting
group_can("edit_album"), not the alias. Once you rename the
permission, it means that they'll get an error, which I think
is a good thing since it'll force them to figure out what to
do. A quick grep around gallery3 and gallery3-contrib
indicates that nobody does this outside of the core code now,
so this is a good time to do this.
So to sum up, here's what I think we need to do:
- Rename edit -> edit_album
- Rename edit_all_photos -> edit_all
- Rename edit_my_photos -> edit_mine
-
Replace the _user_can_edit() logic with what I have above,
and (I suggest) you inline it into user_can()
- Write up all the above scenarios as unit tests (!!!)
Whew!
No virus found in
this message.
Checked by AVG - www.avg.com
Version: 10.0.1209 / Virus Database: 1500/3608 - Release Date:
05/01/11
------------------------------------------------------------------------------
WhatsUp Gold - Download Free Network Management Software
The most intuitive, comprehensive, and cost-effective network
management toolset available today. Delivers lowest initial
acquisition cost and overall TCO of any competing solution.
http://p.sf.net/sfu/whatsupgold-sd__[ g a l l e r y - d e v e l ]_________________________
[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]
|
|
Re: Continuation of previous note
On Sun, May 1, 2011 at 6:31 PM, Tim Almdal <tnalmdal@...> wrote:
I'll give your suggestions a try.
I notice that you're not copying the edit permissions in the
upgrader. Just to verify: we decide that was ok because "edit" is a
superset of "edit_xxx" right?
No, it was my understanding that we didn't copy edit
permissions because I thought we had finally distilled everything
down to the bottom half of this http://gallery.menalto.com/gallery/doc-images/gcon2011/IMG_0162.JPG.html.
and that we felt that edit would work just as it did now. If the
administrator wanted to use the new permissions that they would
have to explicitly chose to enable them.
is your wrapper around _get_all_groups() really necessary?
using the helper _get_all_groups() was just from copying
code... your right i should be able to just call identity::groups.
How about we restrict organize to users who have the "edit"
permission? That should be unambiguous.
works for me
For the remainder of this mail I'm going to assume that we rename
edit->edit_album and use edit as an alias.
Not a safe assumption, at this point, as mentioned
above, I was not coding to (http://gallery.menalto.com/gallery/doc-images/gcon2011/IMG_0161.JPG.html)
but to http://gallery.menalto.com/gallery/doc-images/gcon2011/IMG_0162.JPG.html.
As you can see from that image, we had crossed out the rename of
edit. What's in a name, so it doesn't matter to me whether we
rename it or not.
Those two images are the same, except that the name is different. But the name is relevant because you're overloading "edit" to be a function of all three separate permissions. In your model, we have 3 permissions (edit, edit_all_photos, edit_my_photos) but access::can("edit") doesn't actually refer to the edit permission -- it refers to a function of all three. That is confusing. See my last email, starting at "I initially had a negative reaction..."
I think that trying to extend or convert "edit/edit album"
permission on albums that are added by users is a fools errand and
destined to make things complicated. (We might have to
I am not suggesting that. I am merely suggesting that you rename the permissions to be internally consistent with their names. We are considering the "edit" permission to have the effect of "edit this album" and we should name it internally accordingly. We should not let access::can("edit") be ambiguous in its meaning.
mess
around with the inheritance portion of our existing permission
code). I think the way to do this is force users to have one of
("edit/edit album", edit_all, or edit_mine) permissions in
addition to the add_album or add_photo permissions. That way our
existing inheritancemodel works naturally. I really don't like
the idea (which I think you are suggesting) of when an album is
added we then create ("edit/edit album") permission for the album.
That's a lot of work and breaks the natural inheritance that we
have now.
Forcing the permissions to relate to each other is non-trivial, and ultimately not necessary. edit_album refers to the album as a container and you are granted all the rights you typically get on a container (which extends to the contents of the container). edit_mine and edit_all give you rights on members of the container, but not rights on the container itself. I don't believe that we have to mess with the permission inheritance model.
What's currently in the code, allows inheritance of the
"add_photo" and "add_album" permissions without regard to the edit
permissions. For example you could create an album and I could
add sub-albums and photo's to your album even if I can't edit them
because the add_* permissions where inherited w/o regard to what
my edit permissions are. Tying them together with the add_*
permissions a sub-permission of the edit* permissions prevents
that blank inheritance.
Sorry, I don't follow what you're saying. Add and edit permissions are separate in your model, and are separate in my proposed model. What you say above implies that I'm suggesting that we tie them together somehow, but I'm not suggesting that at all. I am merely suggesting that we consider edit to be an alias for a function of {edit_album, edit_mine, edit_all}.
This makes our logic easier because we stop
differentiating child albums vs. photos. To close the loop, I'm
going to write it in pseudocode below so that I'm sure I have it
right.
access::can("edit",
X) ==
C1
access::can("edit_album", X) || /* X != album? it comes from
parent */
C2 access::can("edit_all", X->parent) ||
(
C3 access::can("edit_mine", X->parent)
&&
is_owner(X)
)
That's what I believe I have implemented.
No, what you have is more complicated because it's differentiating between albums and photos, when in reality according to the pseudocode I have above, that distinction is no longer necessary. See:
On that line you're checking to see if it's an album and if so, you're taking the permission of the parent. I'm saying that you don't need to do that because the parent's edit_album will apply to the child if and only if the child has not overridden it. In my model, if the child overrides edit_album, then the parent can't edit it. Hmm. Now that I think about it, this has to be wrong because it would allow the owner of a sub-album to remove the parent's ability to edit it, even though the parent has edit_album.
So in my mind,
1) Decide which diagram we are using (I'm assuming 162) 2) Clean up the installer code especially the _copy_permission code
as you suggested 3) Change the add_* permissions to also require an edit permission
(bury that check in access.php so no code has to change external to
access.php to enforce that) 4) Write some unit tests. 5) Generate the install SQL
And we should be good to go
I don't think that we've converged yet on the right model. I think that perhaps you misunderstood what I was saying above about permission inheritance.
I want to get all the scenarios in one place, and email isn't great for that. I'm going to start a doc which outlines all the various scenarios so that we can refer to them and then make sure that we've covered everything. I'll ping back when that's done.
-Bharat
Tim
On 5/1/2011 12:54 PM, Bharat Mediratta wrote:
(broadening the audience to include gallery-devel -- this is
a continuation of a discussion that Tim and I are having around
the branch to fix ticket #1273).
----
Tim,
I notice that you're not copying the edit permissions in the
upgrader. Just to verify: we decide that was ok because "edit"
is a superset of "edit_xxx" right? In this diagram:
IIRC, "edit_album" is the equivalent of what you have as
"edit" right now.. I wonder if we should actually rename the
permission to be "Edit album" to avoid confusion there.
_copy_permissions is a PHP based O(N*M) operation where
N==the number of rows and M== the number of groups. That's going
to be really inefficient. Let's get that down to O(M) using the
db builder. It'd be something like:
foreach ($groups as $group) {
...
db::build()
->from($table)
->set($to_column, db::expr($from_column))
->execute();
}
is your wrapper around _get_all_groups() really necessary?
You're only calling that from the upgrader and when we get to
that point there had better be an identity provider active!
More below.
On Sat, Apr 30, 2011 at 6:11 PM, Tim
Almdal <tnalmdal@...>
wrote: Just a little bit more information, I went and used user1 to
add a photo into user2's album (the waterfall). So now there
is a picture that user2 can't do anything about.
The other interesting thing is that user2 can now get the
organize dialog and edit permission dialogs for their
directory. Not sure of this is what we want.
How about we restrict organize to users who have the "edit"
permission? That should be unambiguous.
My basic approach was rather than put all sorts of stuff all
over the place to check edit permissions, I just changed
access.php so that the interface accepts
access::can("edit"...) and it figures out whether the user has
edit, edit_all_photos or edit_my_photos.
I initially had a negative reaction to this idea, but I
thought it through and it makes sense. There's an
inconsistency that bothers me, though. Right now you have 3
perms "edit", "edit_all_photos" and "edit_my_photos". If you
do access::can("edit") it's not obvious to the caller what's
going on there because there is an edit permission.
If you rename "edit" to "edit_album" (which is consistent)
then "edit" doesn't directly map to a column and we can
consider it an alias permission. We had those in G2,
but they were heavyweight (using bit vectors, etc). This will
just be a simple alias that you cover in access::can.
So then we can try to map this to the diagram we came up
with:
For the remainder of this mail I'm going to assume that
we rename edit->edit_album and use edit as an
alias.
Going through the diagram, I'm convinced that your code is
right for the root album (A1) and for the photos (P1, P2).
However, I'm not sure that we have A2 correct. Right now
your equality for A2 is:
edit(A2) == access::can("edit_album", A2) ||
(
access::can("edit_all_photos",
A1) ||
(
is_owner(A2) &&
access::can("edit_my_photos",
A1)
)
)
I'm not sure if that's right.. If I grant a
user the ability to create A2 under A1, I would imagine that
they would start off with the edit_album permission -- but I
could see that they wouldn't get that permission because
they don't have edit_album on the parent so they wouldn't
inherit it. But we still want them to be able to edit the
album they just created, right? I guess that means that we
really do need to treat sub-albums the same as sub-photos in
that case.
Is that logic correct? If so, then we should
rename:
- edit --> edit_album
- edit_all_photos -->
edit_all
- edit_my_photos ->
edit_mine
This makes our logic easier because we stop
differentiating child albums vs. photos. To close the
loop, I'm going to write it in pseudocode below so that
I'm sure I have it right.
access::can("edit", X) ==
C1 access::can("edit_album", X) || /* X !=
album? it comes from parent */
C2 access::can("edit_all", X->parent)
||
(
C3 access::can("edit_mine",
X->parent) &&
is_owner(X)
)
So that means the equalities are:
edit(A2) == edit_album(A1) || edit_all(A1) ||
(edit_mine(A1) && is_owner(A2))
edit(P2) == edit_album(A1) || edit_all(A1) ||
(edit_mine(A1) && is_owner(P2))
Scenarios:
S1: album creation:
- Admin user grants only "view" and "add_album" to groups
G1 (containing U1), G2 (U2) at the root album (A0)
- U1 creates album A1, which inherits permissions from
root album. A1's permissions are now "view" and
"add_album" for G1/G2. U1 cannot edit A1 even
though they own it.
- Admin grants "edit_mine" to A0
- Now U1 can edit A1 because of permission clause C3
above.
- U2 creates album A2 and can immediately edit A2 because
of step 3 and C3.
S2: photo addition (continues from scenario S1):
- U1 creates photo P1A inside A1.
- U1 can edit P1A because of C1
- U2 cannot add photos to A1
- U1 grants "add_photo" to G2 on A1
- U2 now adds photo P1B to A2
- U2 cannot edit P1B because he has no edit permissions
- U1 grants "edit_mine" to G2 on A1
- U2 can now edit P1B but not P1A (** we're not talking
about delete yet)
- U1 revokes "edit_mine" from G2 on A1
- U2 can no longer edit P1B
- U1 grants "edit_all" to G2 on A1
- U2 can now edit P1A (which he does not own) and P1B
(which he owns)
Ok. I think I'm satisfied with that. One thing I noticed
is that you did not implement the aliasing for group_can(). I
don't think that we need to do that currently, but right now
because you haven't changed the name of the "edit" permission
anybody who calls group_can("edit") is going to get an
unexpected result -- they're essentially getting
group_can("edit_album"), not the alias. Once you rename the
permission, it means that they'll get an error, which I think
is a good thing since it'll force them to figure out what to
do. A quick grep around gallery3 and gallery3-contrib
indicates that nobody does this outside of the core code now,
so this is a good time to do this.
So to sum up, here's what I think we need to do:
- Rename edit -> edit_album
- Rename edit_all_photos -> edit_all
- Rename edit_my_photos -> edit_mine
-
Replace the _user_can_edit() logic with what I have above,
and (I suggest) you inline it into user_can()
- Write up all the above scenarios as unit tests (!!!)
Whew!
No virus found in
this message.
Checked by AVG - www.avg.com
Version: 10.0.1209 / Virus Database: 1500/3608 - Release Date:
05/01/11
------------------------------------------------------------------------------
WhatsUp Gold - Download Free Network Management Software
The most intuitive, comprehensive, and cost-effective network
management toolset available today. Delivers lowest initial
acquisition cost and overall TCO of any competing solution.
http://p.sf.net/sfu/whatsupgold-sd__[ g a l l e r y - d e v e l ]_________________________
[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]
|
|
Re: Continuation of previous note
Documenting is good.
But I'm confused again... line 115 was in the original code (if its
an album use its id, if its not an album then get its parent id)
isn't doing the same thing as access::can("edit_album",
X) || /* X != album? it comes from parent */
Tim
On 5/1/2011 7:09 PM, Bharat Mediratta wrote:
On Sun, May 1, 2011 at 6:31 PM, Tim
Almdal <tnalmdal@...>
wrote:
I'll give your
suggestions a try.
I notice that you're not copying the edit permissions in
the upgrader. Just to verify: we decide that was ok
because "edit" is a superset of "edit_xxx" right?
No, it was my understanding that we didn't copy
edit permissions because I thought we had finally
distilled everything down to the bottom half of this http://gallery.menalto.com/gallery/doc-images/gcon2011/IMG_0162.JPG.html.
and that we felt that edit would work just as it did now.
If the administrator wanted to use the new permissions
that they would have to explicitly chose to enable them.
is your wrapper around _get_all_groups()
really necessary?
using the helper _get_all_groups() was just from
copying code... your right i should be able to just call
identity::groups.
How about we restrict organize to users who have the
"edit" permission? That should be unambiguous.
works for me
For the remainder of this mail I'm going to assume that
we rename edit->edit_album and use edit as an
alias.
Not a safe assumption, at this point, as
mentioned above, I was not coding to (http://gallery.menalto.com/gallery/doc-images/gcon2011/IMG_0161.JPG.html)
but to http://gallery.menalto.com/gallery/doc-images/gcon2011/IMG_0162.JPG.html.
As you can see from that image, we had crossed out the
rename of edit. What's in a name, so it doesn't matter to
me whether we rename it or not.
Those two images are the same, except that the name is
different. But the name is relevant because you're
overloading "edit" to be a function of all three separate
permissions. In your model, we have 3 permissions (edit,
edit_all_photos, edit_my_photos) but access::can("edit")
doesn't actually refer to the edit permission -- it refers to
a function of all three. That is confusing. See my last
email, starting at "I initially had a negative reaction..."
I think that trying to
extend or convert "edit/edit album" permission on albums
that are added by users is a fools errand and destined
to make things complicated. (We might have to
I am not suggesting that. I am merely suggesting that you
rename the permissions to be internally consistent with their
names. We are considering the "edit" permission to have the
effect of "edit this album" and we should name it internally
accordingly. We should not let access::can("edit") be
ambiguous in its meaning.
mess around with the
inheritance portion of our existing permission code). I
think the way to do this is force users to have one of
("edit/edit album", edit_all, or edit_mine) permissions
in addition to the add_album or add_photo permissions.
That way our existing inheritancemodel works naturally.
I really don't like the idea (which I think you are
suggesting) of when an album is added we then create
("edit/edit album") permission for the album. That's a
lot of work and breaks the natural inheritance that we
have now.
Forcing the permissions to relate to each other is
non-trivial, and ultimately not necessary. edit_album refers
to the album as a container and you are granted all the rights
you typically get on a container (which extends to the
contents of the container). edit_mine and edit_all give
you rights on members of the container, but not rights on the
container itself. I don't believe that we have to mess with
the permission inheritance model.
What's currently in the
code, allows inheritance of the "add_photo" and
"add_album" permissions without regard to the edit
permissions. For example you could create an album and
I could add sub-albums and photo's to your album even if
I can't edit them because the add_* permissions where
inherited w/o regard to what my edit permissions are.
Tying them together with the add_* permissions a
sub-permission of the edit* permissions prevents that
blank inheritance.
Sorry, I don't follow what you're saying. Add and edit
permissions are separate in your model, and are separate in my
proposed model. What you say above implies that I'm
suggesting that we tie them together somehow, but I'm not
suggesting that at all. I am merely suggesting that we
consider edit to be an alias for a function of
{edit_album, edit_mine, edit_all}.
This makes our logic easier
because we stop differentiating child albums vs.
photos. To close the loop, I'm going to write it in
pseudocode below so that I'm sure I have it right.
access::can("edit", X) ==
C1 access::can("edit_album", X) || /* X !=
album? it comes from parent */
C2 access::can("edit_all",
X->parent) ||
(
C3
access::can("edit_mine", X->parent) &&
is_owner(X)
)
That's what I believe
I have implemented.
No, what you have is more complicated because it's
differentiating between albums and photos, when in reality
according to the pseudocode I have above, that distinction is
no longer necessary. See:
On that line you're checking to see if it's an album and if
so, you're taking the permission of the parent. I'm saying
that you don't need to do that because the parent's edit_album
will apply to the child if and only if the child has not
overridden it. In my model, if the child overrides
edit_album, then the parent can't edit it. Hmm. Now that I
think about it, this has to be wrong because it would allow
the owner of a sub-album to remove the parent's ability to
edit it, even though the parent has edit_album.
So in my mind,
1) Decide which diagram we are using (I'm assuming 162)
2) Clean up the installer
code especially the _copy_permission code as you suggested
3) Change the add_*
permissions to also require an edit permission (bury that
check in access.php so no code has to change external to
access.php to enforce that)
4) Write some unit tests.
5) Generate the install SQL
And we should be good to go
I don't think that we've converged yet on the right model.
I think that perhaps you misunderstood what I was saying
above about permission inheritance.
I want to get all the scenarios in one place, and email
isn't great for that. I'm going to start a doc which outlines
all the various scenarios so that we can refer to them and
then make sure that we've covered everything. I'll ping back
when that's done..
-Bharat
Tim
On 5/1/2011 12:54 PM,
Bharat Mediratta wrote:
(broadening the
audience to include gallery-devel -- this is a
continuation of a discussion that Tim and I are
having around the branch to fix ticket #1273).
----
Tim,
I notice that you're
not copying the edit permissions in the upgrader.
Just to verify: we decide that was ok because
"edit" is a superset of "edit_xxx" right? In this
diagram:
IIRC, "edit_album" is
the equivalent of what you have as "edit" right
now.. I wonder if we should actually rename the
permission to be "Edit album" to avoid confusion
there.
_copy_permissions is a
PHP based O(N*M) operation where N==the number of
rows and M== the number of groups. That's going to
be really inefficient. Let's get that down to O(M)
using the db builder. It'd be something like:
foreach ($groups as
$group) {
...
db::build()
->from($table)
->set($to_column, db::expr($from_column))
->execute();
}
is your wrapper around
_get_all_groups() really necessary? You're only
calling that from the upgrader and when we get to
that point there had better be an identity provider
active!
More below.
On Sat, Apr 30, 2011
at 6:11 PM, Tim Almdal <tnalmdal@...>
wrote:
Just a little bit
more information, I went and used user1 to add a
photo into user2's album (the waterfall). So now
there is a picture that user2 can't do anything
about.
The other
interesting thing is that user2 can now get the
organize dialog and edit permission dialogs for
their directory. Not sure of this is what we
want.
How about we restrict
organize to users who have the "edit" permission?
That should be unambiguous.
My basic approach
was rather than put all sorts of stuff all over
the place to check edit permissions, I just
changed access.php so that the interface accepts
access::can("edit"...) and it figures out
whether the user has edit, edit_all_photos or
edit_my_photos.
I initially had a
negative reaction to this idea, but I thought it
through and it makes sense. There's an
inconsistency that bothers me, though. Right now
you have 3 perms "edit", "edit_all_photos" and
"edit_my_photos". If you do access::can("edit")
it's not obvious to the caller what's going on
there because there is an edit permission.
If you rename "edit" to "edit_album" (which is
consistent) then "edit" doesn't directly map to a
column and we can consider it an alias
permission. We had those in G2, but they were
heavyweight (using bit vectors, etc). This will
just be a simple alias that you cover in
access::can.
So then we can try to
map this to the diagram we came up with:
For the remainder of
this mail I'm going to assume that we rename
edit->edit_album and use edit as an
alias.
Going through the
diagram, I'm convinced that your code is right for
the root album (A1) and for the photos (P1, P2).
However, I'm not sure that we have A2 correct.
Right now your equality for A2 is:
edit(A2) ==
access::can("edit_album", A2) ||
(
access::can("edit_all_photos",
A1) ||
(
is_owner(A2) &&
access::can("edit_my_photos",
A1)
)
)
I'm not sure if that's
right.. If I grant a user the ability to create
A2 under A1, I would imagine that they would
start off with the edit_album permission -- but
I could see that they wouldn't get that
permission because they don't have edit_album on
the parent so they wouldn't inherit it. But we
still want them to be able to edit the album
they just created, right? I guess that means
that we really do need to treat sub-albums the
same as sub-photos in that case.
Is that logic correct?
If so, then we should rename:
- edit -->
edit_album
- edit_all_photos
--> edit_all
- edit_my_photos
-> edit_mine
This makes our logic
easier because we stop differentiating child
albums vs. photos. To close the loop, I'm
going to write it in pseudocode below so that
I'm sure I have it right.
access::can("edit", X) ==
C1 access::can("edit_album",
X) || /* X != album? it comes from parent */
C2
access::can("edit_all", X->parent) ||
(
C3
access::can("edit_mine", X->parent)
&&
is_owner(X)
)
So that means the
equalities are:
edit(A2) == edit_album(A1) ||
edit_all(A1) || (edit_mine(A1) &&
is_owner(A2))
edit(P2) ==
edit_album(A1) || edit_all(A1) ||
(edit_mine(A1) && is_owner(P2))
Scenarios:
S1: album
creation:
- Admin user grants
only "view" and "add_album" to groups G1
(containing U1), G2 (U2) at the root album
(A0)
- U1 creates album
A1, which inherits permissions from root
album. A1's permissions are now "view" and
"add_album" for G1/G2. U1 cannot edit
A1 even though they own it.
- Admin grants
"edit_mine" to A0
- Now U1 can edit A1
because of permission clause C3 above.
- U2 creates album
A2 and can immediately edit A2 because of step
3 and C3.
S2: photo
addition (continues from scenario S1):
- U1 creates photo
P1A inside A1.
- U1 can edit P1A
because of C1
- U2 cannot add
photos to A1
- U1 grants
"add_photo" to G2 on A1
- U2 now adds photo
P1B to A2
- U2 cannot edit P1B
because he has no edit permissions
- U1 grants
"edit_mine" to G2 on A1
- U2 can now edit
P1B but not P1A (** we're not talking about
delete yet)
- U1 revokes
"edit_mine" from G2 on A1
- U2 can no longer
edit P1B
- U1 grants
"edit_all" to G2 on A1
- U2 can now edit
P1A (which he does not own) and P1B (which he
owns)
Ok. I think I'm
satisfied with that. One thing I noticed is that
you did not implement the aliasing for
group_can(). I don't think that we need to do
that currently, but right now because you haven't
changed the name of the "edit" permission anybody
who calls group_can("edit") is going to get an
unexpected result -- they're essentially getting
group_can("edit_album"), not the alias. Once you
rename the permission, it means that they'll get
an error, which I think is a good thing since
it'll force them to figure out what to do. A
quick grep around gallery3 and gallery3-contrib
indicates that nobody does this outside of the
core code now, so this is a good time to do this.
So to sum up, here's
what I think we need to do:
- Rename edit ->
edit_album
- Rename
edit_all_photos -> edit_all
- Rename
edit_my_photos -> edit_mine
- Replace the
_user_can_edit() logic with what I have above,
and (I suggest) you inline it into user_can()
- Write up all the
above scenarios as unit tests (!!!)
Whew!
No virus found in this
message.
Checked by AVG - www.avg.com
Version: 10.0.1209 / Virus Database: 1500/3608 - Release
Date: 05/01/11
No virus found in
this message.
Checked by AVG - www.avg.com
Version: 10.0.1209 / Virus Database: 1500/3609 - Release Date:
05/01/11
------------------------------------------------------------------------------
WhatsUp Gold - Download Free Network Management Software
The most intuitive, comprehensive, and cost-effective network
management toolset available today. Delivers lowest initial
acquisition cost and overall TCO of any competing solution.
http://p.sf.net/sfu/whatsupgold-sd__[ g a l l e r y - d e v e l ]_________________________
[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]
|
|
Re: Continuation of previous note
On Sun, May 1, 2011 at 7:26 PM, Tim Almdal <tnalmdal@...> wrote:
Documenting is good.
But I'm confused again... line 115 was in the original code (if its
an album use its id, if its not an album then get its parent id)
isn't doing the same thing as access::can("edit_album",
X) || /* X != album? it comes from parent */
Yeah, this is definitely confusing. In my original email, I was suggesting that we change that around -- but now I'm going to go into Google Docs and write up a comprehensive set of scenarios that I think we should cover, and we can rethink exactly how we want this to work.
Tim
On 5/1/2011 7:09 PM, Bharat Mediratta wrote:
On Sun, May 1, 2011 at 6:31 PM, Tim
Almdal <tnalmdal@...>
wrote:
I'll give your
suggestions a try.
I notice that you're not copying the edit permissions in
the upgrader. Just to verify: we decide that was ok
because "edit" is a superset of "edit_xxx" right?
No, it was my understanding that we didn't copy
edit permissions because I thought we had finally
distilled everything down to the bottom half of this http://gallery.menalto.com/gallery/doc-images/gcon2011/IMG_0162.JPG.html.
and that we felt that edit would work just as it did now.
If the administrator wanted to use the new permissions
that they would have to explicitly chose to enable them.
is your wrapper around _get_all_groups()
really necessary?
using the helper _get_all_groups() was just from
copying code... your right i should be able to just call
identity::groups.
How about we restrict organize to users who have the
"edit" permission? That should be unambiguous.
works for me
For the remainder of this mail I'm going to assume that
we rename edit->edit_album and use edit as an
alias.
Not a safe assumption, at this point, as
mentioned above, I was not coding to (http://gallery.menalto.com/gallery/doc-images/gcon2011/IMG_0161.JPG.html)
but to http://gallery.menalto.com/gallery/doc-images/gcon2011/IMG_0162.JPG.html.
As you can see from that image, we had crossed out the
rename of edit. What's in a name, so it doesn't matter to
me whether we rename it or not.
Those two images are the same, except that the name is
different. But the name is relevant because you're
overloading "edit" to be a function of all three separate
permissions. In your model, we have 3 permissions (edit,
edit_all_photos, edit_my_photos) but access::can("edit")
doesn't actually refer to the edit permission -- it refers to
a function of all three. That is confusing. See my last
email, starting at "I initially had a negative reaction..."
I think that trying to
extend or convert "edit/edit album" permission on albums
that are added by users is a fools errand and destined
to make things complicated. (We might have to
I am not suggesting that. I am merely suggesting that you
rename the permissions to be internally consistent with their
names. We are considering the "edit" permission to have the
effect of "edit this album" and we should name it internally
accordingly. We should not let access::can("edit") be
ambiguous in its meaning.
mess around with the
inheritance portion of our existing permission code). I
think the way to do this is force users to have one of
("edit/edit album", edit_all, or edit_mine) permissions
in addition to the add_album or add_photo permissions.
That way our existing inheritancemodel works naturally.
I really don't like the idea (which I think you are
suggesting) of when an album is added we then create
("edit/edit album") permission for the album. That's a
lot of work and breaks the natural inheritance that we
have now.
Forcing the permissions to relate to each other is
non-trivial, and ultimately not necessary. edit_album refers
to the album as a container and you are granted all the rights
you typically get on a container (which extends to the
contents of the container). edit_mine and edit_all give
you rights on members of the container, but not rights on the
container itself. I don't believe that we have to mess with
the permission inheritance model.
What's currently in the
code, allows inheritance of the "add_photo" and
"add_album" permissions without regard to the edit
permissions. For example you could create an album and
I could add sub-albums and photo's to your album even if
I can't edit them because the add_* permissions where
inherited w/o regard to what my edit permissions are.
Tying them together with the add_* permissions a
sub-permission of the edit* permissions prevents that
blank inheritance.
Sorry, I don't follow what you're saying. Add and edit
permissions are separate in your model, and are separate in my
proposed model. What you say above implies that I'm
suggesting that we tie them together somehow, but I'm not
suggesting that at all. I am merely suggesting that we
consider edit to be an alias for a function of
{edit_album, edit_mine, edit_all}.
This makes our logic easier
because we stop differentiating child albums vs.
photos. To close the loop, I'm going to write it in
pseudocode below so that I'm sure I have it right.
access::can("edit", X) ==
C1 access::can("edit_album", X) || /* X !=
album? it comes from parent */
C2 access::can("edit_all",
X->parent) ||
(
C3
access::can("edit_mine", X->parent) &&
is_owner(X)
)
That's what I believe
I have implemented.
No, what you have is more complicated because it's
differentiating between albums and photos, when in reality
according to the pseudocode I have above, that distinction is
no longer necessary. See:
On that line you're checking to see if it's an album and if
so, you're taking the permission of the parent. I'm saying
that you don't need to do that because the parent's edit_album
will apply to the child if and only if the child has not
overridden it. In my model, if the child overrides
edit_album, then the parent can't edit it. Hmm. Now that I
think about it, this has to be wrong because it would allow
the owner of a sub-album to remove the parent's ability to
edit it, even though the parent has edit_album.
So in my mind,
1) Decide which diagram we are using (I'm assuming 162)
2) Clean up the installer
code especially the _copy_permission code as you suggested
3) Change the add_*
permissions to also require an edit permission (bury that
check in access.php so no code has to change external to
access.php to enforce that)
4) Write some unit tests.
5) Generate the install SQL
And we should be good to go
I don't think that we've converged yet on the right model.
I think that perhaps you misunderstood what I was saying
above about permission inheritance.
I want to get all the scenarios in one place, and email
isn't great for that. I'm going to start a doc which outlines
all the various scenarios so that we can refer to them and
then make sure that we've covered everything. I'll ping back
when that's done..
-Bharat
Tim
On 5/1/2011 12:54 PM,
Bharat Mediratta wrote:
(broadening the
audience to include gallery-devel -- this is a
continuation of a discussion that Tim and I are
having around the branch to fix ticket #1273).
----
Tim,
I notice that you're
not copying the edit permissions in the upgrader.
Just to verify: we decide that was ok because
"edit" is a superset of "edit_xxx" right? In this
diagram:
IIRC, "edit_album" is
the equivalent of what you have as "edit" right
now.. I wonder if we should actually rename the
permission to be "Edit album" to avoid confusion
there.
_copy_permissions is a
PHP based O(N*M) operation where N==the number of
rows and M== the number of groups. That's going to
be really inefficient. Let's get that down to O(M)
using the db builder. It'd be something like:
foreach ($groups as
$group) {
...
db::build()
->from($table)
->set($to_column, db::expr($from_column))
->execute();
}
is your wrapper around
_get_all_groups() really necessary? You're only
calling that from the upgrader and when we get to
that point there had better be an identity provider
active!
More below.
On Sat, Apr 30, 2011
at 6:11 PM, Tim Almdal <tnalmdal@...>
wrote:
Just a little bit
more information, I went and used user1 to add a
photo into user2's album (the waterfall). So now
there is a picture that user2 can't do anything
about.
The other
interesting thing is that user2 can now get the
organize dialog and edit permission dialogs for
their directory. Not sure of this is what we
want.
How about we restrict
organize to users who have the "edit" permission?
That should be unambiguous.
My basic approach
was rather than put all sorts of stuff all over
the place to check edit permissions, I just
changed access.php so that the interface accepts
access::can("edit"...) and it figures out
whether the user has edit, edit_all_photos or
edit_my_photos.
I initially had a
negative reaction to this idea, but I thought it
through and it makes sense. There's an
inconsistency that bothers me, though. Right now
you have 3 perms "edit", "edit_all_photos" and
"edit_my_photos". If you do access::can("edit")
it's not obvious to the caller what's going on
there because there is an edit permission.
If you rename "edit" to "edit_album" (which is
consistent) then "edit" doesn't directly map to a
column and we can consider it an alias
permission. We had those in G2, but they were
heavyweight (using bit vectors, etc). This will
just be a simple alias that you cover in
access::can.
So then we can try to
map this to the diagram we came up with:
For the remainder of
this mail I'm going to assume that we rename
edit->edit_album and use edit as an
alias.
Going through the
diagram, I'm convinced that your code is right for
the root album (A1) and for the photos (P1, P2).
However, I'm not sure that we have A2 correct.
Right now your equality for A2 is:
edit(A2) ==
access::can("edit_album", A2) ||
(
access::can("edit_all_photos",
A1) ||
(
is_owner(A2) &&
access::can("edit_my_photos",
A1)
)
)
I'm not sure if that's
right.. If I grant a user the ability to create
A2 under A1, I would imagine that they would
start off with the edit_album permission -- but
I could see that they wouldn't get that
permission because they don't have edit_album on
the parent so they wouldn't inherit it. But we
still want them to be able to edit the album
they just created, right? I guess that means
that we really do need to treat sub-albums the
same as sub-photos in that case.
Is that logic correct?
If so, then we should rename:
- edit -->
edit_album
- edit_all_photos
--> edit_all
- edit_my_photos
-> edit_mine
This makes our logic
easier because we stop differentiating child
albums vs. photos. To close the loop, I'm
going to write it in pseudocode below so that
I'm sure I have it right.
access::can("edit", X) ==
C1 access::can("edit_album",
X) || /* X != album? it comes from parent */
C2
access::can("edit_all", X->parent) ||
(
C3
access::can("edit_mine", X->parent)
&&
is_owner(X)
)
So that means the
equalities are:
edit(A2) == edit_album(A1) ||
edit_all(A1) || (edit_mine(A1) &&
is_owner(A2))
edit(P2) ==
edit_album(A1) || edit_all(A1) ||
(edit_mine(A1) && is_owner(P2))
Scenarios:
S1: album
creation:
- Admin user grants
only "view" and "add_album" to groups G1
(containing U1), G2 (U2) at the root album
(A0)
- U1 creates album
A1, which inherits permissions from root
album. A1's permissions are now "view" and
"add_album" for G1/G2. U1 cannot edit
A1 even though they own it.
- Admin grants
"edit_mine" to A0
- Now U1 can edit A1
because of permission clause C3 above.
- U2 creates album
A2 and can immediately edit A2 because of step
3 and C3.
S2: photo
addition (continues from scenario S1):
- U1 creates photo
P1A inside A1.
- U1 can edit P1A
because of C1
- U2 cannot add
photos to A1
- U1 grants
"add_photo" to G2 on A1
- U2 now adds photo
P1B to A2
- U2 cannot edit P1B
because he has no edit permissions
- U1 grants
"edit_mine" to G2 on A1
- U2 can now edit
P1B but not P1A (** we're not talking about
delete yet)
- U1 revokes
"edit_mine" from G2 on A1
- U2 can no longer
edit P1B
- U1 grants
"edit_all" to G2 on A1
- U2 can now edit
P1A (which he does not own) and P1B (which he
owns)
Ok. I think I'm
satisfied with that. One thing I noticed is that
you did not implement the aliasing for
group_can(). I don't think that we need to do
that currently, but right now because you haven't
changed the name of the "edit" permission anybody
who calls group_can("edit") is going to get an
unexpected result -- they're essentially getting
group_can("edit_album"), not the alias. Once you
rename the permission, it means that they'll get
an error, which I think is a good thing since
it'll force them to figure out what to do. A
quick grep around gallery3 and gallery3-contrib
indicates that nobody does this outside of the
core code now, so this is a good time to do this.
So to sum up, here's
what I think we need to do:
- Rename edit ->
edit_album
- Rename
edit_all_photos -> edit_all
- Rename
edit_my_photos -> edit_mine
- Replace the
_user_can_edit() logic with what I have above,
and (I suggest) you inline it into user_can()
- Write up all the
above scenarios as unit tests (!!!)
Whew!
No virus found in this
message.
Checked by AVG - www.avg.com
Version: 10.0.1209 / Virus Database: 1500/3608 - Release
Date: 05/01/11
No virus found in
this message.
Checked by AVG - www.avg.com
------------------------------------------------------------------------------
WhatsUp Gold - Download Free Network Management Software
The most intuitive, comprehensive, and cost-effective network
management toolset available today. Delivers lowest initial
acquisition cost and overall TCO of any competing solution.
http://p.sf.net/sfu/whatsupgold-sd__[ g a l l e r y - d e v e l ]_________________________
[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]
|
|
Re: Continuation of previous note
Ok, here's a doc containing all the scenarios I can think of:
What scenarios are missing here? On Sun, May 1, 2011 at 7:27 PM, Bharat Mediratta <bharat@...> wrote:
On Sun, May 1, 2011 at 7:26 PM, Tim Almdal <tnalmdal@...> wrote:
Documenting is good.
But I'm confused again... line 115 was in the original code (if its
an album use its id, if its not an album then get its parent id)
isn't doing the same thing as access::can("edit_album",
X) || /* X != album? it comes from parent */
Yeah, this is definitely confusing. In my original email, I was suggesting that we change that around -- but now I'm going to go into Google Docs and write up a comprehensive set of scenarios that I think we should cover, and we can rethink exactly how we want this to work.
Tim
On 5/1/2011 7:09 PM, Bharat Mediratta wrote:
On Sun, May 1, 2011 at 6:31 PM, Tim
Almdal <tnalmdal@...>
wrote:
I'll give your
suggestions a try.
I notice that you're not copying the edit permissions in
the upgrader. Just to verify: we decide that was ok
because "edit" is a superset of "edit_xxx" right?
No, it was my understanding that we didn't copy
edit permissions because I thought we had finally
distilled everything down to the bottom half of this http://gallery.menalto.com/gallery/doc-images/gcon2011/IMG_0162.JPG.html.
and that we felt that edit would work just as it did now.
If the administrator wanted to use the new permissions
that they would have to explicitly chose to enable them.
is your wrapper around _get_all_groups()
really necessary?
using the helper _get_all_groups() was just from
copying code... your right i should be able to just call
identity::groups.
How about we restrict organize to users who have the
"edit" permission? That should be unambiguous.
works for me
For the remainder of this mail I'm going to assume that
we rename edit->edit_album and use edit as an
alias.
Not a safe assumption, at this point, as
mentioned above, I was not coding to (http://gallery.menalto.com/gallery/doc-images/gcon2011/IMG_0161.JPG.html)
but to http://gallery.menalto.com/gallery/doc-images/gcon2011/IMG_0162.JPG.html.
As you can see from that image, we had crossed out the
rename of edit. What's in a name, so it doesn't matter to
me whether we rename it or not.
Those two images are the same, except that the name is
different. But the name is relevant because you're
overloading "edit" to be a function of all three separate
permissions. In your model, we have 3 permissions (edit,
edit_all_photos, edit_my_photos) but access::can("edit")
doesn't actually refer to the edit permission -- it refers to
a function of all three. That is confusing. See my last
email, starting at "I initially had a negative reaction..."
I think that trying to
extend or convert "edit/edit album" permission on albums
that are added by users is a fools errand and destined
to make things complicated. (We might have to
I am not suggesting that. I am merely suggesting that you
rename the permissions to be internally consistent with their
names. We are considering the "edit" permission to have the
effect of "edit this album" and we should name it internally
accordingly. We should not let access::can("edit") be
ambiguous in its meaning.
mess around with the
inheritance portion of our existing permission code). I
think the way to do this is force users to have one of
("edit/edit album", edit_all, or edit_mine) permissions
in addition to the add_album or add_photo permissions.
That way our existing inheritancemodel works naturally.
I really don't like the idea (which I think you are
suggesting) of when an album is added we then create
("edit/edit album") permission for the album. That's a
lot of work and breaks the natural inheritance that we
have now.
Forcing the permissions to relate to each other is
non-trivial, and ultimately not necessary. edit_album refers
to the album as a container and you are granted all the rights
you typically get on a container (which extends to the
contents of the container). edit_mine and edit_all give
you rights on members of the container, but not rights on the
container itself. I don't believe that we have to mess with
the permission inheritance model.
What's currently in the
code, allows inheritance of the "add_photo" and
"add_album" permissions without regard to the edit
permissions. For example you could create an album and
I could add sub-albums and photo's to your album even if
I can't edit them because the add_* permissions where
inherited w/o regard to what my edit permissions are.
Tying them together with the add_* permissions a
sub-permission of the edit* permissions prevents that
blank inheritance.
Sorry, I don't follow what you're saying. Add and edit
permissions are separate in your model, and are separate in my
proposed model. What you say above implies that I'm
suggesting that we tie them together somehow, but I'm not
suggesting that at all. I am merely suggesting that we
consider edit to be an alias for a function of
{edit_album, edit_mine, edit_all}.
This makes our logic easier
because we stop differentiating child albums vs.
photos. To close the loop, I'm going to write it in
pseudocode below so that I'm sure I have it right.
access::can("edit", X) ==
C1 access::can("edit_album", X) || /* X !=
album? it comes from parent */
C2 access::can("edit_all",
X->parent) ||
(
C3
access::can("edit_mine", X->parent) &&
is_owner(X)
)
That's what I believe
I have implemented.
No, what you have is more complicated because it's
differentiating between albums and photos, when in reality
according to the pseudocode I have above, that distinction is
no longer necessary. See:
On that line you're checking to see if it's an album and if
so, you're taking the permission of the parent. I'm saying
that you don't need to do that because the parent's edit_album
will apply to the child if and only if the child has not
overridden it. In my model, if the child overrides
edit_album, then the parent can't edit it. Hmm. Now that I
think about it, this has to be wrong because it would allow
the owner of a sub-album to remove the parent's ability to
edit it, even though the parent has edit_album.
So in my mind,
1) Decide which diagram we are using (I'm assuming 162)
2) Clean up the installer
code especially the _copy_permission code as you suggested
3) Change the add_*
permissions to also require an edit permission (bury that
check in access.php so no code has to change external to
access.php to enforce that)
4) Write some unit tests.
5) Generate the install SQL
And we should be good to go
I don't think that we've converged yet on the right model.
I think that perhaps you misunderstood what I was saying
above about permission inheritance.
I want to get all the scenarios in one place, and email
isn't great for that. I'm going to start a doc which outlines
all the various scenarios so that we can refer to them and
then make sure that we've covered everything. I'll ping back
when that's done..
-Bharat
Tim
On 5/1/2011 12:54 PM,
Bharat Mediratta wrote:
(broadening the
audience to include gallery-devel -- this is a
continuation of a discussion that Tim and I are
having around the branch to fix ticket #1273).
----
Tim,
I notice that you're
not copying the edit permissions in the upgrader.
Just to verify: we decide that was ok because
"edit" is a superset of "edit_xxx" right? In this
diagram:
IIRC, "edit_album" is
the equivalent of what you have as "edit" right
now.. I wonder if we should actually rename the
permission to be "Edit album" to avoid confusion
there.
_copy_permissions is a
PHP based O(N*M) operation where N==the number of
rows and M== the number of groups. That's going to
be really inefficient. Let's get that down to O(M)
using the db builder. It'd be something like:
foreach ($groups as
$group) {
...
db::build()
->from($table)
->set($to_column, db::expr($from_column))
->execute();
}
is your wrapper around
_get_all_groups() really necessary? You're only
calling that from the upgrader and when we get to
that point there had better be an identity provider
active!
More below.
On Sat, Apr 30, 2011
at 6:11 PM, Tim Almdal <tnalmdal@...>
wrote:
Just a little bit
more information, I went and used user1 to add a
photo into user2's album (the waterfall). So now
there is a picture that user2 can't do anything
about.
The other
interesting thing is that user2 can now get the
organize dialog and edit permission dialogs for
their directory. Not sure of this is what we
want.
How about we restrict
organize to users who have the "edit" permission?
That should be unambiguous.
My basic approach
was rather than put all sorts of stuff all over
the place to check edit permissions, I just
changed access.php so that the interface accepts
access::can("edit"...) and it figures out
whether the user has edit, edit_all_photos or
edit_my_photos.
I initially had a
negative reaction to this idea, but I thought it
through and it makes sense. There's an
inconsistency that bothers me, though. Right now
you have 3 perms "edit", "edit_all_photos" and
"edit_my_photos". If you do access::can("edit")
it's not obvious to the caller what's going on
there because there is an edit permission.
If you rename "edit" to "edit_album" (which is
consistent) then "edit" doesn't directly map to a
column and we can consider it an alias
permission. We had those in G2, but they were
heavyweight (using bit vectors, etc). This will
just be a simple alias that you cover in
access::can.
So then we can try to
map this to the diagram we came up with:
For the remainder of
this mail I'm going to assume that we rename
edit->edit_album and use edit as an
alias.
Going through the
diagram, I'm convinced that your code is right for
the root album (A1) and for the photos (P1, P2).
However, I'm not sure that we have A2 correct.
Right now your equality for A2 is:
edit(A2) ==
access::can("edit_album", A2) ||
(
access::can("edit_all_photos",
A1) ||
(
is_owner(A2) &&
access::can("edit_my_photos",
A1)
)
)
I'm not sure if that's
right.. If I grant a user the ability to create
A2 under A1, I would imagine that they would
start off with the edit_album permission -- but
I could see that they wouldn't get that
permission because they don't have edit_album on
the parent so they wouldn't inherit it. But we
still want them to be able to edit the album
they just created, right? I guess that means
that we really do need to treat sub-albums the
same as sub-photos in that case.
Is that logic correct?
If so, then we should rename:
- edit -->
edit_album
- edit_all_photos
--> edit_all
- edit_my_photos
-> edit_mine
This makes our logic
easier because we stop differentiating child
albums vs. photos. To close the loop, I'm
going to write it in pseudocode below so that
I'm sure I have it right.
access::can("edit", X) ==
C1 access::can("edit_album",
X) || /* X != album? it comes from parent */
C2
access::can("edit_all", X->parent) ||
(
C3
access::can("edit_mine", X->parent)
&&
is_owner(X)
)
So that means the
equalities are:
edit(A2) == edit_album(A1) ||
edit_all(A1) || (edit_mine(A1) &&
is_owner(A2))
edit(P2) ==
edit_album(A1) || edit_all(A1) ||
(edit_mine(A1) && is_owner(P2))
Scenarios:
S1: album
creation:
- Admin user grants
only "view" and "add_album" to groups G1
(containing U1), G2 (U2) at the root album
(A0)
- U1 creates album
A1, which inherits permissions from root
album. A1's permissions are now "view" and
"add_album" for G1/G2. U1 cannot edit
A1 even though they own it.
- Admin grants
"edit_mine" to A0
- Now U1 can edit A1
because of permission clause C3 above.
- U2 creates album
A2 and can immediately edit A2 because of step
3 and C3.
S2: photo
addition (continues from scenario S1):
- U1 creates photo
P1A inside A1.
- U1 can edit P1A
because of C1
- U2 cannot add
photos to A1
- U1 grants
"add_photo" to G2 on A1
- U2 now adds photo
P1B to A2
- U2 cannot edit P1B
because he has no edit permissions
- U1 grants
"edit_mine" to G2 on A1
- U2 can now edit
P1B but not P1A (** we're not talking about
delete yet)
- U1 revokes
"edit_mine" from G2 on A1
- U2 can no longer
edit P1B
- U1 grants
"edit_all" to G2 on A1
- U2 can now edit
P1A (which he does not own) and P1B (which he
owns)
Ok. I think I'm
satisfied with that. One thing I noticed is that
you did not implement the aliasing for
group_can(). I don't think that we need to do
that currently, but right now because you haven't
changed the name of the "edit" permission anybody
who calls group_can("edit") is going to get an
unexpected result -- they're essentially getting
group_can("edit_album"), not the alias. Once you
rename the permission, it means that they'll get
an error, which I think is a good thing since
it'll force them to figure out what to do. A
quick grep around gallery3 and gallery3-contrib
indicates that nobody does this outside of the
core code now, so this is a good time to do this.
So to sum up, here's
what I think we need to do:
- Rename edit ->
edit_album
- Rename
edit_all_photos -> edit_all
- Rename
edit_my_photos -> edit_mine
- Replace the
_user_can_edit() logic with what I have above,
and (I suggest) you inline it into user_can()
- Write up all the
above scenarios as unit tests (!!!)
Whew!
No virus found in this
message.
Checked by AVG - www.avg.com
Version: 10.0.1209 / Virus Database: 1500/3608 - Release
Date: 05/01/11
No virus found in
this message.
Checked by AVG - www.avg.com
------------------------------------------------------------------------------
WhatsUp Gold - Download Free Network Management Software
The most intuitive, comprehensive, and cost-effective network
management toolset available today. Delivers lowest initial
acquisition cost and overall TCO of any competing solution.
http://p.sf.net/sfu/whatsupgold-sd__[ g a l l e r y - d e v e l ]_________________________
[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]
|
|
Re: Continuation of previous note
After a length discussion about inheritance issues when a user is
allowed to created albums, Bharat and I have decide to postpone this
fix to a later release.
original ticket:
https://sourceforge.net/apps/trac/gallery/ticket/1273
it should probably be considered when ticket https://sourceforge.net/apps/trac/gallery/ticket/1346
is considered
On 5/1/2011 7:26 PM, Tim Almdal wrote:
Documenting is good.
But I'm confused again... line 115 was in the original code (if
its an album use its id, if its not an album then get its parent
id) isn't doing the same thing as access::can("edit_album", X) || /* X != album? it
comes from parent */
Tim
On 5/1/2011 7:09 PM, Bharat Mediratta wrote:
On Sun, May 1, 2011 at 6:31 PM, Tim
Almdal <tnalmdal@...>
wrote:
I'll give your
suggestions a try.
I notice that you're not copying the edit permissions in
the upgrader. Just to verify: we decide that was ok
because "edit" is a superset of "edit_xxx" right?
No, it was my understanding that we didn't
copy edit permissions because I thought we had finally
distilled everything down to the bottom half of this http://gallery.menalto.com/gallery/doc-images/gcon2011/IMG_0162.JPG.html.
and that we felt that edit would work just as it did
now. If the administrator wanted to use the new
permissions that they would have to explicitly chose to
enable them.
is your wrapper around _get_all_groups()
really necessary?
using the helper _get_all_groups() was just
from copying code... your right i should be able to just
call identity::groups.
How about we restrict organize to users who have the
"edit" permission? That should be unambiguous.
works for me
For the remainder of this mail I'm going to assume that
we rename edit->edit_album and use edit as an
alias.
Not a safe assumption, at this point, as
mentioned above, I was not coding to (http://gallery.menalto.com/gallery/doc-images/gcon2011/IMG_0161.JPG.html)
but to http://gallery.menalto.com/gallery/doc-images/gcon2011/IMG_0162.JPG.html.
As you can see from that image, we had crossed out the
rename of edit. What's in a name, so it doesn't matter
to me whether we rename it or not.
Those two images are the same, except that the name is
different. But the name is relevant because you're
overloading "edit" to be a function of all three separate
permissions. In your model, we have 3 permissions (edit,
edit_all_photos, edit_my_photos) but access::can("edit")
doesn't actually refer to the edit permission -- it refers
to a function of all three. That is confusing. See my
last email, starting at "I initially had a negative
reaction..."
I think that trying to
extend or convert "edit/edit album" permission on
albums that are added by users is a fools errand and
destined to make things complicated. (We might have
to
I am not suggesting that. I am merely suggesting that
you rename the permissions to be internally consistent with
their names. We are considering the "edit" permission to
have the effect of "edit this album" and we should name it
internally accordingly. We should not let
access::can("edit") be ambiguous in its meaning.
mess around with the
inheritance portion of our existing permission code).
I think the way to do this is force users to have one
of ("edit/edit album", edit_all, or edit_mine)
permissions in addition to the add_album or add_photo
permissions. That way our existing inheritancemodel
works naturally. I really don't like the idea (which
I think you are suggesting) of when an album is added
we then create ("edit/edit album") permission for the
album. That's a lot of work and breaks the natural
inheritance that we have now.
Forcing the permissions to relate to each other is
non-trivial, and ultimately not necessary. edit_album refers
to the album as a container and you are granted all the
rights you typically get on a container (which extends to
the contents of the container). edit_mine and edit_all give
you rights on members of the container, but not rights on
the container itself. I don't believe that we have to mess
with the permission inheritance model.
What's currently in the
code, allows inheritance of the "add_photo" and
"add_album" permissions without regard to the edit
permissions. For example you could create an album
and I could add sub-albums and photo's to your album
even if I can't edit them because the add_*
permissions where inherited w/o regard to what my edit
permissions are. Tying them together with the add_*
permissions a sub-permission of the edit* permissions
prevents that blank inheritance.
Sorry, I don't follow what you're saying. Add and edit
permissions are separate in your model, and are separate in
my proposed model. What you say above implies that I'm
suggesting that we tie them together somehow, but I'm not
suggesting that at all. I am merely suggesting that we
consider edit to be an alias for a function of
{edit_album, edit_mine, edit_all}.
This makes our logic easier
because we stop differentiating child albums vs.
photos. To close the loop, I'm going to write it in
pseudocode below so that I'm sure I have it right.
access::can("edit", X) ==
C1 access::can("edit_album", X) ||
/* X != album? it comes from parent */
C2
access::can("edit_all", X->parent) ||
(
C3
access::can("edit_mine", X->parent) &&
is_owner(X)
)
That's what I
believe I have implemented.
No, what you have is more complicated because it's
differentiating between albums and photos, when in reality
according to the pseudocode I have above, that distinction
is no longer necessary. See:
On that line you're checking to see if it's an album and
if so, you're taking the permission of the parent. I'm
saying that you don't need to do that because the parent's
edit_album will apply to the child if and only if the child
has not overridden it. In my model, if the child overrides
edit_album, then the parent can't edit it. Hmm. Now that I
think about it, this has to be wrong because it would allow
the owner of a sub-album to remove the parent's ability to
edit it, even though the parent has edit_album.
So in my mind,
1) Decide which diagram we
are using (I'm assuming 162)
2) Clean up the installer
code especially the _copy_permission code as you
suggested
3) Change the add_*
permissions to also require an edit permission (bury
that check in access.php so no code has to change
external to access.php to enforce that)
4) Write some unit tests.
5) Generate the install SQL
And we should be good to go
I don't think that we've converged yet on the right
model. I think that perhaps you misunderstood what I was
saying above about permission inheritance.
I want to get all the scenarios in one place, and email
isn't great for that. I'm going to start a doc which
outlines all the various scenarios so that we can refer to
them and then make sure that we've covered everything. I'll
ping back when that's done..
-Bharat
Tim
On 5/1/2011 12:54 PM,
Bharat Mediratta wrote:
(broadening the
audience to include gallery-devel -- this is a
continuation of a discussion that Tim and I are
having around the branch to fix ticket #1273).
----
Tim,
I notice that you're
not copying the edit permissions in the upgrader.
Just to verify: we decide that was ok because
"edit" is a superset of "edit_xxx" right? In this
diagram:
IIRC, "edit_album" is
the equivalent of what you have as "edit" right
now.. I wonder if we should actually rename the
permission to be "Edit album" to avoid confusion
there.
_copy_permissions is
a PHP based O(N*M) operation where N==the number
of rows and M== the number of groups. That's going
to be really inefficient. Let's get that down to
O(M) using the db builder. It'd be something
like:
foreach ($groups as
$group) {
...
db::build()
->from($table)
->set($to_column, db::expr($from_column))
->execute();
}
is your wrapper
around _get_all_groups() really necessary? You're
only calling that from the upgrader and when we
get to that point there had better be an identity
provider active!
More below.
On Sat, Apr 30,
2011 at 6:11 PM, Tim Almdal <tnalmdal@...>
wrote:
Just a little bit
more information, I went and used user1 to add
a photo into user2's album (the waterfall). So
now there is a picture that user2 can't do
anything about.
The other
interesting thing is that user2 can now get
the organize dialog and edit permission
dialogs for their directory. Not sure of this
is what we want.
How about we
restrict organize to users who have the "edit"
permission? That should be unambiguous.
My basic approach
was rather than put all sorts of stuff all
over the place to check edit permissions, I
just changed access.php so that the interface
accepts access::can("edit"...) and it figures
out whether the user has edit, edit_all_photos
or edit_my_photos.
I initially had a
negative reaction to this idea, but I thought it
through and it makes sense. There's an
inconsistency that bothers me, though. Right
now you have 3 perms "edit", "edit_all_photos"
and "edit_my_photos". If you do
access::can("edit") it's not obvious to the
caller what's going on there because there is an
edit permission. If you rename "edit" to
"edit_album" (which is consistent) then "edit"
doesn't directly map to a column and we can
consider it an alias permission. We had
those in G2, but they were heavyweight (using
bit vectors, etc). This will just be a simple
alias that you cover in access::can.
So then we can try
to map this to the diagram we came up with:
For the remainder
of this mail I'm going to assume that we rename
edit->edit_album and use edit as an
alias.
Going through the
diagram, I'm convinced that your code is right
for the root album (A1) and for the photos (P1,
P2). However, I'm not sure that we have A2
correct. Right now your equality for A2 is:
edit(A2) ==
access::can("edit_album", A2) ||
(
access::can("edit_all_photos",
A1) ||
(
is_owner(A2) &&
access::can("edit_my_photos",
A1)
)
)
I'm not sure if that's
right.. If I grant a user the ability to
create A2 under A1, I would imagine that they
would start off with the edit_album permission
-- but I could see that they wouldn't get that
permission because they don't have edit_album
on the parent so they wouldn't inherit it.
But we still want them to be able to edit the
album they just created, right? I guess that
means that we really do need to treat
sub-albums the same as sub-photos in that
case.
Is that logic correct?
If so, then we should rename:
- edit -->
edit_album
- edit_all_photos
--> edit_all
- edit_my_photos
-> edit_mine
This
makes our logic easier because we stop
differentiating child albums vs. photos. To
close the loop, I'm going to write it in
pseudocode below so that I'm sure I have it
right.
access::can("edit",
X) ==
C1
access::can("edit_album", X) || /* X !=
album? it comes from parent */
C2
access::can("edit_all", X->parent) ||
(
C3
access::can("edit_mine", X->parent)
&&
is_owner(X)
)
So that means the
equalities are:
edit(A2) ==
edit_album(A1) || edit_all(A1) ||
(edit_mine(A1) && is_owner(A2))
edit(P2) ==
edit_album(A1) || edit_all(A1) ||
(edit_mine(A1) && is_owner(P2))
Scenarios:
S1: album
creation:
- Admin user
grants only "view" and "add_album" to groups
G1 (containing U1), G2 (U2) at the root
album (A0)
- U1 creates album
A1, which inherits permissions from root
album. A1's permissions are now "view" and
"add_album" for G1/G2. U1 cannot
edit A1 even though they own it.
- Admin grants
"edit_mine" to A0
- Now U1 can edit
A1 because of permission clause C3 above.
- U2 creates album
A2 and can immediately edit A2 because of
step 3 and C3.
S2: photo
addition (continues from scenario S1):
- U1 creates photo
P1A inside A1.
- U1 can edit P1A
because of C1
- U2 cannot add
photos to A1
- U1 grants
"add_photo" to G2 on A1
- U2 now adds
photo P1B to A2
- U2 cannot edit
P1B because he has no edit permissions
- U1 grants
"edit_mine" to G2 on A1
- U2 can now edit
P1B but not P1A (** we're not talking
about delete yet)
- U1 revokes
"edit_mine" from G2 on A1
- U2 can no longer
edit P1B
- U1 grants
"edit_all" to G2 on A1
- U2 can now edit
P1A (which he does not own) and P1B (which
he owns)
Ok. I think I'm
satisfied with that. One thing I noticed is
that you did not implement the aliasing for
group_can(). I don't think that we need to do
that currently, but right now because you
haven't changed the name of the "edit"
permission anybody who calls group_can("edit")
is going to get an unexpected result -- they're
essentially getting group_can("edit_album"), not
the alias. Once you rename the permission, it
means that they'll get an error, which I think
is a good thing since it'll force them to figure
out what to do. A quick grep around gallery3
and gallery3-contrib indicates that nobody does
this outside of the core code now, so this is a
good time to do this.
So to sum up,
here's what I think we need to do:
- Rename edit
-> edit_album
- Rename
edit_all_photos -> edit_all
- Rename
edit_my_photos -> edit_mine
- Replace the
_user_can_edit() logic with what I have
above, and (I suggest) you inline it into
user_can()
- Write up all the
above scenarios as unit tests (!!!)
Whew!
No virus found in this
message.
Checked by AVG - www.avg.com
Version: 10.0.1209 / Virus Database: 1500/3608 -
Release Date: 05/01/11
No virus found
in this message.
Checked by AVG - www.avg.com
Version: 10.0.1209 / Virus Database: 1500/3609 - Release Date:
05/01/11
------------------------------------------------------------------------------
WhatsUp Gold - Download Free Network Management Software
The most intuitive, comprehensive, and cost-effective network
management toolset available today. Delivers lowest initial
acquisition cost and overall TCO of any competing solution.
http://p.sf.net/sfu/whatsupgold-sd
__[ g a l l e r y - d e v e l ]_________________________
[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]
No virus found in
this message.
Checked by AVG - www.avg.com
Version: 10.0.1209 / Virus Database: 1500/3609 - Release Date:
05/01/11
------------------------------------------------------------------------------
Achieve unprecedented app performance and reliability
What every C/C++ and Fortran developer should know.
Learn how Intel has extended the reach of its next-generation tools
to help boost performance applications - inlcuding clusters.
http://p.sf.net/sfu/intel-dev2devmay__[ g a l l e r y - d e v e l ]_________________________
[ list info/archive --> http://gallery.sf.net/lists.php ]
[ gallery info/FAQ/download --> http://gallery.sf.net ]
|
