SAK-49716 Scheduled (un)publishing not pinning/removing sites correctly #12350
Conversation
portal/portal-service-impl/impl/src/java/org/sakaiproject/portal/service/PortalServiceImpl.java
Outdated
Show resolved
Hide resolved
| @@ -779,14 +780,29 @@ else if (iconType.equalsIgnoreCase("cl")){ | |||
| } | |||
|
|
|||
| private boolean canUserUpdateSite(String userId, String siteId) { | |||
| return securityService.unlock(userId, SiteService.SECURE_UPDATE_SITE, siteService.siteReference(siteId)); | |||
| try { | |||
There was a problem hiding this comment.
this was the performance killer which is why I changed this to a security service check, it would be preferred to not switch back to the expensive calls?
There was a problem hiding this comment.
right, you're now loading the entire site, pages, tools, properties, members
There was a problem hiding this comment.
Oddly, securityService.unlock will return 'true' for student members of a site during scheduled publish events or scheduled unpublish events. This is counter-intuitive especially because securityService.unlock returns false (as expected) during the events of publishing and unpublishing a site manually.
Fortunately, for scheduled publish events, canUserUpdateSite is not as significant a determinant like it is for scheduled unpublish events.
I haven't begun adding debugging code to SakaiSecurity.unlock to assess what's causing that function to return true for student members. If that method is indeed a bug, that would obviously be something to investigate and fix.
However, if that function should be left as is, then one approach (a kind of hack) would be to distinguish scheduled unpublish events (site.unpublish.scheduled) from their manual analog (site.unpublish) in order execute the more performance costly logic only when responding to scheduled unpublish.
Thoughts?
There was a problem hiding this comment.
the first thing I think of is the use of a SecurityAdvisor that is set to allow everything .... so during the publishing process it doesn't filter and just says TRUE for everything .... where is the code? my guess is you will find a SecurityAdvisor
There was a problem hiding this comment.
In the kernel SakaiSecurity.java (approx. line 728) does indeed consult SecurityAdvisor for a few conditions. Empirically it seems to be a factor in returning 'true' for the unlock method during scheduled unpublishing/publishing.
How should I proceed without risking introducing regressions elsewhere?
There was a problem hiding this comment.
remove all of the SecurityAdvisor use
then you need to run the job as a user. take a look at a job in the jobscheduler directory. just grep for session.setUserEid
then the job will run as a real user (admin user) instead of running with the advisors
There was a problem hiding this comment.
Got it. Thanks for the clarification.
There was a problem hiding this comment.
I've now force pushed suggested revisions.
There was a problem hiding this comment.
nice job. i assume all working well in your testing?
There was a problem hiding this comment.
Thanks, yes. For me this passed the test plan in SAK-49716.
jira: https://sakaiproject.atlassian.net/browse/SAK-49716
I revised PortalServiceImpl.canUserUpdateSite because securityService.unlock-- the basis for that method-- failed to deliver the desired value for students for the scheduled unpublishing test case. Whereas use of site.isAllowed(userId, SiteService.SECURE_UPDATE_SITE) instead provided consistent results for all test cases.