Re: Review Request 73599: Conditionally Disable 'Session Inactivity Timeout'

2021-09-23 Thread Nikhil Bonte 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73599/#review223522
---


Ship it!




Ship It!

- Nikhil Bonte


On Sept. 23, 2021, 2:05 a.m., Ashutosh Mestry wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73599/
> ---
> 
> (Updated Sept. 23, 2021, 2:05 a.m.)
> 
> 
> Review request for atlas, Nikhil Bonte, Nixon Rodrigues, Prasad Pawar, and 
> Sarath Subramanian.
> 
> 
> Bugs: ATLAS-4435
> https://issues.apache.org/jira/browse/ATLAS-4435
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> Please see JIRA for details.
> 
> Additional info:
> * UI pop-up dialog looks at the value passed in the response of 
> 'admin/session' REST endpoint. Not passing the configuration value, disables 
> the feature from UI (Smart!).
> 
> 
> Diffs
> -
> 
>   intg/src/main/java/org/apache/atlas/AtlasConfiguration.java fa519ef5a 
>   
> webapp/src/main/java/org/apache/atlas/web/filters/AtlasAuthenticationFilter.java
>  b8d21b9bb 
>   webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java 
> baa040f4c 
>   
> webapp/src/main/java/org/apache/atlas/web/security/AtlasAuthenticationSuccessHandler.java
>  1b1a80826 
> 
> 
> Diff: https://reviews.apache.org/r/73599/diff/3/
> 
> 
> Testing
> ---
> 
> Manual testing:
> * Verified against Knox SSO enabled cluster.
> * User-name password enabled cluster.
> 
> 
> Thanks,
> 
> Ashutosh Mestry
> 
>

Re: Review Request 73599: Conditionally Disable 'Session Inactivity Timeout'

2021-09-22 Thread Ashutosh Mestry via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73599/
---

(Updated Sept. 22, 2021, 8:35 p.m.)


Review request for atlas, Nikhil Bonte, Nixon Rodrigues, Prasad Pawar, and 
Sarath Subramanian.


Changes
---

Updates include: Minor refactoring for clarity.


Bugs: ATLAS-4435
https://issues.apache.org/jira/browse/ATLAS-4435


Repository: atlas


Description
---

Please see JIRA for details.

Additional info:
* UI pop-up dialog looks at the value passed in the response of 'admin/session' 
REST endpoint. Not passing the configuration value, disables the feature from 
UI (Smart!).


Diffs (updated)
-

  intg/src/main/java/org/apache/atlas/AtlasConfiguration.java fa519ef5a 
  
webapp/src/main/java/org/apache/atlas/web/filters/AtlasAuthenticationFilter.java
 b8d21b9bb 
  webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java 
baa040f4c 
  
webapp/src/main/java/org/apache/atlas/web/security/AtlasAuthenticationSuccessHandler.java
 1b1a80826 


Diff: https://reviews.apache.org/r/73599/diff/3/

Changes: https://reviews.apache.org/r/73599/diff/2-3/


Testing
---

Manual testing:
* Verified against Knox SSO enabled cluster.
* User-name password enabled cluster.


Thanks,

Ashutosh Mestry

Re: Review Request 73599: Conditionally Disable 'Session Inactivity Timeout'

2021-09-22 Thread Ashutosh Mestry via Review Board 


> On Sept. 21, 2021, 7:02 p.m., Sailaja Polavarapu wrote:
> > webapp/src/main/java/org/apache/atlas/web/filters/AtlasAuthenticationFilter.java
> > Line 359 (original), 374 (patched)
> > 
> >
> > I think it will be intuitive to move logoutHandler != null check to 
> > inner if condition on line #375.

Please see my comment below.


- Ashutosh


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73599/#review223509
---


On Sept. 22, 2021, 5:42 p.m., Ashutosh Mestry wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73599/
> ---
> 
> (Updated Sept. 22, 2021, 5:42 p.m.)
> 
> 
> Review request for atlas, Nikhil Bonte, Nixon Rodrigues, Prasad Pawar, and 
> Sarath Subramanian.
> 
> 
> Bugs: ATLAS-4435
> https://issues.apache.org/jira/browse/ATLAS-4435
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> Please see JIRA for details.
> 
> Additional info:
> * UI pop-up dialog looks at the value passed in the response of 
> 'admin/session' REST endpoint. Not passing the configuration value, disables 
> the feature from UI (Smart!).
> 
> 
> Diffs
> -
> 
>   intg/src/main/java/org/apache/atlas/AtlasConfiguration.java fa519ef5a 
>   
> webapp/src/main/java/org/apache/atlas/web/filters/AtlasAuthenticationFilter.java
>  b8d21b9bb 
>   webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java 
> baa040f4c 
>   
> webapp/src/main/java/org/apache/atlas/web/security/AtlasAuthenticationSuccessHandler.java
>  1b1a80826 
> 
> 
> Diff: https://reviews.apache.org/r/73599/diff/2/
> 
> 
> Testing
> ---
> 
> Manual testing:
> * Verified against Knox SSO enabled cluster.
> * User-name password enabled cluster.
> 
> 
> Thanks,
> 
> Ashutosh Mestry
> 
>

Re: Review Request 73599: Conditionally Disable 'Session Inactivity Timeout'

2021-09-22 Thread Ashutosh Mestry via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73599/
---

(Updated Sept. 22, 2021, 5:42 p.m.)


Review request for atlas, Nikhil Bonte, Nixon Rodrigues, Prasad Pawar, and 
Sarath Subramanian.


Changes
---

Updates include: 
- Addressed review comments.
- Minor refactoring.


Bugs: ATLAS-4435
https://issues.apache.org/jira/browse/ATLAS-4435


Repository: atlas


Description
---

Please see JIRA for details.

Additional info:
* UI pop-up dialog looks at the value passed in the response of 'admin/session' 
REST endpoint. Not passing the configuration value, disables the feature from 
UI (Smart!).


Diffs (updated)
-

  intg/src/main/java/org/apache/atlas/AtlasConfiguration.java fa519ef5a 
  
webapp/src/main/java/org/apache/atlas/web/filters/AtlasAuthenticationFilter.java
 b8d21b9bb 
  webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java 
baa040f4c 
  
webapp/src/main/java/org/apache/atlas/web/security/AtlasAuthenticationSuccessHandler.java
 1b1a80826 


Diff: https://reviews.apache.org/r/73599/diff/2/

Changes: https://reviews.apache.org/r/73599/diff/1-2/


Testing
---

Manual testing:
* Verified against Knox SSO enabled cluster.
* User-name password enabled cluster.


Thanks,

Ashutosh Mestry

Re: Review Request 73599: Conditionally Disable 'Session Inactivity Timeout'

2021-09-22 Thread Ashutosh Mestry via Review Board 


> On Sept. 22, 2021, 4:58 p.m., Sarath Subramanian wrote:
> > webapp/src/main/java/org/apache/atlas/web/filters/AtlasAuthenticationFilter.java
> > Line 311 (original), 325 (patched)
> > 
> >
> > would 'sessionTimeout' be initialized by then? I see it getting 
> > initialized only at getConfiguration(), so won't it be always take default 
> > ('0'). line 326 will never be reached.

This follows the AuthenticationFilter initialization path. All our secrets 
(which are also part of the config) get initialized here. In short, yes, it 
does get initialized.


> On Sept. 22, 2021, 4:58 p.m., Sarath Subramanian wrote:
> > webapp/src/main/java/org/apache/atlas/web/filters/AtlasAuthenticationFilter.java
> > Line 360 (original), 375 (patched)
> > 
> >
> > add null check ('logoutHandler') to inner 'if' - closer to where it is 
> > used for better readabality.

Doing this will change the flow. I want to be able to resort to the state where 
this code did not exist. Hence I have made it this way.


- Ashutosh


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73599/#review223517
---


On Sept. 21, 2021, 6:38 p.m., Ashutosh Mestry wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73599/
> ---
> 
> (Updated Sept. 21, 2021, 6:38 p.m.)
> 
> 
> Review request for atlas, Nikhil Bonte, Nixon Rodrigues, Prasad Pawar, and 
> Sarath Subramanian.
> 
> 
> Bugs: ATLAS-4435
> https://issues.apache.org/jira/browse/ATLAS-4435
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> Please see JIRA for details.
> 
> Additional info:
> * UI pop-up dialog looks at the value passed in the response of 
> 'admin/session' REST endpoint. Not passing the configuration value, disables 
> the feature from UI (Smart!).
> 
> 
> Diffs
> -
> 
>   intg/src/main/java/org/apache/atlas/AtlasConfiguration.java fa519ef5a 
>   
> webapp/src/main/java/org/apache/atlas/web/filters/AtlasAuthenticationFilter.java
>  b8d21b9bb 
>   webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java 
> baa040f4c 
>   
> webapp/src/main/java/org/apache/atlas/web/security/AtlasAuthenticationSuccessHandler.java
>  1b1a80826 
> 
> 
> Diff: https://reviews.apache.org/r/73599/diff/1/
> 
> 
> Testing
> ---
> 
> Manual testing:
> * Verified against Knox SSO enabled cluster.
> * User-name password enabled cluster.
> 
> 
> Thanks,
> 
> Ashutosh Mestry
> 
>

Re: Review Request 73599: Conditionally Disable 'Session Inactivity Timeout'

2021-09-22 Thread Sarath Subramanian 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73599/#review223517
---


Fix it, then Ship it!





webapp/src/main/java/org/apache/atlas/web/filters/AtlasAuthenticationFilter.java
Line 311 (original), 325 (patched)


would 'sessionTimeout' be initialized by then? I see it getting initialized 
only at getConfiguration(), so won't it be always take default ('0'). line 326 
will never be reached.



webapp/src/main/java/org/apache/atlas/web/filters/AtlasAuthenticationFilter.java
Line 360 (original), 375 (patched)


add null check ('logoutHandler') to inner 'if' - closer to where it is used 
for better readabality.


- Sarath Subramanian


On Sept. 21, 2021, 11:38 a.m., Ashutosh Mestry wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73599/
> ---
> 
> (Updated Sept. 21, 2021, 11:38 a.m.)
> 
> 
> Review request for atlas, Nikhil Bonte, Nixon Rodrigues, Prasad Pawar, and 
> Sarath Subramanian.
> 
> 
> Bugs: ATLAS-4435
> https://issues.apache.org/jira/browse/ATLAS-4435
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> Please see JIRA for details.
> 
> Additional info:
> * UI pop-up dialog looks at the value passed in the response of 
> 'admin/session' REST endpoint. Not passing the configuration value, disables 
> the feature from UI (Smart!).
> 
> 
> Diffs
> -
> 
>   intg/src/main/java/org/apache/atlas/AtlasConfiguration.java fa519ef5a 
>   
> webapp/src/main/java/org/apache/atlas/web/filters/AtlasAuthenticationFilter.java
>  b8d21b9bb 
>   webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java 
> baa040f4c 
>   
> webapp/src/main/java/org/apache/atlas/web/security/AtlasAuthenticationSuccessHandler.java
>  1b1a80826 
> 
> 
> Diff: https://reviews.apache.org/r/73599/diff/1/
> 
> 
> Testing
> ---
> 
> Manual testing:
> * Verified against Knox SSO enabled cluster.
> * User-name password enabled cluster.
> 
> 
> Thanks,
> 
> Ashutosh Mestry
> 
>

Re: Review Request 73599: Conditionally Disable 'Session Inactivity Timeout'

2021-09-21 Thread Nixon Rodrigues 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73599/#review223512
---


Ship it!




Ship It!

- Nixon Rodrigues


On Sept. 21, 2021, 6:38 p.m., Ashutosh Mestry wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73599/
> ---
> 
> (Updated Sept. 21, 2021, 6:38 p.m.)
> 
> 
> Review request for atlas, Nikhil Bonte, Nixon Rodrigues, Prasad Pawar, and 
> Sarath Subramanian.
> 
> 
> Bugs: ATLAS-4435
> https://issues.apache.org/jira/browse/ATLAS-4435
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> Please see JIRA for details.
> 
> Additional info:
> * UI pop-up dialog looks at the value passed in the response of 
> 'admin/session' REST endpoint. Not passing the configuration value, disables 
> the feature from UI (Smart!).
> 
> 
> Diffs
> -
> 
>   intg/src/main/java/org/apache/atlas/AtlasConfiguration.java fa519ef5a 
>   
> webapp/src/main/java/org/apache/atlas/web/filters/AtlasAuthenticationFilter.java
>  b8d21b9bb 
>   webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java 
> baa040f4c 
>   
> webapp/src/main/java/org/apache/atlas/web/security/AtlasAuthenticationSuccessHandler.java
>  1b1a80826 
> 
> 
> Diff: https://reviews.apache.org/r/73599/diff/1/
> 
> 
> Testing
> ---
> 
> Manual testing:
> * Verified against Knox SSO enabled cluster.
> * User-name password enabled cluster.
> 
> 
> Thanks,
> 
> Ashutosh Mestry
> 
>

Re: Review Request 73599: Conditionally Disable 'Session Inactivity Timeout'

2021-09-21 Thread Sailaja Polavarapu 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73599/#review223509
---




webapp/src/main/java/org/apache/atlas/web/filters/AtlasAuthenticationFilter.java
Line 359 (original), 374 (patched)


I think it will be intuitive to move logoutHandler != null check to inner 
if condition on line #375.


- Sailaja Polavarapu


On Sept. 21, 2021, 6:38 p.m., Ashutosh Mestry wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/73599/
> ---
> 
> (Updated Sept. 21, 2021, 6:38 p.m.)
> 
> 
> Review request for atlas, Nikhil Bonte, Nixon Rodrigues, Prasad Pawar, and 
> Sarath Subramanian.
> 
> 
> Bugs: ATLAS-4435
> https://issues.apache.org/jira/browse/ATLAS-4435
> 
> 
> Repository: atlas
> 
> 
> Description
> ---
> 
> Please see JIRA for details.
> 
> Additional info:
> * UI pop-up dialog looks at the value passed in the response of 
> 'admin/session' REST endpoint. Not passing the configuration value, disables 
> the feature from UI (Smart!).
> 
> 
> Diffs
> -
> 
>   intg/src/main/java/org/apache/atlas/AtlasConfiguration.java fa519ef5a 
>   
> webapp/src/main/java/org/apache/atlas/web/filters/AtlasAuthenticationFilter.java
>  b8d21b9bb 
>   webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java 
> baa040f4c 
>   
> webapp/src/main/java/org/apache/atlas/web/security/AtlasAuthenticationSuccessHandler.java
>  1b1a80826 
> 
> 
> Diff: https://reviews.apache.org/r/73599/diff/1/
> 
> 
> Testing
> ---
> 
> Manual testing:
> * Verified against Knox SSO enabled cluster.
> * User-name password enabled cluster.
> 
> 
> Thanks,
> 
> Ashutosh Mestry
> 
>

Review Request 73599: Conditionally Disable 'Session Inactivity Timeout'

2021-09-21 Thread Ashutosh Mestry via Review Board 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/73599/
---

Review request for atlas, Nikhil Bonte, Nixon Rodrigues, and Sarath Subramanian.


Bugs: ATLAS-4435
https://issues.apache.org/jira/browse/ATLAS-4435


Repository: atlas


Description
---

Please see JIRA for details.


Diffs
-

  intg/src/main/java/org/apache/atlas/AtlasConfiguration.java fa519ef5a 
  
webapp/src/main/java/org/apache/atlas/web/filters/AtlasAuthenticationFilter.java
 b8d21b9bb 
  webapp/src/main/java/org/apache/atlas/web/resources/AdminResource.java 
baa040f4c 
  
webapp/src/main/java/org/apache/atlas/web/security/AtlasAuthenticationSuccessHandler.java
 1b1a80826 


Diff: https://reviews.apache.org/r/73599/diff/1/


Testing
---

Manual testing:
* Verified against Knox SSO enabled cluster.
* User-name password enabled cluster.


Thanks,

Ashutosh Mestry