Reamer commented on code in PR #4631:
URL: https://github.com/apache/zeppelin/pull/4631#discussion_r1258415612
##########
zeppelin-server/src/main/java/org/apache/zeppelin/service/NotebookService.java:
##########
@@ -871,17 +871,60 @@ public void updateNote(String noteId,
return null;
}
- if (!(Boolean) note.getConfig().get("isZeppelinNotebookCronEnable")) {
+ if (!zConf.isZeppelinNotebookCronEnable()) {
+ boolean hasCronSettings = false;
if (config.get("cron") != null) {
- config.remove("cron");
+ LOGGER.warn("cron should be null when cron is disabled");
+ hasCronSettings = true;
+ }
+ if (config.get("cronExecutingUser") != null) {
+ LOGGER.warn("cronExecutingUser should be null when cron is
disabled");
+ hasCronSettings = true;
+ }
+ if (config.get("cronExecutingRoles") != null) {
+ LOGGER.warn("cronExecutingRoles should be null when cron is
disabled");
+ hasCronSettings = true;
+ }
+ if (hasCronSettings) {
+ callback.onFailure(new IllegalArgumentException("Wrong configs"),
context);
+ return null;
+ }
+ } else {
+ String requestCronUser = (String) config.get("cronExecutingUser");
+ List<String> requestCronRoles = (List<String>)
config.get("cronExecutingRoles");
+
+ if
(!authorizationService.hasRunPermission(Collections.singleton(requestCronUser),
note.getId())) {
+ LOGGER.error("Wrong cronExecutingUser: {}", requestCronUser);
+ callback.onFailure(new IllegalArgumentException(requestCronUser),
context);
+ return null;
+ } else {
+ // This part should be restarted but we need to prepare to notice
who can be a cron user in advance
+ if (!context.getUserAndRoles().contains(requestCronUser)) {
+ LOGGER.error("Wrong cronExecutingUser: {}", requestCronUser);
+ callback.onFailure(new
IllegalArgumentException(requestCronUser), context);
+ return null;
+ }
+
+ if (context.getUserAndRoles().containsAll(requestCronRoles)) {
Review Comment:
I don't quite understand this check. What is wrong with it?
##########
zeppelin-interpreter/src/main/java/org/apache/zeppelin/conf/ZeppelinConfiguration.java:
##########
@@ -596,6 +596,10 @@ public String getShiroPath() {
return new File(shiroPath).exists() ? shiroPath : StringUtils.EMPTY;
}
+ public boolean isAuthenticationEnabled() {
+ return StringUtils.EMPTY != getShiroPath();
Review Comment:
I think
[StringUtils.isblank(...)](https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/StringUtils.html#isBlank-java.lang.CharSequence-)
is better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
