No problem! Glad the disallowed_groups will be helpful. I’ve merged that PR now.
I need to take a pass through all our official themes & components now that we have disallowed_groups and resolve_group_memberships available, I suggest you and @moin do the same when you can for your own themes and components, as after I’ve made changes to our official repos I would really like to move forward with making the upcoming change from the OP stable .
There is a lot of other core work now relying on/using anonymous_users and logged_in_users and I’d really like to delete the everyone group.
i just did a full update of my instance, and i am now adding the disallowed_groups object setting to my component for everyone and anonymous_users based on the auto group ids here:
what am i doing wrong in the object setting? i notice that even without the disallowed_groups in the objects, it still doesn’t show the anonymous_users group (so at the moment, there is no difference between the list in my component whether i set disallowed_groups or not). i tested with other group ids and i must be doing something wrong with how i am using disallowed_groups (or syntax) because it doesn’t seem to have any effect regardless of which ones i use.
Oh there were some GitHub issues earlier in the day, so only just now has disallowed_groups change worked its way into latest Commits · discourse/discourse · GitHub
Not completely sure that will be the issue, but can you try updating again and see if it persists? If not LMK and point me to your theme component (or is it just your group sidebar one?) so I can debug
i opened a quick PR to add those two classes (anonymous_users and logged_in_users) to the theme component, but i don’t think it is really necessary (see my note below).
i haven’t really tested it (lol) but i think it’s pretty straight-forward. the code just checks if the current user exists, and if they do, then they are a member of logged_in_users, and if not, then they are anonymous_users.
note: you can do this sort of anon/logged in css without that component. i’m pretty sure Discourse automatically adds .anon anyways, so the css for anonymous vs logged in can be achieved without installing the component, but the PR simply adds code to use the new group conventions.
I’m working on a plan for next steps of this upcoming change now, I think there are still some places in the core/plugin codebases looking at everyone directly or not using user.in_any_groups? on the server side.
Is there a reason why this hasn’t been merged yet?
The reason I opened this topic is that I finally updated my component
I did that. But I still have the impression that it helps only for forums adding the component after I changed this. On those already using it, the new default isn’t applied (which is usually good!). So I still see the problem that there is an unexpected change in behavior for those already using the component.
Also do you know what happens if an admin configured a setting with a group which I add as a disallowed group in my update?
Since I don’t think the component is used on many forums I wasn’t too worried and merged anyways, but both the migration and setting disallowed groups might also be relevant to other theme developers
Nope, for some reason I guess my brain thought this was not a PR on the discourse org Will merge shortly after the CI checks run.
For cases like these and others in future, I think the best way is probably writing a migration on a per theme/component basis Migrate Discourse theme settings
I asked when this would have to happen in my post above. Migrating without being sure the new groups work on all forums could break stuff too, and admins can toggle the change on and off. So it feels impossible to migrate at the right time.
Apologies if this was already mentioned, and I missed it…
I realized, if resolve_group_membership is included in the settings, I can only access the boolean value via the user_in_ prefix, but no longer can access the raw value of the setting field.
aabbccdd_allowed_groups:
refresh: true
default: "1|2"
type: list
list_type: group
resolve_group_membership: true
console.log(settings.aabbccdd_allowed_groups); // undefined
console.log(settings.user_in_aabbccdd_allowed_groups); // true or false
I think so. Otherwise, the solution to this bug could have been different.
It also makes sense to me. user_in_x also checks groups the frontend doesn’t know about, because the group is only visible to admins or its visibility is limited by default, like everyone. So you get different results depending on what you use, so combining the two could have unexpected consequences.
Thanks Moin, this is exactly right. @gormus the only place that the actual group IDs come through still is in the admin UI for the theme settings.
Not sure if this is clear based on what I’ve posted before, but the anonymous_users and logged_in_users are usable at any time without this upcoming change being enabled, I added them independently months ago. This is the main thing the upcoming change does:
So either way, you are safe if you get rid of everyone everywhere it’s used and only use anonymous_users and logged_in_users from now on.
Today I plan to figure out a plan of the remaining work I need to do to truly get rid of everyone for site settings (leaving category settings alone at the moment), and I will post it here, so hopefully that will help stay on the same page going forward, and I can keep updating this plan as I go.
But the groups aren’t visible in the interface if the change is disabled. Admins cannot change a setting to those groups. So when I add “everyone” as a disallowed_group, they no longer see any groups which allow them to set up a component to be visible for visitors. For me, “usable” implies not only working but also being visible. Since it’s still possible to disable the change, I wouldn’t call relying only on the new groups “safe”.
Hmm you’re right. There are a couple more things I need to fix when the upcoming change is disabled for this disallowed_group problem to go away:
anonymous_users and logged_in_users aren’t in the group picker at all. I think it’s safe to allow these here now, then it won’t matter if you’ve added everyone to disallowed_groups if the upcoming change is turned off.
Fix Guardian::AnonymousUser#in_any_groups? to respect anonymous_users with the upcoming change turned off.
Add the same read time aliasing of 0 (everyone) → 5 (logged_in_users) for theme settings, which we do for site settings.
I think I might also mark everyone with (legacy) in the group selector(s) while the upcoming change is still optional, and disabled.
Will prioritise these ones, and add them to my overall plan I’m working on.
I’ve made a dedicated topic here @moinThe road to stable, then permanent, for granular_anonymous_and_logged_in_groups_permissions . The OP is incomplete, I’m still going through all the cases locally, I will keep updating it. I don’t mind if you keep posting in this topic, but I would prefer if we can do further discussions in the new one, so I can quote parts of the OP or add to it as appropriate.
so some people were confused by everyone meaning just logged in users?
who?!
everyone is everyone surely - very clear.
everyone is everyone who is hitting the site, logged in or no, surely that’s simple?
so now a Category that is fully public has to have a minimum of two groups now instead of one - logged in and anon? that’s silly and not an upgrade?
and if not, and you only have to put “anon” because that’s a synonym for “everyone” - that’s no longer correct as logged in users are not anon.
where there was some “learning” around discourse was TL0 in some cases meaning “all those who have an account and logged in” but could also mean “those who have not yet reached TL1 but have an account and are logged in”
the key here is that it didn’t represent a single group of people, it represented a threshold and that was key.
by changing just TL0 to “logged in users” you now break the consistency of each security level being a threshold. TL1 is also a threshold and not really a “single group”. so does that mean we need a group called “logged in users who are at least trust level 1”?!
I’m not at all convinced any of this needed changing - change for change sake?
Very confusing and access control is a very sensitive part of the app!
This is messing with some really long term staple Discourse norms which have stood the test of time because they were correct in the first place.