[pve-devel] [PATCH access-control/manager v2] fix #3668: improving realm sync

[Thomas Lamprecht]

On 22.03.22 16:23, Dominik Csapak wrote:
> On 3/22/22 14:44, Thomas Lamprecht wrote:
>> On 22.03.22 07:11, Thomas Lamprecht wrote:
>>> On 04.02.22 15:24, Dominik Csapak wrote:
>>>> this deprecates the 'full' sync option and replaces it with
>>>> a 'mode' option, where we add a third one that updates
>>>> the current users (while retaining their custom set attributes not
>>>> exisiting in the source) and removing users that don't exist anymore
>>>> in the source
>>>>
>>> I'm not yet 100% sure about the specific mode names, as sync normally means
>>> 100% sync, I'll see if I find some other tool (rsync?) with similar option naming
>>> problems. Independent from the specific names, this really needs a docs patch,
>>> ideally with a table listing the modi as rows and having the various "user added",
>>> "user removed", "properties added/updated", "properties removed" as columns, for a
>>> better understanding of the effects..
>>>
>> A thought (train): what we decide with this isn't what gets added/updated, that's
>> always the same, only what gets removed if vanished on the source, so maybe:
>>
>> remove-vanished: < none | user | user-and-properties >
>>
>> Or if we can actually also remove either user *or* group then: s/user/entity/ ?
>>
>> ps. the web interface should probably do a s/Purge/Purge ACLs/ too; or with that
>> in mind we could actually drop that do and have:
>>
>> remove-vanished: < none | user | user-and-properties | user-and-properties-and-acl >
>>
>> And with that, we could go the separate semicolon-endcoded-flag-list like we do for
>> some CT features (or mount options) IIRC:
>>
>> remove-vanished: [<user>];[<properties>];[acls]
>>
>> I.e., those three flags would replace your new mode + purge like:
>>
>> +--------+--------+---------------------+
>> |  Mode  | Purge  | -> removed-vanished |
>> +--------+--------+---------------------+
>> | update |      0 | "" (none)           |
>> | sync   |      0 | user                |
>> | full   |      0 | user;properties     |
>> | update |      1 | acl                 |
>> | sync   |      1 | acl;user            |
>> | full   |      1 | acl;user;properties |
>> +--------+--------+---------------------+
>>
>> The selector for them could be either three check boxes on one line (similar to the
>> privilege level radio buttons from CT restore) or even a full blown combobox with all
>> the options spelled out.
>>
>> It's only slightly weird for acl, as there the "remove-vanished" somewhat implies that
>> we import acl's in the first place, if we really don't want that we could keep
>> "Purge ACLs" as separate option that is only enabled if "remove-vanished" "user" flag
>> is set, put IMO not _that_ of a big problem to understand compared to the status quo.
>>
>> Does (any of) this make sense to you?
> 
> yes this sounds sensible, but i agree about the possibly confusing 'remove-vanished'
> implication for acls. Maybe 'remove-on-vanish' ?

sounds the same to me semantically, so see no improvement there.

> this would (semantically) decouple the 'vanished' thing from the 'removed' thing,
> at least a little bit.

IMO purely subjective and if a real grammar/semantic connection would be there that
I just miss (always a possibility) it'd be to subtle.

I think that the confusion potential overall would get quite a bit reduced that getting
this slightly confusing one newly is still a net benefit and can be easily defused with
a short docs note.

> in either case the docs would have to be updated anyway (as you already said)
> 
> aside from that, i think line 4 in your table is not really practical,
> since it would remove the acls but leave the users ?

The user cannot do anything anymore (like auto-disable) but you still have a reference
to it and all its configured fields + TFA, either  to re-enable it later or to check
contact info if one would investigate a specific task, so IMO its still practical for
setups that want to auto-disable but not auto-remove.