Thread: [PATCH] pgarchives: pglister_sync: import lists with subscriber_access set to True
[PATCH] pgarchives: pglister_sync: import lists with subscriber_access set to True
From
Célestin Matte
Date:
pglister_sync.py is a script used to synchronize things between pglister and pgarchives: lists, subscribers etc. subscriber_access is not set in pglister_sync's query, and is set to null=False in Django's model. As a consequence, pglister_syncfails to add new lists: Traceback (most recent call last): File "/srv/pgarchives/local/loader/pglister_sync.py", line 68, in <module> 'groupname': l['group']['groupname'], psycopg2.errors.NotNullViolation: null value in column "subscriber_access" violates not-null constraint DETAIL: Failing row contains (8, test-pglister-sync, test-pglister-sync, , t, 1, null). I don't know if there is a way to configure postgres to use the default value , but I think it would be wiser to explicitlyset this variable in pglister_sync.py. By default, subscriber_access is set to False and there is no way to modify that within the web interface. As a consequence, access to lists on private servers is restricted to superusers, and there is no easier way to modify thatthan to edit the database manually. It seems more logical to me that this value be set to True by default, as access can still be moderated to avoid lists beingpublicly available. That said, it may be better to have a way to modify that within the web interface in pglister. -- Célestin Matte
Attachment
Re: [PATCH] pgarchives: pglister_sync: import lists with subscriber_access set to True
From
Célestin Matte
Date:
Sorry, accidentally sent unfinished email containing contradictory information... Please ignore the previous one. pglister_sync.py is a script used to synchronize things between pglister and pgarchives: lists, subscribers etc. subscriber_access is not set in pglister_sync's query, and is set to null=False in Django's model. As a consequence, pglister_syncfails to add new lists: Traceback (most recent call last): File "/srv/pgarchives/local/loader/pglister_sync.py", line 68, in <module> 'groupname': l['group']['groupname'], psycopg2.errors.NotNullViolation: null value in column "subscriber_access" violates not-null constraint DETAIL: Failing row contains (8, test-pglister-sync, test-pglister-sync, , t, 1, null). I don't know if there is a way to configure postgres to use the default value , but I think it would be wiser to explicitlyset this variable in pglister_sync.py (attached patch in previous email) It seems more logical to me to set this value to True, as otherwise access to lists on private servers is restricted to superusers,and there is no easier way to modify that than to edit the database manually. But then, the model may need tobe update to have this value set to default=True as well. What do you think? That said, it may be better to have a way to modify that within the web interface in pglister. -- Célestin Matte
Re: [PATCH] pgarchives: pglister_sync: import lists with subscriber_access set to True
From
Magnus Hagander
Date:
On Fri, Jan 28, 2022 at 6:37 PM Célestin Matte <celestin.matte@cmatte.me> wrote: > > pglister_sync.py is a script used to synchronize things between pglister and pgarchives: lists, subscribers etc. > > subscriber_access is not set in pglister_sync's query, and is set to null=False in Django's model. As a consequence, pglister_syncfails to add new lists: > Traceback (most recent call last): > File "/srv/pgarchives/local/loader/pglister_sync.py", line 68, in <module> > 'groupname': l['group']['groupname'], > psycopg2.errors.NotNullViolation: null value in column "subscriber_access" violates not-null constraint > DETAIL: Failing row contains (8, test-pglister-sync, test-pglister-sync, , t, 1, null). > > I don't know if there is a way to configure postgres to use the default value , but I think it would be wiser to explicitlyset this variable in pglister_sync.py. Interesting. Yes there is, set the columns default to false. Which is what is running on both our production servers for postgresql.org -- but I'm unsure how it actually got there given that Django is dumb enough not to propagate that down to the database, assuming all access will forever be through the ORM. We must've done some manual step when deploying that, that has since been forgotten. This should definitely be fixed. > By default, subscriber_access is set to False and there is no way to modify that within the web interface. > As a consequence, access to lists on private servers is restricted to superusers, and there is no easier way to modifythat than to edit the database manually. > > It seems more logical to me that this value be set to True by default, as access can still be moderated to avoid listsbeing publicly available. In what way would access "still be moderated"? In pgarchives, that's a pure boolean and there are no further checks. User accounts are auto-created. The idea is that anything that's "open" should have to be set explicitly and thus we should default to it being off. Based on that I have at least initially applied a version of your patch that sets it to false. > That said, it may be better to have a way to modify that within the web interface in pglister. I agree in principle. The argument does fall off a bit on the fact that there is *no* admin interface to pgarchives. You don't have a way to add a list manually either, without doing it directly in SQL. So we either accept that SQL is the way things are done, or we should tackle the bigger problem of setting up such an interface. But I think we could get pretty far by just enabling the general django admin interface and set up the required classes for that -- we don't necessarily need to move things like reparsing and hiding of messages into such an admin interface. -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
Re: [PATCH] pgarchives: pglister_sync: import lists with subscriber_access set to True
From
Célestin Matte
Date:
> This should definitely be fixed. Thanks! >> By default, subscriber_access is set to False and there is no way to modify that within the web interface. >> As a consequence, access to lists on private servers is restricted to superusers, and there is no easier way to modifythat than to edit the database manually. >> >> It seems more logical to me that this value be set to True by default, as access can still be moderated to avoid listsbeing publicly available. > > In what way would access "still be moderated"? In pgarchives, that's a > pure boolean and there are no further checks. User accounts are > auto-created. I meant that subscriptions can still be moderated in pglister (if lists are configured that way), so that anybody does nothave access to archives. > The idea is that anything that's "open" should have to be set > explicitly and thus we should default to it being off. Based on that I > have at least initially applied a version of your patch that sets it > to false. That makes sense. >> That said, it may be better to have a way to modify that within the web interface in pglister. > > I agree in principle. The argument does fall off a bit on the fact > that there is *no* admin interface to pgarchives. You don't have a way > to add a list manually either, without doing it directly in SQL. So we > either accept that SQL is the way things are done, or we should tackle > the bigger problem of setting up such an interface. But I think we > could get pretty far by just enabling the general django admin > interface and set up the required classes for that -- we don't > necessarily need to move things like reparsing and hiding of messages > into such an admin interface. I meant this could be added in the admin interface of pglister, not pgarchives, as it already exists and pglister_sync canthen push (and update) the configuration to pgarchives. -- Célestin Matte
Re: [PATCH] pgarchives: pglister_sync: import lists with subscriber_access set to True
From
Magnus Hagander
Date:
On Wed, Feb 2, 2022 at 9:16 AM Célestin Matte <celestin.matte@cmatte.me> wrote: > > > This should definitely be fixed. > > Thanks! > > >> By default, subscriber_access is set to False and there is no way to modify that within the web interface. > >> As a consequence, access to lists on private servers is restricted to superusers, and there is no easier way to modifythat than to edit the database manually. > >> > >> It seems more logical to me that this value be set to True by default, as access can still be moderated to avoid listsbeing publicly available. > > > > In what way would access "still be moderated"? In pgarchives, that's a > > pure boolean and there are no further checks. User accounts are > > auto-created. > > I meant that subscriptions can still be moderated in pglister (if lists are configured that way), so that anybody doesnot have access to archives. Ah, right. That still doesn't fully solve the problem though -- because once somebody has been approved in moderation, they automatically get access to everything on the list from before they were added. Which may not be what's wanted -- we have cases where we're archiving things for "no regular use", but leave it for superusers to be able to go look things up in an emergency for example. In that usecase, it's not tied to the subscription at all. > > The idea is that anything that's "open" should have to be set > > explicitly and thus we should default to it being off. Based on that I > > have at least initially applied a version of your patch that sets it > > to false. > > That makes sense. > > >> That said, it may be better to have a way to modify that within the web interface in pglister. > > > > I agree in principle. The argument does fall off a bit on the fact > > that there is *no* admin interface to pgarchives. You don't have a way > > to add a list manually either, without doing it directly in SQL. So we > > either accept that SQL is the way things are done, or we should tackle > > the bigger problem of setting up such an interface. But I think we > > could get pretty far by just enabling the general django admin > > interface and set up the required classes for that -- we don't > > necessarily need to move things like reparsing and hiding of messages > > into such an admin interface. > > I meant this could be added in the admin interface of pglister, not pgarchives, as it already exists and pglister_synccan then push (and update) the configuration to pgarchives. Ah, then I understand. That's an interesting aspect. Right now, pglister has no knowledge of what actually runs on the archiving side other than "send the emails over here" and "create links that look like this". In general, I like that level of disconnect -- it should be possible to swap out the archive solution from underneath it, or indeed run the same pglister instance against multiple different types of archives. -- Magnus Hagander Me: https://www.hagander.net/ Work: https://www.redpill-linpro.com/
Re: [PATCH] pgarchives: pglister_sync: import lists with subscriber_access set to True
From
Célestin Matte
Date:
Attached another proposed solution to that problem, with a series of patches for pglister and pgarchives: - 0001-Add-subscriber_access-field-to-ListGroup_pglister.patch adds a subscriber_access field in pglister's admin section - 0002-Add-subscriber_access-to-archives-API_pglister.patch adds subscriber_access to the API's archive section in pglister - 0001-pglister_sync-obtain-subscriber_access-from-API_pgarchives.patch uses the value received from the API instead of adefault "False" that has to be manually changed As a reminder, the problem was that subscriber_access was set to False by default, which means that any list created on pglisterand set to be archived on pgarchives won't be reachable by subscribers, unless the subscriber_access field is manuallymodified in the database. -- Célestin Matte
Attachment
Re: [PATCH] pgarchives: pglister_sync: import lists with subscriber_access set to True
From
Célestin Matte
Date:
Update: previously attached patches were incorrectly using subscriber_access in ListGroup, although it's a field of Listin pgarchives. Besides, they were not built on top of upstream. Please note that this set of patches is built on top of the patch I sentin gitlab [1], as upstream currently crashes on migration. New sets of patched attached. [1] https://gitlab.com/pglister/pglister/-/merge_requests/33 On 28/02/2023 11:39, Célestin Matte wrote: > Attached another proposed solution to that problem, with a series of patches for pglister and pgarchives: > - 0001-Add-subscriber_access-field-to-ListGroup_pglister.patch adds a subscriber_access field in pglister's admin section > - 0002-Add-subscriber_access-to-archives-API_pglister.patch adds subscriber_access to the API's archive section in pglister > - 0001-pglister_sync-obtain-subscriber_access-from-API_pgarchives.patch uses the value received from the API instead ofa default "False" that has to be manually changed > > As a reminder, the problem was that subscriber_access was set to False by default, which means that any list created onpglister and set to be archived on pgarchives won't be reachable by subscribers, unless the subscriber_access field ismanually modified in the database. -- Célestin Matte
Attachment
Re: [PATCH] pgarchives: pglister_sync: import lists with subscriber_access set to True
From
Célestin Matte
Date:
On 05/02/2022 18:24, Magnus Hagander wrote: > In general, I like that level of disconnect -- it should be possible > to swap out the archive solution from underneath it, or indeed run the > same pglister instance against multiple different types of archives. Re-reading this thread, I now understand that the patches I sent later on did not satisfy this requirement. Having no way to give access to without resorting to SQL does not seem ideal to me, though. Should I push a patch to activatethe admin interface on pgarchives, as mentioned earlier in the thread? -- Célestin Matte