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

Basic auth middleware #238

Merged
merged 7 commits into from
Dec 4, 2020
Merged

Conversation

tmattio
Copy link
Collaborator

@tmattio tmattio commented Dec 3, 2020

This PR adds a basic_auth middleware to protect handlers with an authentication mechanism.

It also adds an Auth module with functions to encode/decode Authorization header values.

The basic_auth middleware has type

val basic_auth
  :  ?unauthorized_handler:Rock.Handler.t
  -> key:'a Context.key
  -> realm:string
  -> auth_callback:(username:string -> password:string -> 'a option)
  -> unit
  -> Rock.Middleware.t

I believe it is flexible enough and will allow users to build their own authentication logic, without having to deal with the boilerplate logic.

TODO

  • Document the Auth module
  • Support Bearer, and maybe cookie?

@tmattio
Copy link
Collaborator Author

tmattio commented Dec 3, 2020

@Jerben I'm curious, do you have any authentication layer in Sihl? If so, would it be compatible with the changes in this PR?

Copy link
Contributor

@reynir reynir left a comment

Choose a reason for hiding this comment

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

I look over the changes and have some comments about the example, but also how 'other' authentication methods are handled.

example/user_auth/main.ml Outdated Show resolved Hide resolved
example/user_auth/README.md Outdated Show resolved Hide resolved
opium/src/middlewares/middleware_basic_auth.ml Outdated Show resolved Hide resolved
opium/src/request.mli Show resolved Hide resolved
@joseferben
Copy link
Collaborator

@Jerben I'm curious, do you have any authentication layer in Sihl? If so, would it be compatible with the changes in this PR?

@tmattio At the moment, we have that in the projects themselves. Your approach looks good and we could definitely use the middleware as proposed, I especially like the configurable rock handler 👍

opium/src/opium.mli Outdated Show resolved Hide resolved
example/user_auth/README.md Outdated Show resolved Hide resolved
@reynir
Copy link
Contributor

reynir commented Dec 4, 2020

It looks good to me now. Thanks for implementing it! 🎉

@tmattio
Copy link
Collaborator Author

tmattio commented Dec 4, 2020

It looks good to me now. Thanks for implementing it!

Thanks for the reviews, appreciate it! I'll investigate further to support Bearer / other auth methods before I merge it: I'm not confident enough that the current implementation generalizes well 🙂

@tmattio tmattio merged commit ded357f into rgrinberg:master Dec 4, 2020
@tmattio tmattio deleted the basic-auth-middleware branch December 4, 2020 16:54
tmattio added a commit to tmattio/opam-repository that referenced this pull request Dec 9, 2020
CHANGES:

## Added

- New `Auth` module to work with `Authorization` header (rgrinberg/opium#238)
- New `basic_auth` middleware to protect handlers with a `Basic` authentication method (rgrinberg/opium#238)
- New `Response.of_file` API for conveniently creating a response of a file (rgrinberg/opium#244)
- Add a package `opium-graphql` to easily create GraphQL server with Opium (rgrinberg/opium#235)
- Add a function `App.run_multicore` that uses pre-forking and spawns multiple processes that will handle incoming requests (rgrinberg/opium#239)

## Fixed

- Fix reading cookie values when multiple cookies are present in `Cookie` header (rgrinberg/opium#246)
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