[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: |
|
||||||||||||||||
| 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 - |
| 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:
Now more on the logic side of the things
|