WIP: Make it general purpose with easy integration by eskimor · Pull Request #2 · obsidiansystems/obelisk-oauth

eskimor · 2019-02-26T19:14:23Z

No description provided.

and made it suitable for both backend routes and frontend routes.

introduced Common module and did some cleanup.

for consistency.

ryantrinkle

I think this package can be considerably simplified. In particular, I think the logic regarding local storage can (and should) be eliminated, with the user providing the means for persistence and/or communication.

ryantrinkle · 2019-02-27T20:52:36Z

backend/Obelisk/OAuth/Backend.hs

+module Obelisk.OAuth.Backend
+  (getOAuthToken) where
+
+import           Data.Functor.Identity


Please don't add spaces here.

ryantrinkle · 2019-02-27T20:52:50Z

backend/Obelisk/OAuth/Backend.hs

+import           Data.Functor.Sum
+import qualified Data.Text                             as T
+import qualified Data.Text.Encoding                    as T
+import           Network.HTTP.Client                   (Request (..),


Please don't do 2D formatting in source code

ryantrinkle · 2019-02-27T20:56:00Z

common/Obelisk/OAuth/Config.hs

+  -> Maybe (r (R OAuthRoute))
+  -> m (OAuthConfigPublic r)
+getOAuthConfigPublic p@(OAuthProvider provider) mr = liftIO $ do
+  providerUri <- getTextConfigRequired $ "config/common/oauth/" <> provider <> "/uri"


You're using the scheme "config/common/oauth/" <> provider <> "/blah" in a lot of places; if any of those places fail to agree, you will have a bug. You should try to centralize this decision so that it is made in exactly one place.

ryantrinkle · 2019-02-27T20:56:45Z

common/Obelisk/OAuth/Config.hs

+  providerUri <- getTextConfigRequired $ "config/common/oauth/" <> provider <> "/uri"
+  mRedirectBase <- getTextConfig $ "config/common/route"
+  mScope <- getTextConfig $ "config/common/oauth" <> provider <> "/scope"
+  clientId <- getTextConfigRequired $ "config/common/oauth/" <> provider <> "/client-id"


I'm skeptical of the approach of using many separate files; wouldn't it be easier to use a single JSON file? This is really a stylistic decision, but i'm curious if there's a specific reason you prefer this way.

Non really. I just got the impression this was the current Obelisk way of doing configs. A single JSON file is probably better.

Makes sense. Let's go with the JSON file approach.

ryantrinkle · 2019-02-27T20:57:32Z

common/Obelisk/OAuth/Config.hs

+    { _oAuthConfig_responseType = AuthorizationResponseType_Code
+    , _oAuthConfig_provider = p
+    , _oAuthConfig_providerUri = providerUri
+    , _oAuthConfig_redirectUri = (,) <$> mRedirectBase <*> mr


Shouldn't these always agree on whether they're Just or Nothing? If one's missing and the other isn't, shouldn't that be a "parse" failure?

Note: It is (much) better to be strict with parsing this kind of stuff. We want to blow up as soon as possible if the configs are invalid.

That's a good point. Although there is only one case that should be a parse error: When mr is Just, but mRedirectBase is Nothing (which is unlikely). The other three cases are actually valid. Will fix it nonetheless.

ryantrinkle · 2019-02-27T21:09:02Z

common/Obelisk/OAuth/State/Internal.hs

+--   on some value derived from the current time.
+genOAuthState :: MonadIO m => m OAuthState
+genOAuthState = liftIO $ do
+  let fastRandom nr = maybe (getEntropy nr) pure =<< getHardwareEntropy nr


Why are we getting entropy this way? Why not just getEntropy?

I just wanted it as random as possible (that way was suggested in the entropy docs) - it should not really matter though.

That way was suggested in the entropy docs for a specific use case, not as the default. Not everyone trusts the hardware RNG - this was the topic of extensive debate in the Linux kernel developer community. getEntropy will get cryptographic-quality entropy in the standard way for the given system (usually /dev/urandom on linux), so that's what we should generally do. Crypto needs to be kept simple.

Yeah I know that debate. Sure I will go with just getEntropy!

ryantrinkle · 2019-02-27T21:11:28Z

frontend/ChangeLog.md

+
+## 0.1.0.0 -- YYYY-mm-dd
+
+* First version. Released on an unsuspecting world.


I think this is generated by cabal; let's just delete the whole file.

ryantrinkle · 2019-02-27T21:13:42Z

frontend/Obelisk/OAuth/Frontend.hs

+makeReflexLenses ''OAuthFrontend
+
+
+type OAuthConstraints t m =


Let's not have "Constraints" in the name of a constraint synonym. (See Joel Spolsky's article on why "Hungarian" notation is bad)

I don't really see the problem in this case, also what should I name it then?

HasOAuth or SupportsOAuth or something like that would be good. Just, generally, the name of a thing shouldn't include the type of the thing (or, in this case, the kind of the thing).

Thanks - I like SupportsOAuth :-)

ryantrinkle · 2019-02-27T21:16:00Z

frontend/Obelisk/OAuth/Frontend.hs

+    mOldState <- getItemStorage sessionStorage StoreOAuth_State
+    let
+      onParamsErr = fmapMaybe (fmap (checkState mOldState =<<) . getParams) onRoute
+    performEvent_ $ removeItemStorage sessionStorage StoreOAuth_State <$ onParamsErr


I don't think it makes sense for this package to worry about how the oauth-related data is persisted. Let's let the caller specify whatever's needed to store this, e.g. in local storage or somewhere else.

Also, can you explain what's going on here with removing the storage item and such?

Well, as we redirect the user to github - we need to store the generated state somewhere, so we have it when the user comes back. I picked session storage as it is ideal for this purpose.

I remove it from storage mostly for good measure to ensure any kind of replay attacks are not possible. It should not be necessary as session storage is transient anyway.

Letting the user specify what's needed to store this is most likely overkill, also customization could be added later on when the need arises in a backwards compatible way. (Assuming users don't do a full pattern match on the config of course.

I believe the risk of the user using the same key in session storage for something else is pretty low, although we should probably add to the documentation what keys are used in session storage by default. - Will do!

I know I'm late to this, but I have a couple of questions:

a) What if I want to use cookies instead of session storage?
b) Is there a strong reason to not support storing token in cookies?

Hi @srid!

Where the state token is stored is considered an implementation detail, I don't see why one would care where it is stored (ok prerender ...) - as long as obelisk-oauth works as expected.

Also I am much in favor of implementing #4 which would do away with any kind of storage anyway and would also have a couple of other benefits as well (no page reloads, no lost state, no broken Requester workflow, ...)

Storing the access token (as oppposed to the state, that's just an internal security measure) in cookies should be fine over https. I don't like cookies in general because of their automatic sending semantics, which makes a bunch of attacks possible in the first place and it can be argued whether it is a good idea to send security sensitive information automatically on each request. On the other hand, this is standard practice for a lot of websites, you just need to be careful when you grant access based on that automatically transmitted token in order to prevent CSRF attacks and such.

Storing it in cookies so that you can avoid prerender should be fine.

ryantrinkle · 2019-02-27T21:20:19Z

frontend/Obelisk/OAuth/Frontend/Storage.hs

+{-| Internal module for handling local and session storage.
+
+   TODO: This should really go into some general purpose package and be exposed
+   there.


eskimor · 2019-02-28T13:00:25Z

I think this package can be considerably simplified. In particular, I think the logic regarding local storage can (and should) be eliminated, with the user providing the means for persistence and/or communication.

Without a default implementation this would reduce the usefulness of the package and would also increase the already quite big interface.

By providing a default implementation though, this could also be added later on in a backwards compatible way if needed without sacrificing ease of use in the common case. I can also add this right a way if you think it is worthwhile of course. If so, how should the interface look like? I was thinking of a record providing functions for storing and retrieval put in the Config.

Frontend is now based on Requester which might have been a stupid idea. As sending and receiving Events is a pretty bad abstraction if you need to map responses to requests.

(Does not yet typecheck.)

ryantrinkle · 2019-03-04T15:20:59Z

frontend/Obelisk/OAuth/Frontend/Command.hs

+  , command_getToken
+  ) where
+
+import Control.Monad.Free (Free (..), liftF, MonadFree (..))


What are the pros/cons of using Free versus asking the user to provide a record of functions?

I'll get back to this, have to get the contract sharing going now.

@ryantrinkle Sorry for the delay, I really had to get the contract sharing going. I thought using Requester would make sense here and needed a data structure in for Request that allows me to capture some context at request time and transfer it to response time. I realized that a Free monad (as any other monad) allows me to do that.

Especially with things like deriving via I believe a more pleasant API could be provided with a generic monad satisfying some type class like MonadStore/MonadOAuth or something.

Or of course I could also ditch Requester all together and just go with passing in a record of functions and use performEvent.

With deriving via the MonadStore approach could actually be very convenient to use - but probably overkill for this library.

All those approaches are pretty much equivalent, it's a matter of taste and of ergonomics and where we want to go in general with library design. As of now, I would not really go with Free as I don't see any real benefits of this approach.

By the way, I came to the conclusion that Request should almost always be a monad, at least when used in libraries. Would you agree on that or did I get something wrong?

eskimor added 10 commits February 20, 2019 20:00

Some refactoring in common.

4ac92c2

OAuth -> OAuthRoute

83c4630

and made it suitable for both backend routes and frontend routes.

Implemented OAuth user configuration.

e98b28f

Use Config in Request generation.

1479873

Config: Some cleanup + Show instance.

cd0c14b

Introduced OAuthError and frontend functionality.

11f02f2

Implemented backend functionality,

871d401

introduced Common module and did some cleanup.

Added some exports, some doc fixes.

3cd908f

Added missing error pretty printing.

5fb2beb

oauthRouteEncoder -> oAuthRouteEncoder

6161ef9

for consistency.

ryantrinkle requested changes Feb 27, 2019

View reviewed changes

eskimor added 10 commits March 3, 2019 00:06

WIP: Towards a working/usable implementation.

a86018b

Frontend is now based on Requester which might have been a stupid idea. As sending and receiving Events is a pretty bad abstraction if you need to map responses to requests.

Made common compile again.

d640c1a

Introduced Provider id parser that fails with OAuthError.

7c12b43

Finished frontend implementation so far.

9db61d3

(Does not yet typecheck.)

Made imports look ugly.

f93b3c2

Made code even uglier.

2af5b39

Made AuthorizationRequest typecheck.

2117fb0

Towards type-checking.

ccff723

Frontend type-checks.

661035d

Easy request - response mapping by use of free monad.

8829b15

ryantrinkle reviewed Mar 4, 2019

View reviewed changes

eskimor added 7 commits March 4, 2019 22:45

OAuthProvider -> IsOAuthProvider

cc1c8bd

Expose all frontend modules.

e38a8db

Added missing ToJSON/FromJSON instances.

bcb8685

New Error constructor.

3ef2a14

Additional OAuth error + missing instances.

ef5ba77

Basic backend implementation.

1cbe75c

Added some todos.

6770a9f

eskimor added 4 commits March 7, 2019 13:40

More error types & todos.

f727512

Ignore auth requests when an authorization is in progress.

358f420

Added another TODO item.

a61fa57

Fixed typo.

a423d70


		## 0.1.0.0 -- YYYY-mm-dd

		* First version. Released on an unsuspecting world.

Conversation

eskimor commented Feb 26, 2019

Uh oh!

ryantrinkle 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!

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!

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!

eskimor commented Feb 28, 2019

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants