udash-core
udash-core copied to clipboard
Introduced `AsyncAuthRequires`
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.
@ddworak / @ghik what do you think about this one?
Sorry for taking so long to review. TBH I don't think this is a good idea, major points would be:
- This approach unecessarily mixes the behaviour with underlying async backend (in this case
Future
s). This is not an argument enough alone, as you could reimplement this withF[_]
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.
- 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.
- 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 I haven't forgotten about this PR but I'm a bit overloaded on another things and can come back to it soon.
@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 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.
Closing as stale - feel free to reopen or add another issue or PR.