My initial question was if it would be possible to allow crawlers access to locked down categories when the category-lockdown plugin is being used. This turned out to be pretty hard since there are a number of architectural cough “issues” in Discourse.
The crawl branch does not return any content. It successfully circumvents the 402 from happening but somehow the content is not returned to the client. I will play around with this a bit.
Thanks for making the branch. I did try something similar but the
use_crawler_layoutwas not accessible from the Guardian and I didn’t get the smart idea of trapping this in the ApplicationController. Although I am afraid that although
rescue_discourse_actionsis not called, the code after the exception is not executed.
Yeah, its tricky for sure. You need to somehow pass a boolean value to the
TopicViewobject(likely in opts param) while creating it so you can decide from
check_and_raise_exceptionswhether to raise an exception or not.
So the issue is that TopicView creates its own Guardian (line 57) instead of reusing the guardian that was created by ApplicationController. The Guardian created in TopicView does not have access to the request object.
Passing stuff in opts was what I thought about as well but it would require completely overriding the show method of the TopicViewController, and that would be a hell to maintain.
The only thing I can think of is moving the logic out of TopicView and passing it into TopicController. Overriding the show method is a bad option but at the end of the show method, the
perform_show_responseis called, and it is only called from that single place (I guess they have been splitting up a large method to keep their code quality indicators green).
Overriding perform_show_response would be a good place I think, because it would be
def perform_show_response # logic here, with access to use_crawler_layout super end
However, it would only cover the show topic method and all other places where content could be seen is not covered by this. So maybe we would be throwing the baby out with the bath water…
When I started thinking about this 2 days ago, I thought this would be easy
Yeah, I reached exactly the same conclusions you reached.
TopicViewhas been used in many places which aren’t covered if we use this solution.
I was thinking about whether we can set some global state before the
TopicViewobject created and access that global state. Its still tricky because we need to make sure what we’re reading is correct and we need to destroy it correctly.
Ok, you can do this. Its possible to
renderin the rescue block.
Maybe I am misunderstanding, but check_and_raise_exceptions is called at the (almost) very top of the TopicView constructor. So when the rescue code runs, a large part of the code has not been executed?
I like your idea of setting some kind of “global” flag. This way we could skip raising the exception in the TopicController.show flow and in all other cases the code would behave like it does now.
I think it’s really dirty but
it looks likethis works:
- override TopicController.show to have
def show $crawler = use_crawler_layout? super end
add one simple line to the
module ::CategoryLockdown def self.is_locked(guardian, topic) return false if $crawler ... (the rest of the code) ... end class NoAccessLocked < StandardError; end end