Fix #5675: Feature Request for Private Community Boards - #6478

force_authenticated duplicates the logic for policies?

You can probably just modify app/policies/application_policy.rb index method. Policies for sessions, 2fa, password resets, etc already have explicit sets to anonymous users.

Thanks for the suggestion: indeed using policies is much cleaner (no need for "skip" override).

I updated PR with the following:

• Switched to use application policy
• Reverted almost every template change: since all the pages show 403 anyway, why hide the links. The only template change proposed is to show "Sign Up" link under /users URL for the admin users when signups_restricted_to_admin is set.
• Tests are written.

I ran the entire test suite, got 89 failed tests, all are related to external sites (extractors tests). I guess this is expected.

Update:

During my tests I've found some pages that were not affected by the ApplicationPolicy index method change.
All of them had ApplicationPolicy as a parent class but had their index method redefined with unconditional return true.
I removed the index method from them, so now they use the parent method (which by default returns true).

The list of policies I updated:

• ai_tag_policy.rb
• background_job_policy.rb
• forum_post_policy.rb
• forum_topic_policy.rb
• media_asset_policy.rb
• media_metadata_policy.rb
• rate_limit_policy.rb
• user_event_policy.rb

Maybe I don't understand something there, and those methods were redefined for a reason? Any advice is appreciated.

Look around the policies for them. There's redundant index methods in their policies, which can probably be stripped since they also duplicate application policy's index method.

Yes, that's what I did after noticing the first such URL - I grepped for the index? method in app/policies?. Some files did have logic in the index method, and I did not modify them. The others, that I listed above, only had an empty method, those files are now modified.

I updated the pull request with the newest changes.

Before going through the pull: the main problem with merging something like this is that we basically have to support it in perpetuity.

This is the kind of feature that is very easily breakable by new endpoint addition, it's not a matter of when but if, because we are not going to be focused on mantaining features that the main site will never use.

Also, you're trying to introduce two pretty big features at once in this pull, and neither will ever be used by the main site. These two features work in tandem, so why are they separate? And the workflow for new user creation does not make much sense to me. Are you expecting admins to manually create users and then give account passwords away? I don't think that's acceptable.

If you want a completely locked system, it would probably make more sense to restrict vision to verified users (members) rather than registered users. Basically, all new accounts start as restricted, and the only way to view stuff is be promoted to member by an admin. That way you avoid direct password handling. But again, that's something that you'd have in your own fork, we're not really interested in having that kind of logic in the main repo because it would just be a feature we'd have to mantain forever despite never using it.