[ZEPPELIN-4356] Zeppelin should stop/die/etc when can't create/access notebook repo

[bhavikpatel9977]

What is this PR for?

Currently, when the Shiro.ini file is not configured properly then Zeppelin stays up but returns MultiException; Instead it would be better to stop/die/etc.

I have made the required code changes. Step further, I order to improvise it I have also raised the question on Stack overflow. Any valuable suggestion from your side will be helpful.

What type of PR is it?

[Bug Fix | Improvement]

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-4356

How should this be tested?

• Zeppelin should work with authentication enabled
• CI pass

Screenshots (if appropriate)

N/A

Questions:

• Does the licenses files need update? no
• Is there breaking changes for older versions? no
• Does this needs documentation? no

[bhavikpatel9977]

@zjffdu / @Leemoonsoo / @liuxunorg Can you please review

[jongyoul]

Basically, I think it would be better to perform validation logic before starting the server. By doing it, you could avoid multithreading issues that you mentioned in my PR. But, what I worried about it, Shiro configuration is also pluggable in current code, thus you'd better consider the case that shiro.ini doesn't exist. WDYT?

[jongyoul]

And thank you for improving my code :-)

[bhavikpatel9977]

Basically, I think it would be better to perform validation logic before starting the server. By doing it, you could avoid multithreading issues that you mentioned in my PR. But, what I worried about it, Shiro configuration is also pluggable in current code, thus you'd better consider the case that shiro.ini doesn't exist. WDYT?

Yes, that is what I am doing validating the configuration before starting the server. As you said Shiro configuration is also pluggable in current code so it should get initialized after Shiro Environment gets initialized? Also is there any specific reason for moving logic (initialization of classes) from the default constructor to the main method?

[jongyoul]

I thought we should consider the case where we don't use Shiro. That's the only reason why I moved that logic into the constructor of SecurityService

[jongyoul]

Thanks. Now, you can validate shiro.ini before injecting it into Jetty. I think we could add your logic inside public static void main directly. But you might consider the case that shiro.ini doesn't exist and isn't needed. And could you please add test cases?

[bhavikpatel9977]

Thanks. Now, you can validate shiro.ini before injecting it into Jetty. I think we could add your logic in side public static void main directly. But you might consider the case that shiro.ini doesn't exist and isn't needed. And could you please add tests cases?

Yes, we can directly add logic in public static void main but I have used injection to avoid updating existing test-cases and I have to write new test class under configuration dir, right as it is configuration related validation.

Based on the shiro.ini file we are checking the triggering the Authentication Service, so if shiro path exists then only we are validating it.

[jongyoul]

Thanks. I missed it. Then we could check if some objects are inherited from AuthorizingRealm or not, correct?

2019년 10월 31일 (목) 12:20, Prabhjyot [email protected]님이 작성:

@prabhjyotsingh commented on this pull request.

In zeppelin-server/src/main/java/org/apache/zeppelin/server/ZeppelinServer.java https://github.com/apache/zeppelin/pull/3488#discussion_r340945687:

@@ -298,6 +301,27 @@ public void contextDestroyed(ServletContextEvent servletContextEvent) {} } }

• private static void validateShiroIni() throws Exception {
• if (conf.getShiroPath().length() > 0) {

•  Ini ini = new Ini();
  

•  ini.loadFromPath(conf.getShiroPath());
  

•  Ini.Section usersSection = ini.get("users");
  

•  String activeDirectoryRealm = ini.getSectionProperty("main", "activeDirectoryRealm");
  

I see two problems in this approach

• This is just a name user can choose it to be anything, for example activeDirectoryRealm user can choose to write this as just adRealm
• What happens if in the future community adds a new realm?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/apache/zeppelin/pull/3488?email_source=notifications&email_token=AA3R7FRVETK73QIOI6TAENDQRJFG5A5CNFSM4JAXZVE2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJ2JLYA#pullrequestreview-309630432, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA3R7FS3ZBWKLVEYI57WDF3QRJFG5ANCNFSM4JAXZVEQ .

[prabhjyotsingh]

Yes, I think that should do it.

[jongyoul]

Got it. It looks more clear way to check it.

2019년 10월 31일 (목) 12:53, Prabhjyot [email protected]님이 작성:

Yes, I think that should do it.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/apache/zeppelin/pull/3488?email_source=notifications&email_token=AA3R7FST523MZPR3ZTOSXCTQRJJFNA5CNFSM4JAXZVE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECWPN7Q#issuecomment-548206334, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA3R7FXS5N7JC2VFFVRBBBTQRJJFNANCNFSM4JAXZVEQ .

[bhavikpatel9977]

Thanks. I missed it. Then we could check if some objects are inherited from AuthorizingRealm or not, correct? 2019년 10월 31일 (목) 12:20, Prabhjyot [email protected]님이 작성: … @.**** commented on this pull request. ------------------------------ In zeppelin-server/src/main/java/org/apache/zeppelin/server/ZeppelinServer.java <#3488 (comment)>: > @@ -298,6 +301,27 @@ public void contextDestroyed(ServletContextEvent servletContextEvent) {} } } + private static void validateShiroIni() throws Exception { + if (conf.getShiroPath().length() > 0) { + Ini ini = new Ini(); + ini.loadFromPath(conf.getShiroPath()); + + Ini.Section usersSection = ini.get("users"); + String activeDirectoryRealm = ini.getSectionProperty("main", "activeDirectoryRealm"); I see two problems in this approach - This is just a name user can choose it to be anything, for example activeDirectoryRealm user can choose to write this as just adRealm - What happens if in the future community adds a new realm? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3488?email_source=notifications&email_token=AA3R7FRVETK73QIOI6TAENDQRJFG5A5CNFSM4JAXZVE2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJ2JLYA#pullrequestreview-309630432>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA3R7FS3ZBWKLVEYI57WDF3QRJFG5ANCNFSM4JAXZVEQ .

We are reading the shiro.ini directly then we will not get an object(realms) that we can check against AuthorizingRealm, right?

[jongyoul]

But you can check if classes are children of AuthorizingRealm or not by many ways including reflection.

2019년 10월 31일 (목) 13:26, bhavikpatel9977 [email protected]님이 작성:

Thanks. I missed it. Then we could check if some objects are inherited from AuthorizingRealm or not, correct? 2019년 10월 31일 (목) 12:20, Prabhjyot [email protected]님이 작성: … <#m_-3078128932017298217_> @.**** commented on this pull request. ------------------------------ In zeppelin-server/src/main/java/org/apache/zeppelin/server/ZeppelinServer.java <#3488 (comment) https://github.com/apache/zeppelin/pull/3488#discussion_r340945687>: > @@ -298,6 +301,27 @@ public void contextDestroyed(ServletContextEvent servletContextEvent) {} } } + private static void validateShiroIni() throws Exception { + if (conf.getShiroPath().length() > 0) { + Ini ini = new Ini(); + ini.loadFromPath(conf.getShiroPath()); + + Ini.Section usersSection = ini.get("users"); + String activeDirectoryRealm = ini.getSectionProperty("main", "activeDirectoryRealm"); I see two problems in this approach - This is just a name user can choose it to be anything, for example activeDirectoryRealm user can choose to write this as just adRealm - What happens if in the future community adds a new realm? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3488 https://github.com/apache/zeppelin/pull/3488?email_source=notifications&email_token=AA3R7FRVETK73QIOI6TAENDQRJFG5A5CNFSM4JAXZVE2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJ2JLYA#pullrequestreview-309630432>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA3R7FS3ZBWKLVEYI57WDF3QRJFG5ANCNFSM4JAXZVEQ .

We are reading the shiro.ini directly then we will not get an object(realms) that we can check against AuthorizingRealm, right?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/apache/zeppelin/pull/3488?email_source=notifications&email_token=AA3R7FQXZBTU274MKBFK2GLQRJM5ZA5CNFSM4JAXZVE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECWQYKA#issuecomment-548211752, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA3R7FWNC6MNOB7BCYRFL23QRJM5ZANCNFSM4JAXZVEQ .

[bhavikpatel9977]

But you can check if classes are children of AuthorizingRealm or not by many ways including reflection. 2019년 10월 31일 (목) 13:26, bhavikpatel9977 [email protected]님이 작성: … Thanks. I missed it. Then we could check if some objects are inherited from AuthorizingRealm or not, correct? 2019년 10월 31일 (목) 12:20, Prabhjyot @.님이 작성: … <#m_-3078128932017298217_> @.* commented on this pull request. ------------------------------ In zeppelin-server/src/main/java/org/apache/zeppelin/server/ZeppelinServer.java <#3488 (comment) <#3488 (comment)>>: > @@ -298,6 +301,27 @@ public void contextDestroyed(ServletContextEvent servletContextEvent) {} } } + private static void validateShiroIni() throws Exception { + if (conf.getShiroPath().length() > 0) { + Ini ini = new Ini(); + ini.loadFromPath(conf.getShiroPath()); + + Ini.Section usersSection = ini.get("users"); + String activeDirectoryRealm = ini.getSectionProperty("main", "activeDirectoryRealm"); I see two problems in this approach - This is just a name user can choose it to be anything, for example activeDirectoryRealm user can choose to write this as just adRealm - What happens if in the future community adds a new realm? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3488 <#3488>?email_source=notifications&email_token=AA3R7FRVETK73QIOI6TAENDQRJFG5A5CNFSM4JAXZVE2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCJ2JLYA#pullrequestreview-309630432>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA3R7FS3ZBWKLVEYI57WDF3QRJFG5ANCNFSM4JAXZVEQ . We are reading the shiro.ini directly then we will not get an object(realms) that we can check against AuthorizingRealm, right? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#3488?email_source=notifications&email_token=AA3R7FQXZBTU274MKBFK2GLQRJM5ZA5CNFSM4JAXZVE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOECWQYKA#issuecomment-548211752>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA3R7FWNC6MNOB7BCYRFL23QRJM5ZANCNFSM4JAXZVEQ .

We will not get a list of realms classes directly(by reading .ini file); also I have checked getRealmsList() here also we are getting loading the SecurityManager class to get realms list.

I think we have to follow the earlier implementation approach.

[jongyoul]

You can use a system classloader to get the list of realms. https://github.com/apache/shiro/blob/master/core/src/main/java/org/apache/shiro/config/IniSecurityManagerFactory.java#L193-L222