udash-core icon indicating copy to clipboard operation
udash-core copied to clipboard

Introduced `AsyncAuthRequires`

Open catap opened this issue 3 years ago • 5 comments

This is simple wrapper around AuthRequires which works fine with Future[UserContext]. Such future is standard behaviour for DB-layer which is based on slick.

catap avatar Jan 18 '22 19:01 catap

@ddworak / @ghik what do you think about this one?

catap avatar Jan 18 '22 19:01 catap

Sorry for taking so long to review. TBH I don't think this is a good idea, major points would be:

  1. This approach unecessarily mixes the behaviour with underlying async backend (in this case Futures). This is not an argument enough alone, as you could reimplement this with F[_] etc. A more important point though is: if the authorization is not async, making it look so doesn't make the code better (just more complex). It's harder to test and, should this be the only variant, harder to use in general. It also doesn't seem clean to me if a service defines it's arguments as e.g.:
class MyService(userContext: Future[UserContext])

because this shows, that the implementation of other services heavily influences the API of this service and leaks underlying implementation.

  1. I don't think this provides enough value at callsite, specifically your tests could easily rewritten with a simple:
      for {
        ctx <- user
        _ = AuthRequires.require(P1.and(P2))(ctx)
      }

If you don't like providing this implicit explicitly, you can:

        _ <- user.map(implicit ctx => AuthRequires.require(P1.and(P2)))

For less lines consider

        _ <- user.map(AuthRequires.require(P1.and(P2))(_))

There's also a compiler plugin https://github.com/oleg-py/better-monadic-for#define-implicits-in-for-comprehensions-or-matches if you want to be able to declare an implicit within a for-comprehension - that would improve my first example.

  1. While I understand that fetching the context is often an async op, that asynchronicity is often hidden within a web application, as developers tend to:
  • store user context within the session
  • cache user context for token / etc. in memory (cache access doesn't have to be blocking or async)

ddworak avatar Feb 28 '22 14:02 ddworak

@ddworak I haven't forgotten about this PR but I'm a bit overloaded on another things and can come back to it soon.

catap avatar May 11 '22 08:05 catap

@ddworak I see you point and I can't dismiss it.

From another hand I have slick as ORM which returns Future[_] and here I have a lack of understanding how can I implement a trivial authorisation.

Maybe you may suggest better approach?

catap avatar Jul 17 '22 11:07 catap

@catap I've prepared a small example on top of basic udash.g8 template: https://github.com/ddworak/udash-async-auth/commit/a8b21baf1fb87466a9efa7bb7c9abbdaeaaf14b2

If we were not relying on the fact that findUserCtx can be synchronous (due to cache), we'd have essentially have to either block when creating SecureRpc in io.company.app.backend.rpc.ExposedRpcInterfaces#secure or add the capability to return Future[Rpc] in the RPC framework. Usually it doesn't come all the way to that.

ddworak avatar Aug 02 '22 14:08 ddworak

Closing as stale - feel free to reopen or add another issue or PR.

ddworak avatar Feb 06 '23 06:02 ddworak