search

zopefoundation / Products.PluggableAuthService

Pluggable Zope authentication / authorization framework
Other
9 stars 18 forks source link

Add new event to be able to notify group deletion. #3

Closed DieKatze closed 8 years ago

jensens commented 8 years ago

Looks fine to me and the feature is very useful. Any objections against a merge?

gbastien commented 8 years ago

Hi @jensens,

this has also be reviewed internally by different developers (@laulaz), I am +1 for merging ;-)

Gauthier

mauritsvanrees commented 8 years ago

Looks good. Merged, thanks!

davisagli commented 8 years ago

Why is a new event needed instead of notifying PrincipalDeleted also when a group is removed? Groups and users are both principals.

gbastien commented 8 years ago

Hi @davisagli

we indeed wanted to use this event for user and group deletion but looking at packages using this event, we found plone.app.contentrules use it for the "User removed" rule... And maybe other packages in the collective or so are using the PrincipalDeleted event only for detecting user deletion... That is why we decided to add another event even though PrincipalDeleted should be used in both cases, or should be renamed to UserDeleted for sanity...

Gauthier

laulaz commented 8 years ago

I would also add that even functions docstrings are all referring specifically to User deletion (instead of Principal).

davisagli commented 8 years ago

That's a good point from a backwards-compatibility perspective; I agree with your approach. I do think it would be nice to deprecate the PrincipalDeleted name and change it to UserDeleted, to avoid confusion.

jensens commented 8 years ago

The idea of the abstract Principal as a base class for User and Group is ok. I would keep the PrincipalDeleted as is, but derive UserDeleted and GroupDeleted from it. This may need changes in Plone core to reflect it, but imo its worth the effort in order to have a clear naming.

gbastien commented 8 years ago

Hi @jensens @davisagli

OK, we will work on this next week, and update this PR, do we try to keep backward compatibility or not? Indeed notifying GroupDeleted event will be catched by subscribers using IPrincipalDeletedEvent... Or do we raise/log if Principal event is used to warn user?

mauritsvanrees commented 8 years ago

Don't update this pull request: it has already been merged. Create a new one.

gbastien commented 8 years ago

@mauritsvanrees right ;-) but what about the backward compat part? The problem resides that notifying GroupDeleted event will also be catched by subscribers using IPrincipalDeletedEvent... Or am I wrong?

mauritsvanrees commented 8 years ago

I think it is best if GroupDeleted and UserDeleted inherit from PrincipalDeleted. Same for their interfaces. Code that currently listens for IPrincipalDeleted events, will then get both events. It's a bit hard to predict if that will cause problems in some packages, but it seems the most logical approach to me.

gbastien commented 8 years ago

@mauritsvanrees ok thank you, we will go for that.