[MGNLETK-85] Site security handling Created: 15/Aug/12  Updated: 08/Oct/12  Resolved: 02/Oct/12

Status: Closed
Project: Extended Templating Kit (closed)
Component/s: None
Affects Version/s: None
Fix Version/s: 2.0.5

Type: Improvement Priority: Major
Reporter: Ondrej Chytil Assignee: Ondrej Chytil
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
causality
is causing MGNLSTK-1010 Change roles bootstrap files to respe... Closed
relation
is related to MAGNOLIA-3914 Site aware ACL - port Closed
Template:
Acceptance criteria:
Empty
Date of First Response:

 Description   

Concept page is suggesting URISecurityFilter should be extended to respect site security and prevent cross-site access. This filter should replace URISecurityFilter when STK is installed and is part of port/improve site-aware ACL - MAGNOLIA-3914.



 Comments   
Comment by Jan Haderka [ 17/Sep/12 ]
CrossSiteSecurityFilter.java:81
 Iterator<Domain> it = site.getDomains().iterator();

What happens when site doesn't have any domains defined is it guaranteed that Site always returns list and not null?

Same class, bit further down:

           while(it.hasNext()){
  84                 authorized = true;

You reset authorized value on every iteration completely discarding result set by previous check. Is that correct? If so then code should be structured differently to show it.

Why is CrossSiteSecurityFilter extending URISecurityFilter in a first place? There should be only on filter that takes care of Site specific URIs and that's SiteSecurityFilter.

ETKModuleVersionHandler - i think that more important than to place crossSite filter before uriSecurity} is to place it after {{channel and multiSite as those two manipulate site that crossSite needs to use.

bypasses - everywhere else bypass for /.magnolia is named dotMagnolia. Can you follow same pattern?

SiteUriSecurityFilterTest - methods testDennyOtherSitePermission and testDennyOtherSitePermission have exactly same content and same check. I think one of them is wrong.

Comment by Jan Haderka [ 26/Sep/12 ]

What kind of problem does the "solver" class solves? IMHO none. What it really is is a CrossSiteAccessDefinition. Another thing - since the patterns are defined once this class is defined, you should compile them only once and keep them inside rather then recompiling them on every execution of the filter ... It might be actually cleaner to move the whole check inside of this class in which case it would be appropriate to call it CrossSiteAccessResolver since then it would indeed resolve whether or not to allow cross site access and also allow others to implement their own arbitrary rules to check.

Comment by Jan Haderka [ 27/Sep/12 ]

Any reason for calling all those pattern props in the Resolver "Patter"?

Comment by Ondrej Chytil [ 27/Sep/12 ]

Ah, I seek a better world without some useless "n".

Comment by Jan Haderka [ 01/Oct/12 ]

Now the logic was moved to separate class here's few more things I'm able to see in the code:

  • private static final variables should be defined on top of the class (if you don't remember why, I'll remind you the interview test once we are back in the office).
  • while solve and resolve translates to same word in your native language, there is actually difference in english, so please take my word on it and call the variable "resolver" (and get/set/add methods that go w/ it)
  • the setResolvers method is missing, which would be a real pain once you improve c2b/n2b
  • there is no need for resolver.getPatternOrNull() method to be public or is there? Protected at best.

Now more on the logic side of the things

  • code checks for domain only if both domainPattern and sitePattern are set. Is that really correct? Should not it be enough if domainPattern is set for this to work?
Generated at Mon Feb 12 01:48:20 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.