Issue/533 admin console features #652

kardan · 2017-03-13T10:38:56Z

…vo-lumen into issue/533-admin-console-features

Pending parent component refresh strategy to reload user list

WIP extending admin scripts add-tenant

…vo-lumen into issue/533-admin-console-features

Disabled database setup while developing the Keycloak part. Don’t handle the case when the user not exists yet.

…vo-lumen into issue/533-admin-console-features

jonase

backend review

jonase · 2017-03-13T11:53:18Z

backend/src/akvo/lumen/admin/add_tenant.clj

@@ -1,25 +1,59 @@
 (ns akvo.lumen.admin.add-tenant
+  "The following env vars are assumed to be present:
+  KEYCLOAK, KEYCLOAK_SECRET These can be found in the Keycloak admin in the
+  akvo-lumen-confidential client section.


Could we enumerate what the options to KEYCLOAK are and what they mean?

jonase · 2017-03-13T11:54:38Z

backend/src/akvo/lumen/admin/add_tenant.clj

+  [label]
+  (cond
+    (< (count label) 3)
+    (throw (Exception. "To short label, should be 3 or more characters."))


We often use ex-info instead of Exception (or some subclass). Perhaps (ex-info "Too short label ......" {:label label})?

jonase · 2017-03-13T11:55:56Z

backend/src/akvo/lumen/admin/add_tenant.clj

+    (< (count label) 3)
+    (throw (Exception. "To short label, should be 3 or more characters."))
+
+    (not (empty? (set/intersection (set blacklist)


(contains? (set blacklist) label) instead here?

jonase · 2017-03-13T11:58:48Z

backend/src/akvo/lumen/admin/add_tenant.clj

+  (try
+    (setup-database (conform-label label) title)
+    (let [user-creds (setup-keycloak label email)]
+      (println "User creds:")


Why do we need to print the admin users credentials?

If we generate a password we want to be able to communicate it.

Thinking about it maybe this should have been automated by sending an email in the script.

👍 I think it's fine for now that we communicate the admin credentials back to the user

jonase · 2017-03-13T11:59:24Z

backend/src/akvo/lumen/admin/add_tenant.clj

+      (println "User creds:")
+      (pprint user-creds))
+    (catch Exception e
+      (println "Got error")


Perhaps print the stacktrace instead?

Removed the top level Exception for ex-info instead.

jonase · 2017-03-13T12:01:27Z

backend/src/akvo/lumen/component/keycloak.clj

@@ -93,6 +110,32 @@
        (response {:status (:status ed)
                   :body (:reasonPhrase ed)})))))

+(defn tenant-admin?
+  [request-draft api-root tenant user-id]


@kardan could you explain what this request-draft object is? I see it's being used a lot but I'm not sure I understand what it is

Request-draft is a request object that is used with clj-http (the second argument to request functions). Thinking about it in hindsight I do think this is a questionable design. I should have named it request-headers and passed that around. Would have been just as simple to build a map with {:headers :body } as what I do now (assoc request-draft :body }.

jonase · 2017-03-13T12:02:30Z

backend/src/akvo/lumen/component/keycloak.clj

+                        (filter #(and (get % "emailVerified")
+                                      (get % "enabled"))
+                                admins))]
+    (not (empty? (set/intersection #{user-id} (set admin-ids))))))


I think you could use contains? here as well instead of (not (empty (set/intersection ...)))

jonase · 2017-03-13T12:05:38Z

backend/src/akvo/lumen/component/keycloak.clj

+        (http/ok (fetch-user-by-id request-draft api-root tenant user-id))
+        (do
+          (println (format "Tried to promote user: %s" user-id))
+          (http/internal-server-error))))))


Will we actually get to this else part if keycloak returns something other than 204? If I remember correctly clj.http client will throw an exception on 4XX/5XX so we would still see http/internal-server-error but not generated from this part of the code?

Same question for the two subsequent places where we have the same branching logic: Is the else part reachable?

jonase · 2017-03-13T12:07:33Z

backend/src/akvo/lumen/component/user_manager.clj

+
+
+
+#_(defn do-promote-user-to-admin


Removed the left overs!

jonase · 2017-03-13T12:08:42Z

backend/src/akvo/lumen/component/user_manager.clj

+    (if (keycloak/tenant-member? keycloak tenant email)
+      (http/bad-request {"reson" "Already tenant member"})
+      (do
+        (future


Do we want/need to do this async and always return 200 OK?

Now describes the different options for KEYCLOAK and where to find the KEYCLOAK_SECRET.

- Removed commented code - Removed the future - yikes!

- Moved for passing around a request-draft object to request-headers - Fixed bug when inviting non existing user and trying to use “nil” username in a keycloak api call.

jonase · 2017-03-14T04:25:16Z

backend/src/akvo/lumen/admin/util.clj

@@ -19,3 +21,22 @@
          password (env :pgpassword)}}]
   (format "jdbc:postgresql://%s/%s?user=%s&password=%s&ssl=true"
           host database user password)))
+
+(defn- keycloak-url
+  "Hardcoded urls, seemed like a fair thing to do."


This comment is a bit weird?

jonase · 2017-03-14T04:27:03Z

backend/src/akvo/lumen/component/keycloak.clj

@@ -37,7 +53,7 @@
  (-> (client/get (format "%s/.well-known/openid-configuration" issuer))
      :body json/decode))

-(defn request-draft
+(defn request-headers
  "Create a partial request with json content type and a valid bearer token.


I think this docstring should be tweaked a bit now when you return headers only?

jonase · 2017-03-14T04:30:31Z

backend/src/akvo/lumen/component/keycloak.clj

+
+(defn tenant-member?
+  "Return true for both members and admins."
+  [{api-root :api-root :as keycloak} tenant email]


Not sure if it's better but usually when I don't want to rename the bindings I use :keys instead:

{:keys [api-root] :as keycloak}

kardan and others added 30 commits February 28, 2017 13:11

[#533] Stubs on new API endpoints

65601d7

[#533] First steps on delete invite

8595bfa

[#533] Delete invite API endpoint

22652b6

[#533] Test for delete invite API endpoint

aea13c0

[#533] Added docs to user-management/delete-invite

faf2afe

[#355] Added email validation to InviteUser modal

77bad8a

Merge branch 'issue/533-admin-console-features' of github.com:akvo/ak…

efb6077

…vo-lumen into issue/533-admin-console-features

[#533] WIP promote user to admin endpoint

ed8606d

[#533] WIP added placeholder component markup for User actions

516a55c

Merge branch 'issue/533-admin-console-features' of github.com:akvo/ak…

d1cee65

…vo-lumen into issue/533-admin-console-features

[#533] First cut on promote user endpoint

5bd9e3a

[#533] First cut on demote user endpoint

b3f24bb

[#533] WIP added placeholder text to fix eslint error

7d4d23d

[#533] Remove user from tenant endpoint

1127718

[#533] WIP hooked up (dummy) user actions

8958041

Merge branch 'issue/533-admin-console-features' of github.com:akvo/ak…

17e9528

…vo-lumen into issue/533-admin-console-features

[#533] WIP user action options now change depending on admin status

48d92cd

[#533] WIP active user cannot delete herself

5fbee9d

[#533] WIP active admin user cannot demote herself

6e6163f

[#533 Guard against re-invitation on invite endpoint

3577849

Merge branch 'develop' into issue/533-admin-console-features

d581a17

#[533] Refactor tests for user admin endpoints

725ae49

[#533] Clean up user manager tests

5136c8f

[#533] WIP first cut of user action API calls

72c731b

Merge branch 'issue/533-admin-console-features' of github.com:akvo/ak…

b9f1f4e

…vo-lumen into issue/533-admin-console-features

[#533] WIP implemented 'promote' and demote' user actions

000f46c

Pending parent component refresh strategy to reload user list

[#533] WIP removed redundant prop type declaration

5cea8f4

[#533] WIP refactored user action selects

e0a5c05

[#533] WIP promote/demote action now only available when appropriate

59ba7cf

[#533] WIP user list now annotates logged-in user with '(me)'

fe16082

kardan and others added 14 commits March 6, 2017 15:13

[#533] Return user rep from user promote / demote endpints

6470ac2

WIP extending admin scripts add-tenant

[#533] WIP reverted to instant onChange instead of form onSubmit

caabb4f

Merge branch 'issue/533-admin-console-features' of github.com:akvo/ak…

731202a

…vo-lumen into issue/533-admin-console-features

[#533] WIP fixed onUserActionChange to correctly chain promises

1f06503

[#533] WIP added styles to user action select

475f2f8

[#533] WIP Extend add-tenant script

8e29d89

Disabled database setup while developing the Keycloak part. Don’t handle the case when the user not exists yet.

[#533] Extend add-tenant script to handle new user

9f8e43c

[#533] Updated docs

07d97e9

Merge branch 'develop' into issue/533-admin-console-features

a27bb2b

[#533] WIP removed redundant references from Users component

5ffc26f

[#533] WIP FIXED api.delete() -> api.del() typo in Users component

487b5d7

[#533] Removed no-content responses since the client wants json

d141355

Merge branch 'issue/533-admin-console-features' of github.com:akvo/ak…

21b9fce

…vo-lumen into issue/533-admin-console-features

[#533] Updated docs on add-tenant script.

c1d4dbf

kardan added the needs review label Mar 13, 2017

kardan assigned kardan and peeb Mar 13, 2017

kardan requested a review from jonase March 13, 2017 10:39

jonase reviewed Mar 13, 2017

View reviewed changes

kardan added 3 commits March 13, 2017 13:56

[#533 Extended doc string on add-tenant namespace"

da958ac

Now describes the different options for KEYCLOAK and where to find the KEYCLOAK_SECRET.

[#533] Changes after feedback

a1a958a

- Removed commented code - Removed the future - yikes!

[#533] Refactored how keycloak request are done & fixed bug

e51a39b

- Moved for passing around a request-draft object to request-headers - Fixed bug when inviting non existing user and trying to use “nil” username in a keycloak api call.

jonase reviewed Mar 14, 2017

View reviewed changes

kardan and others added 4 commits March 14, 2017 09:51

[#533] Updated add-tenant script env vars handling

738a33b

[#533] Changes after feedback

28f135b

Updated release notes for 0.6.2

e313264

Merge branch 'develop' into issue/533-admin-console-features

f9c7669

jonase merged commit bc9af8a into develop Mar 14, 2017

jonase deleted the issue/533-admin-console-features branch March 14, 2017 10:16

jonase removed the needs review label Mar 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/533 admin console features #652

Issue/533 admin console features #652

kardan commented Mar 13, 2017

jonase left a comment

jonase Mar 13, 2017

kardan Mar 13, 2017

jonase Mar 13, 2017

kardan Mar 13, 2017

jonase Mar 13, 2017

kardan Mar 13, 2017

jonase Mar 13, 2017

kardan Mar 13, 2017

kardan Mar 13, 2017

jonase Mar 13, 2017

jonase Mar 13, 2017

kardan Mar 13, 2017

jonase Mar 13, 2017

kardan Mar 13, 2017

jonase Mar 13, 2017

kardan Mar 13, 2017

jonase Mar 13, 2017

jonase Mar 13, 2017

jonase Mar 13, 2017

kardan Mar 13, 2017

jonase Mar 13, 2017

jonase Mar 14, 2017

jonase Mar 14, 2017

jonase Mar 14, 2017




		#_(defn do-promote-user-to-admin

Issue/533 admin console features #652

Issue/533 admin console features #652

Conversation

kardan commented Mar 13, 2017

jonase 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

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