Introduce ACLs for menu entries

[kewisch]

Hey @ThiefMaster, I'd love to get some feedback on this approach, as it is turning out to be massively more complicated than I initially expected. There are still quite a few loose ends, but I want to see if I am going into the right direction.

Is your feature request related to a problem? Please describe.
We sometimes have sponsored attendees at our events. We'd like to show them a page in the sidebar with the expense policies. We don't want this page to be visible to everyone else.

Describe the solution you'd like
I'd like Menu Entries to have ACLs so I can restrict viewing the and their visibility in the menu to people who have registered using a specific form.

Describe alternatives you've considered
Right now, all we can do is send them the link to an unlisted page, or open up the expense policy page to everyone (non-sponsored attendees, random remote participants)

Current Status
I've hooked up UI using the permission list field and have taken a stab at the backend database. A few features are still missing:

• An explicit way to make it available to "Everyone" - I was thinking to either do so if the ACLs are empty, or add another token principal "Everyone" that needs to be explicitly added.
• Protection mode - I'll have to play around with this. I see a few ways protection modes could map to different scenarios, it could also be a way to avoid a specific "Everyone" principal.
• I've not yet managed to get saving settings to work due to some backref issues, it isn't finding the menu entry from the principal.
• Need to add a speakers button to the UI that uses the new principle.
• Probably makes sense to hook up event and category roles as well
• There are a few functions that need to be implemented for completeness, though I've not yet seen if they are used in this scenario.

Questions:

• Where I am struggling most is the database setup and fields. I've seen multiple ways to do this in the codebase (sqlalchemy schemas vs flask sqlalchemy), and there is no 1:1 comparison I can make. Am I missing anything substantial? Especially in the backrefs, I'm not sure when I need a db backref, a normal backref, and what properties I need to implement for a backref.
• I've given the speaker principal an id, which is the event id. I think we could do without and just make this a simple token principal, the event data being taken from context. Makes sense? I'd still need to have something in SpeakerPrincipal so that I can reference the event though.
• I'm using theProtectionMixin since it provides many convenience functions. Is this the right path, or is it going overboard for a simple yes/no type of access?

Any other comments you might have are appreciated.

Screenshots

[ThiefMaster]

ProtectionMixin (you also want the option to set it as protected or inheriting) makes perfect sense here. About the special "speaker" principal I'm not sure what's the best solution here, since you can have only one such entry for an ACL. OTOH, being able to grant access to speakers may be interesting in other places as well. I have to think about what's the best approach. What comes to my mind first is just using a column for this special kind of access (ie what we have right now, but to grant access to speakers instead of restricting it to them) and when it's protected we check that plus the ACL.

I'll think a bit about it and get back to you one of these days (I'm off today and kind of busy until at least wednesday, so likely later this week).

[ThiefMaster]

So after having given this some thought, I think making it an Principal type in the backend is not a good option, because all other principal types reference something, but this one doesn't have anything useful to reference.

I think using a column but integrating it more nicely with the access check logic is the best way to go. You could implement this in ProtectionMixin. Check how I implemented e.g. allow_access_key: Only when this is set on the class level will the column be added to the model, so you can add generic code for this while keeping it opt-in.

And to keep this plugin from becoming overly complex I'd limit it to:

• adding this new functionalikty
• adding ACL (and inheriting/protected protection modes) for custom pages
• adding a migration step to update the old data, ie marking custom pages as protected when they had restrictions before and converting those old restrictions accordingly: set the new flag to give speakers access if the access was set to MenuEntryAccess.speakers, or add all registration forms from the event to the ACL in case it was set to MenuEntryAccess.registered_participants

for the migration step you probably want to include this in the same one that also applies the database structure changes so you can first add the new columns and acl table, then migrate the data, and then delete the old unused column (and vice versa in case of downgrade).

[kewisch]

What I am worried about is that this creates some hacks for PrincipalListField. The field's data is a list of principals, if we have a non-principal field here then we'd need to adjust those to convey both a list of principals, and a list of toggles such as allowing speakers. I'll investigate further if there is an elegant way to do this.

[kewisch]

Looks like it won't be too much of an issue, just need to adjust the data structure sent in the form and process it correctly. I have some questions I'll ask inline in a sec.

[kewisch]

This question would be good to answer. It sounds like this code might predate the ProtectionMixin, since it is trying to be smart on its own. Shouldn't this just be checking disallowed protection modes on the object itself?

[ThiefMaster]

It's not enough because an event can typically have inheriting protection except if it's an unlisted event (which has no category to inherit from).

[kewisch]

I'd argue that this should rather be in the protection mixin, maybe with a property effective_disallowed_protection_modes, or renaming the property already defined on the class to _disallowed_protection_modes . IIUC when an event is unlisted, it has no protection parent, so the extra check on it being an unlisted even is redundant and this should work:

@property
def effective_disallowed_protection_modes(self):
    if self.protection_parent:
        return self.disallowed_protection_modes
    else:
        return self.disallowed_protection_modes - {ProtectionMode.inheriting}

I imagine any additional use of disallowed_protection_modes would need to reproduce the parent check logic, but given it is the only use right now I don't need to change it in this PR. Let me know what you prefer.

[ThiefMaster]

i'd keep it simple in the PR. refactoring and features in the same PR usually make things harder to review

[kewisch]

@ThiefMaster in exploring how where to add the "Speakers" option in the UI, I'm wondering if it makes sense to expose all of the built-in event roles in the Event Roles dropdown? This would add "Speaker", "Author", "Convener", and maybe "Chairperson".

I could do that as a special case where the built-in event roles are just added to the dropdown next to the custom roles in the UI, or I could make some (to be explored) changes to the principal to accept special values for the builtin event roles and also retrieve them from the API.

What do you think?

[ThiefMaster]

TBH I'd keep this PR simple and first of all add it with a separate form field... and then consider improvements in a separate PR.

"Event Roles" are user-defined roles (basically event-scoped groups) so I would not mix those.

FWIW for improvements I'd consider UI similar to what we have on the event protection page and then maybe have grayed-out lines for special things like speakers, which would become active when you tick them to enable them. But that'd be more of a bigger task to convert all the ACL related UI fields to a nice react-based widget.

[kewisch]

I came to this conclusion since on the "Participant Roles" view in the event there is a mix of the custom event roles and the built-in roles like speaker and author. So there is already a mix going on there? While they are technically separate, from a product point of view they are very much the same.

If we don't want to do a major revamp now, what I'd recommend is:

• Have an allow_speaker option in the protection field
• If this field is enabled, then when building the Event Roles dropdown, add a built-in static row for speakers
• If the user selects this special event role, it gets added to the top in similar styling as the event roles ( box with SPK in it, same color as in participant roles view )
• When the data for the widget is serialized, it sees that special event role and converts it to a separate toggle, like this: { 'flags': { 'speakers': true }, 'principals': [...serialized principals list that was there before...] }
• Server side separates flags and principals and updates the two database fields accordingly.

If you'd like to go with a completely separate BooleanField toggle in this PR that works for me, but I would really like to integrate it into the protection widget because it seems more elegant, so I would tackle that as the very next thing. I'd love to do something that isn't as elaborate as completely revamping the ACL UI, but still would integrate speakers into the widget we have.

[ThiefMaster]

I think the participant roles is special since this isn't really access control related. I'm very much against mixing event roles and other things in the same dropdown.

So in this PR I'd go for the completely separate toggle.. There's a good chance I'll merge this PR only after branching off to start development for 3.3 in master, so merging it into master in this "kind of intermediate" (yet stable) state wouldn't be a problem at all - and it would avoid a harder-to-review PR!

[kewisch]

Ok, very well. I did notice after commenting the fancy boxes and icons from the event protection widget aren't in this one, so it would indeed make them hard to differentiate. I should be able to complete this in the next few days, all I have left is the speaker toggle, testing the data migration more elaborately, and some tests.

[kewisch]

@ThiefMaster this one should be ready for review now. Do we do unit tests for migrations in some way? The downgrade seems a bit more complex. Also if you could provide some guidance on the exporters, I really don't know what I am doing there and if it is correct.

[ThiefMaster]

No good way to test migrations in unit tests... :/

TBH for something this complex I usually go for the easy way and don't try to do it in pure SQL but simply query it and then update the data using Python code (all the places with a context.is_offline_mode() check).

Anyway, I'll giving your migration code a try! If it works in pure SQL even better ;)

[ThiefMaster]

In case you are curious about the things I changed, I split them into many individual commits with descriptive messages.

[ThiefMaster]

Also if you could provide some guidance on the exporters, I really don't know what I am doing there and if it is correct.

Since you asked about how the export stuff works some time ago: I'll add some comments below to explain it

[ThiefMaster]

And table row matching this condition won't be exported. This is done to get rid of anything that is not portable between instances (category roles and groups) - only events and users are included in exports.

[ThiefMaster]

This ensures ordering in the export file: registration forms and event roles must be imported first, because it's impossible to create the menu entry principals without those if they are referenced (they can't be set lazily later, because an event role principal cannot have null event role id)

[ThiefMaster]

This triggers the export if MenuEntryPrincipal objects if they are referenced in a MenuEntry that has been exported

[ThiefMaster]

When the user chooses to not import missing users during the event import, then this skips the MenuEntryPrincipal object altogether (it's pointless to import a user acl entry if that user does not exist on the target indico instance)