Added Google as an oauth provider and refactorings by eipplusone · Pull Request #114 · braidchat/braid

eipplusone · 2018-05-29T14:55:49Z

Routes are now less provider dependent.
github.clj deprecated in favor provider-agnostic oauth.clj.
Indentation changes according to https://github.com/bbatsov/clojure-style-guide.

In order to test the changes on would need to register on https://console.developers.google.com/apis/credentials.

Routes are now less provider dependent. github.clj deprecated in favor provider-agnostic oauth.clj Indentation changes according to https://github.com/bbatsov/clojure-style-guide

jamesnvc · 2018-05-29T15:58:41Z

profiles.sample.clj

-    :embedly-key "embedly api key"
+   ;; for avatar and file uploads
+   ;; :aws-domain
+   "braid.mysite.com"


looks like the key here is commented out but not the value

jamesnvc

This looks good! I made some stylistic comments; I'll give it a test run later today.

jamesnvc · 2018-05-29T16:02:06Z

src/braid/core/client/gateway/forms/user_auth/views.cljs

+        :on-click (fn [e]
+                    (.preventDefault e)
+                                        ; must use dispatch-sync
+                                        ; b/c dispatch triggers pop-up blocker


These should probably be ";;" comments so they're aligned with the code (I see that the were single-; before, but if we're fixing the indent, should do this as well)

jamesnvc · 2018-05-29T16:05:15Z

src/braid/core/server/api/github.clj

-    [braid.core.server.util :refer [map->query-str ->transit transit->form]])
-  (:import
-    (org.apache.commons.codec.binary Base64)))
+(ns braid.core.server.api.github)


Presumably this file should just be deleted?

jamesnvc · 2018-05-29T16:06:15Z

src/braid/core/server/api/oauth.clj

+   (org.apache.commons.codec.binary Base64)))
+
+(defn- config-oauth-get [key provider]
+  (let [oauth-paramater-list (-> config :oauth-provider-list edn/read-string)]


should be parameter list, not paramater list?

jamesnvc · 2018-05-29T16:07:56Z

src/braid/core/server/api/oauth.clj

+  [code state provider]
+  (let [info (transit->form (Base64/decodeBase64 state))]
+    (when (and (map? info)
+                                        ; TODO: use spec to validate state when we can use 1.9


change comment to ";;" & fix indent

jamesnvc · 2018-05-29T16:07:56Z

src/braid/core/server/api/oauth.clj

+                                    :data (->> [::nonce ::sent-at ::register? ::group-id]
+                                               (map info)
+                                               string/join)})
+                                        ; Was the request from the last 30 minutes?


change comment to ";;" & fix indent

jamesnvc · 2018-05-29T16:08:04Z

src/braid/core/server/api/oauth.clj

+  [code state provider]
+  (let [info (transit->form (Base64/decodeBase64 state))]
+    (when (and (map? info)
+                                        ; TODO: use spec to validate state when we can use 1.9


change comment to ";;" & fix indent

jamesnvc · 2018-05-29T16:11:00Z

src/braid/core/server/routes/api/public.clj

-    [braid.core.server.routes.helpers :refer [current-user error-response edn-response]]
-    [braid.core.server.s3 :as s3]
-    [braid.core.server.sync :as sync]))
+  (:require [braid.core.common.util :refer [valid-email? valid-nickname?]]


We're trying to incrementally change the ns form to be like this; could you change this to a newline after the :require?

jamesnvc · 2018-05-29T16:12:40Z

src/braid/core/server/routes/client.clj

@@ -1,6 +1,7 @@
 (ns braid.core.server.routes.client
  (:require
   [braid.core.server.api.github :as github]


Can this require be removed now (does the github ns contain anything anymore)?

jamesnvc · 2018-05-30T23:26:16Z

profiles.sample.clj

+   ;; for link info extraction
+   :embedly-key "embedly api key"
+   ;; for oauth
+   {:oauth-provider-list {:github {:auth-uri "https://github.com/login/oauth/authorize?"


This should be unwrapped (:oauth-provider-list ..., not {:oauth-provider-list ...)

jamesnvc · 2018-05-30T23:55:06Z

Something we'll have to figure out for this...this puts a nested map in the env for oauth-provider-list, which works when getting env from the profiles.clj, but for deployment we pass those values in as environment variables, and so they can only be strings.

Simple solution: Something we've done for other things with the same issue is pass the map in as a string and have the place where the value is used call edn/read-string if the value is a string.

More complicated but probably ultimately better solution: Switch to using config instead of environ for config things (we've used it for other projects & I generally like it).

jamesnvc · 2018-05-31T01:45:44Z

src/braid/core/server/routes/api/public.clj

+       (if-let [{:keys [access_token]} (oauth/exchange-token code state provider)]
+         (let [email (oauth/email-address access_token provider)
+               user (user/user-with-email email)]
+           (def access_token access_token)


This line should probably be deleted?

jamesnvc · 2018-05-31T01:50:55Z

This could also use a bit of documentation as to what exactly one has to do in the Google developer console -- I assume one needs the "People" API enabled to get the email address?

jamesnvc · 2018-05-31T01:59:18Z

This isn't working for me; when I try to register with the Google OAuth, things fail in the handler because the email is null -- presumably there's some specific API I need to enable on the Google Dev console to make this work?

eipplusone · 2018-06-01T04:01:54Z

Environ already reads the map from profiles as a string so there's edn/read-string in the parsing logic already. The bigger pain is having to enter that map as a string on the console, so maybe a file name with that map being expected is a better solution. What do you think @jamesnvc?

And yes, People API needs to be enabled for the project.

jamesnvc · 2018-06-02T18:28:40Z

I wonder if it might make more sense to just have env keys for, e.g. :github-client-id, :google-client-secret, etc & put the other keys, with the URIs in code, since the code depends on knowledge of what to do with the returned values anyway.

jamesnvc · 2018-06-02T18:48:24Z

Ah, and trying to get this working, the error message from Google API call seems to indicate that the Google+ API needs to be enabled for the key. This should probably be documented somewhere...maybe in the module docstring?

EDIT: or maybe there's something else I'm missing? I added the Google+ API to a test app, but when I try to use the Google button to register on Braid, it fails & I can see the response from google is:

{:error {:errors [{:domain "global", :reason "notFound", :message "Not Found"}],
            :code 404, :message "Not Found"}}

Not sure why this is happening...@eipplusone what did you have to do to get it working for you?

eipplusone · 2018-06-03T09:25:59Z

Let me write/test it with config changes. I don't think the request makes it to the right api endpoint. It's just a guess though, I haven't seen this error.

Added Google as an oauth provider and refactorings

c17fa83

Routes are now less provider dependent. github.clj deprecated in favor provider-agnostic oauth.clj Indentation changes according to https://github.com/bbatsov/clojure-style-guide

jamesnvc reviewed May 29, 2018

View reviewed changes

PR style changes

ad71082

jamesnvc reviewed May 30, 2018

View reviewed changes

jamesnvc reviewed May 31, 2018

View reviewed changes

rafd force-pushed the master branch from b06af86 to 5a5dd6d Compare January 14, 2021 17:58

Conversation

eipplusone commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesnvc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesnvc commented May 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesnvc commented May 31, 2018

Uh oh!

jamesnvc commented May 31, 2018

Uh oh!

eipplusone commented Jun 1, 2018

Uh oh!

jamesnvc commented Jun 2, 2018

Uh oh!

jamesnvc commented Jun 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eipplusone commented Jun 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

eipplusone commented May 29, 2018 •

edited

Loading

jamesnvc commented Jun 2, 2018 •

edited

Loading