Skip to content

Conversation

@aquast
Copy link
Member

@aquast aquast commented Jul 6, 2018

I've added an white list feature to allow requests only from specific domains. If either an unaccepted domain is choosen or something goes wrong with the image generation an error-image is displayed now.

Amid the works I've been in some trouble to set up tilesCache outside the webapp. As fare as I understand a context.xml is needed to be copied into $CATALINA_BASE/CATALINE/localhost/

@aquast aquast requested a review from jschnasse July 6, 2018 12:50
@aquast
Copy link
Member Author

aquast commented Jul 6, 2018

I've forgotten I made timeouts configurable via deepzoomer.cfg

Copy link
Contributor

@jschnasse jschnasse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not easy to review, because there is no corresponding issue. Just some remarks while walking through. Maybe also some misunderstandings/lack of understanding on my side.

  • Whitelist
    Whitelist implementation is too verbose in my opinion. Just load whitelist to static hashmap and query it before entering critical tasks. I think the right place is at the controller and not within the utility classes. if(!hashmap.containsKey(ip)){ ...return HTTP FORBIDDEN }. (Also, I don't think this should be an exceptional case, just normal data flow. But this is maybe a matter of taste.) BTW, why is class DomainWhiteListCheckerabstract?

For future reference I think you should add at least some hints to provide answers to the following questions.

  • What are the changes in web.xml, context.xml and deepzoomer.cfg for?
  • What are the changes in DeepZoomerUrlService.java for?
  • What are the changes in ZoomifyUrlService.java for?

Maybe the answers are quite obvious to you, but for example, why is an additional entry in context.xml needed? I totally missed that point.

@aquast
Copy link
Member Author

aquast commented Jul 25, 2018

Thank you for review. As discussed yesterday F2F there seems to be a different behavior/access rights implementation for tomcat at Ubuntu and openSuSE: OpenSuSE requires a context path to be set in context.xml in order to allow tomcat webapps access to directories outside the webapps directory. If Ubuntu doesn't, does that mean we have to take extra provisions to prevent tomcat webapps to do this on Ubuntu - and is this something you've already done in your last commit ;-) ? I'm not quite sure about this.

I agree with most of your other remarks. Decision to implement Whitelist within FileUtil was made due to maintenance reasons - and also kind of pressue to find a solution in a hurry. I will review this decision as it has some issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants