weavejester/ring-oauth2

Implicit dependencies on other middelware

ingesolvoll opened this issue · 5 comments

ingesolvoll commented

This middleware assumes that the developer already applied wrap-session, wrap-cookie, wrap-params and possibly more. I struggled a bit today before remembering that. I guess this would be a blocker for less experienced ring users.

Would it be a good idea to insert a paragraph about those implicit dependencies in README.md? There already exists a reference to wrap-defaults, but that one is rather specific about a single problem.

weavejester commented

It requires wrap-session and wrap-params. A note in the README would be very useful, as I seem to have forgotten to do that!

ingesolvoll commented

I can make a pull request if you want help

Kah0ona commented

This has been added recenlty to the docs. Is this clear enough now? It mentions wrap-params, but not yet wrap-session.

jcpsantiago commented

I don't see any changes in the README, it only mentions wrap-defaults but not explicit ordering. I may be having this issue, or maybe not.

          (http/run-server
            (-> (routes lagosta-routes)
                (wrap-oauth2 okta-attrs)
                (wrap-defaults (-> site-defaults (assoc-in [:session :cookie-attrs :same-site] :lax)))
                wrap-params)
            {:port 3434})))

does this look correct? I'm trying to use Okta and going to my :launch-uri does nothing i.e. I'm not redirected to Okta's authorize-uri

SOLVED: it wasn't related, but maybe a bug? my :launch-uri was defined as an absolute url http://localhost:3434/login which didn't work, but /login does. README says

It can be any relative URI as long as it is unique. It can also be an absolute URI like

weavejester commented 
          (http/run-server
            (-> (routes lagosta-routes)
                (wrap-oauth2 okta-attrs)
                (wrap-defaults (-> site-defaults (assoc-in [:session :cookie-attrs :same-site] :lax)))
                wrap-params)
            {:port 3434})))

does this look correct?

Yes, except that wrap-params is also added by wrap-defaults, so you can omit it.

README says

It can be any relative URI as long as it is unique. It can also be an absolute URI like

That's the documentation for the redirect URI:

The redirect URI provides the internal callback. It can be any relative URI as long as it is unique. It can also be an absolute URI like

It's probably a good idea to mention that the launch-uri must be a relative URI, since it adds a route associated with that URI.