Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Servant auth server PoC #1560

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Vlix
Copy link

@Vlix Vlix commented Mar 14, 2022

An attempt at making a better Auth instance of HasServer.
Also added an example of how this would make for easy adding of different auth procedures.

Any comments, critiques, etc. are very much welcome and appreciated.

, AllAuth auths a
, HasContextEntry ctxs (AuthHandler Request (NewAuthResult a))
) => HasServer (NewAuth mods auths a :> api) ctxs where
type ServerT (NewAuth mods auths a :> api) m =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense define the instance for NewAuth mods (auth ': auths) a :> api to statically ensure that at least one auth mode is defined ? We could define a custom type error for the case where the list of auths is empty.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, probably a good idea, yeah.

@gdeest
Copy link
Contributor

gdeest commented Mar 18, 2022

In the current implementation, all of the 'auths' are checked one by one, and the first that is present is tried and will either be returned when successful, or when not, throw an error given the appropriate 'ErrorFormatter' or return a 'Left err' if the 'Lenient' modifier is set. Only if NONE of the auths are present will it either throw an 'err401' or, if the 'Optional' modifier is set, return a 'Nothing'. This error might also be customizeable if we make it be required from the context.

Just to clarify my understanding here (I am not overly familiar with servant-auth). With the current Auth combinator, if two modes of authentication are defined (say, Cookie and JWT, in that order):

  • If a cookie is set, but authentication fails for some reason (e.g., invalid XSRF token), an error is raised.
  • If NO cookie is set, then JWT is tried.
  • If there is no JWT token, a 401 error is raised.
  • If there is a JWT token, but it proves to be invalid, some other error is raised.

Whereas you would prefer for subsequent auth schemes to be tried anyhow, and only return a 401 error when all of them fail ?

@gdeest
Copy link
Contributor

gdeest commented Mar 18, 2022

I very much like that the HasServer instance no longer mentions JWT or Cookie settings explicitly: this is a big wart in current design.

@Vlix
Copy link
Author

Vlix commented Mar 18, 2022

Just to clarify my understanding here (I am not overly familiar with servant-auth). With the current Auth combinator, if two modes of authentication are defined (say, Cookie and JWT, in that order):

  • If a cookie is set, but authentication fails for some reason (e.g., invalid XSRF token), an error is raised.
  • If NO cookie is set, then JWT is tried.
  • If there is no JWT token, a 401 error is raised.
  • If there is a JWT token, but it proves to be invalid, some other error is raised.

Whereas you would prefer for subsequent auth schemes to be tried anyhow, and only return a 401 error when all of them fail ?

No, my implementation works the same way. If the first auth returns a Nothing (meaning it's not there), it will try the next. But if it IS present, it is tried and taken as either succeeding (Right) or failing (Left) and it will return a 401 error if the modifier is Strict (or absent), but return the Left errMsg if the modifier Lenient is set.
-> The go helper function in processAllAuthHandlers shows it looping on Nothing and returning on Just x

In this way, if you'd want more than one authentication to be present and tried, you'd have to use two NewAuth mods auths a. i.e. "path" :> NewAuth '[] [Auth1] MyType :> NewAuth '[] [Auth2] MyType2 :> Get '[JSON] Result

But if you accept a JWT in the query parameters, a header or BasicAuth, it would just look like this and only one has to succeed:
"path" :> NewAuth '[] [JWTQueryParam, JWTHeader, BasicAuth] MyAuth :> Get '[JSON] Result

@Vlix
Copy link
Author

Vlix commented Mar 18, 2022

Note to self:

  • make stack build every Servant package with lts-18.27
  • make two instances of HasServer (NewAuth ...
    • TypeError ... => NewAuth '[]
    • NewAuth (auth ': auths)

@domenkozar
Copy link
Contributor

@Vlix do you need any help with this?

@Vlix
Copy link
Author

Vlix commented Feb 8, 2024

do you need any help with this?

I haven't spent time on this at all, so if you want to pick it up, or just give your own crack at it, please do :)

@tchoutri tchoutri marked this pull request as draft March 4, 2024 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants