This element could do with an explanation that it can't be changed on existing flags.

CommentFileSizeAuthor
#9 2815095-9.flag_.Add-description-to-Scope-field-to-explain-why-its-disabled.patch850 bytesjoachim
#2 2815095-2.flag_.Add-description-to-Scope-field-to-explain-why-its-disabled.patch442 bytesjoachim
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented
Status: Active » Needs review
FileSize
2815095-2.flag_.Add-description-to-Scope-field-to-explain-why-its-disabled.patch442 bytes
martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented

The patch looks good as it documents, the current form behaviour

but I note that programmatically Flag::setGlobal() is public on the interface, so I expect global can be toggled after Flag::create() is called.

The expected behaviour has some cache invalidation implications in relation to an upcoming change to

FlagCountManager::getUserFlagFlaggingCount()

#2832106: identify different anonymous users by storing session id on Flaggings

that we have been discussing.

Do you wish to make isGlobal() a locked down value - set during construction?

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented

> but I note that programmatically Flag::setGlobal() is public on the interface, so I expect global can be toggled after Flag::create() is called.

The flag entity is actually created in EntityForm::getEntityFromRouteMatch(), which is called when the form is loaded. So the global property is set on it once it already exists. So locking global down might not be that simple.

At any rate, this issue is about the a UI, where the global property is already locked down.

Locking it down programmatically makes sense but not a priority -- file it so we don't forget it?

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Thanks for supplying the wider perspective.

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented

Locking it down programmatically makes sense but not a priority -- file it so we don't forget it?

I can't conceive of a use case that would ever need to toggle the global state, but I also don't see it as a loophole that MUST be closed.

I can only see broad security concerns that I am not worried about

along the lines of... Can a hacker gain advantage by toggling the state - which combined with other code changes subtly makes the site disclose anothers users session_id.
but if hackers can alter php code the there are much bigger security concerns.

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented
Title: Add description to Scope field to explain why it's disabled » Add description to Scope field to explain it will be / why it is disabled
Assigned: Unassigned » joachim
Status: Reviewed & tested by the community » Needs work

Thanks for the review... but it occurs to me that we should also warn on the add flag form that this is a one-time setting. I'll do another patch.

> I can't conceive of a use case that would ever need to toggle the global state, but I also don't see it as a loophole that MUST be closed.

I someone changes that setting, their flagging data will stop making sense. Going from global to personal will be ok (assuming we now record a uid for global flags, which I think we do), but going from personal to global will be a mess.

martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented

I someone changes that setting, their flagging data will stop making sense. Going from global to personal will be ok (assuming we now record a uid for global flags, which I think we do), but going from personal to global will be a mess.

Thanks for talking this through

So thinking about

#2832106: identify different anonymous users by storing session id on Flaggings

That mess is what I lightly called 'cache invalidation' issues in #3 with regard to getUserFlagFlaggingCount()

So my realization is that we either MUST lockdown or MUST invalidate.

As the session_id is always recorded for the anonymous case then toggling global would lead to a reinterpretation of entries in the database.

Again we are talking about our understanding of the module as we have or will code it .... we are far from a useful application of this toggle.-

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented
Assigned: joachim » Unassigned
Status: Needs work » Needs review
FileSize
2815095-9.flag_.Add-description-to-Scope-field-to-explain-why-its-disabled.patch850 bytes
martin107’s picture

martin107 CreditAttribution: martin107 as a volunteer commented
Status: Needs review » Reviewed & tested by the community

works for me

joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented

> As the session_id is always recorded for the anonymous case then toggling global would lead to a reinterpretation of entries in the database.

Yup, toggling global breaks stuff. Like I say, I'm happy for us to add something to guard against that, but equally, if you do poking into the internals of flags like that, you invalidate your warranty :p

I mean, we can't protect the user against EVERYTHING -- hacking their DB tables by hand, putting large magnets next to their server, etc etc...

  • joachim committed b1bc269 on 8.x-4.x 
    Issue #2815095 by joachim: Added description to Scope field to explain...
joachim’s picture

joachim CreditAttribution: joachim as a volunteer commented
Status: Reviewed & tested by the community » Fixed

Thanks for the review. Committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.