improve sudo, not fixing existing bugs yet ref T68510 #242

satang500 · 2017-05-16T21:42:32Z

only show sudo button to users who have access
sudo should not require logging in again, but should still require confirmation

tedpearson

I am extremely concerned about the security of this approach. I would recommend completely rethinking the design of this feature before proceeding.

tedpearson · 2017-05-16T21:51:54Z

hydra-main/src/main/java/com/addthis/hydra/job/auth/AuthenticationManagerNested.java

+        }
+        if( inner != null && inner.isAdmin(user)) {
+            return true;
+        } else if (outer != null && outer.isAdmin(user)) {


There's no need for an "else if" since you are returning from the "if" above.

tedpearson · 2017-05-16T21:52:19Z

hydra-main/src/main/java/com/addthis/hydra/job/auth/PermissionsManager.java

@@ -111,6 +111,10 @@ public User authenticate(String username, String secret) {
        return authentication.authenticate(username, secret);
    }

+    public boolean isamdin(User user) {


Incorrect spelling

What @tedpearson meant is camel case isAdmin is the convention

What i meant is that "amdin" is not a word.

tedpearson · 2017-05-16T22:02:36Z

hydra-web/src/js/app.js

            } else {
                alertify.error("Unknown token name " + tokenName);
                return;
            }
            var username = $.trim(usernameInput.value);
            var password = passwordInput.value;
+            passwordCache = passwordInput.value;


I would HIGHLY DISCOURAGE any attempt to save ldap passwords on the browser side. I recommend that we completely change our approach to something that does not involve the caching of passwords on the browser.

You should be using a secure https only cookie that cannot be accessed by js and that gets refreshed on every request so that it doesn't expire unless there is a period of inactivity greater than the ttl.

Ah, i see what this does. Yeah, caching in memory isn't great. It's visible in the call stack of anything in this require block.

yuesong

Thanks. This is a good PR. Most comments are related to coding styles and possible simplification. End user functionality appears to be working properly.

The only thing that I didn't see is that sudo should timeout automatically after a short period of time, e.g. 15 minutes.

Merging master into this PR would make review easier.

yuesong · 2017-05-24T01:52:42Z

hydra-web/src/js/app.js

+            var token = Cookies.get("token");
+            if(app.issudo(username, token)) {
+                app.sudoCb(false, true);
+            }


Why not move this block of code to the success handler of /update/settings a few lines below? They both use username and token cookies and handle sudo checkbox value and visibility.

yuesong · 2017-05-24T01:53:25Z

hydra-web/src/js/app.js

@@ -111,6 +119,27 @@ function(
                }
            });
        },
+        issudo: function(username, token) {


I'd rename this function to isadmin to match endpoint name

will change

yuesong · 2017-05-24T01:57:34Z

hydra-web/src/js/app.js

+                        app.sudoCb(false, true);
+                    } else {
+                        app.sudoCb(false, false);
+                    }


Instead of the explicit if/else, you can use response directly:

app.sudoCb(false, response == 'true')

or assign it to a variable called visible to make the code more self-documenting

yuesong · 2017-05-24T01:58:45Z

hydra-web/src/js/app.js

+                data: {
+                    username: username,
+                    token: token,
+                    sudo: Cookies.get("sudo"),


Remove the extra trailing ,

nice catch. will remove.

yuesong · 2017-05-24T02:01:50Z

hydra-web/src/js/app.js

        sudo:function() {
+            var sudoUrl = app.authprefix() + "/authentication/sudo";


Consider refactor this function - I feel the way it mixes username/token validation and unsudo makes it less readable than it can be.

yuesong · 2017-05-24T02:13:03Z

hydra-main/src/main/java/com/addthis/hydra/job/auth/PermissionsManager.java

@@ -34,6 +35,7 @@

    private static final Logger log = LoggerFactory.getLogger(PermissionsManager.class);

+    @VisibleForTesting


Unnecessary?

yuesong · 2017-05-24T02:13:31Z

hydra-main/src/main/java/com/addthis/hydra/job/auth/PermissionsManager.java

@@ -111,6 +111,10 @@ public User authenticate(String username, String secret) {
        return authentication.authenticate(username, secret);
    }

+    public boolean isamdin(User user) {


What @tedpearson meant is camel case isAdmin is the convention

yuesong · 2017-05-24T02:14:14Z

hydra-main/src/main/java/com/addthis/hydra/job/auth/PermissionsManager.java

+    public User getUser(String username) {
+        User user = authentication.getUser(username);
+        return user;
+    }


return authentication.getUser(username);

yuesong · 2017-05-24T02:17:31Z

hydra-main/src/main/java/com/addthis/hydra/job/web/resources/AuthenticationResource.java

+            builder = Response.ok(Boolean.toString(false));
+        } else {
+            builder = Response.ok(Boolean.toString(true));
+        }


Just do:

builder = Response.ok(Boolean.toString(isadmin))

yuesong · 2017-05-24T02:17:47Z

hydra-main/src/main/java/com/addthis/hydra/job/web/resources/AuthenticationResource.java

            builder.header("Access-Control-Allow-Origin",
                           "http://" + uriInfo.getAbsolutePath().getHost() +
                           ":" + configuration.webPort);
            builder.header("Access-Control-Allow-Methods", "POST");
            return builder.build();
        } catch (Exception ex)  {
-            log.warn("Internal error in sudo attempt for user {} with ssl {}", username, usingSSL, ex);
-            return Response.serverError().entity(ex.toString()).build();
+            // TODO: don't show "Accept out https certificate" and hydra.png


Why this TODO here?

mistakenly added will remove

yuesong · 2017-05-24T02:34:42Z

Also please make sure any script that requires sudo privilege, such as fail-minion, still works.

satang500 · 2017-05-24T20:30:50Z

will keep the bug described in app.js as it is for now.

yuesong · 2017-05-24T22:49:34Z

New changes look good to me. You should merge master to this PR (and resolve the web build files conflict and regenerate them with webpack -p as part of the merge).

improve sudo, not fixing existing bugs yet ref T68510

270e0b5

tedpearson suggested changes May 16, 2017

View reviewed changes

Irene Cho added 2 commits May 22, 2017 10:38

improve sudo ref T68510

44808c6

add feature-checking sudo popup a confirmation

8b11daa

yuesong suggested changes May 24, 2017

View reviewed changes

modified code based on comment

8261295

yuesong approved these changes May 24, 2017

View reviewed changes

Irene Cho added 2 commits May 24, 2017 23:35

modified timeout for sudo

a9797b3

Merge branch 'master' into sudoimp

12cfc46

yuesong merged commit bba99d0 into master May 25, 2017

yuesong deleted the sudoimp branch May 25, 2017 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve sudo, not fixing existing bugs yet ref T68510 #242

improve sudo, not fixing existing bugs yet ref T68510 #242

satang500 commented May 16, 2017

tedpearson left a comment

tedpearson May 16, 2017

tedpearson May 16, 2017

yuesong May 24, 2017

tedpearson May 24, 2017

tedpearson May 16, 2017

jhorwit2 May 18, 2017 •

edited

Loading

jhorwit2 May 18, 2017

yuesong left a comment

yuesong May 24, 2017

yuesong May 24, 2017

satang500 May 24, 2017

yuesong May 24, 2017

yuesong May 24, 2017

satang500 May 24, 2017

yuesong May 24, 2017

yuesong May 24, 2017

yuesong May 24, 2017

yuesong May 24, 2017

yuesong May 24, 2017

yuesong May 24, 2017

satang500 May 24, 2017

yuesong commented May 24, 2017

satang500 commented May 24, 2017

yuesong commented May 24, 2017

		sudo:function() {
		var sudoUrl = app.authprefix() + "/authentication/sudo";

		@@ -34,6 +35,7 @@

		private static final Logger log = LoggerFactory.getLogger(PermissionsManager.class);

		@VisibleForTesting

improve sudo, not fixing existing bugs yet ref T68510 #242

improve sudo, not fixing existing bugs yet ref T68510 #242

Conversation

satang500 commented May 16, 2017

tedpearson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhorwit2 May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuesong left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuesong commented May 24, 2017

satang500 commented May 24, 2017

yuesong commented May 24, 2017

jhorwit2 May 18, 2017 •

edited

Loading