actix-web update to 1.x
Closed this issue ยท 26 comments
Project Improvement
actix-web
1.x has some breaking changes. The library and examples need to be updated to remain useful.
Tracking pull request
- A pull request (does not yet exist)
Here's something interesting I just realized: Actix Web 1.0 with the secure-cookies
feature depends on ring 0.14, which is incompatible with ring 0.13... I'm not sure how to resolve this
Potential ways:
- Have multiple features that allow selecting a compatible version of
ring
inoxide-auth
. Finally somewhat sane since frontends can each select their exclusive features. - Remove the dependency on
ring
. This would be a very slightly breaking change as the HMAC key can currently be converted from aring
struct. The potential ofring
is barely used so this doesn't require looking for another general purpose crypto library. But needs some auditing of that library, I believe the others are not as popular.
ring = { version = "0.14", optional = true }
ring13 = { version = "0.1.0", optional = true }
Using a ring shim crate doesn't work, even if both ring
and ring13
are optional dependencies.
It seems that all crates in a workspace need to lead to a single selection of packages and version, and linking collisions from native libraries are also checked across all these packages. Otherwise it would work to specify manually a SemVer range like so (not-withstanding potential breaking changes to ring
which affect compatibility):
ring = ">=0.13,<0.17"
Maybe we can 'just' get rid of the workspace and build each separately.
Hm, 0.14
has a breaking change to https://docs.rs/ring/0.14.0/ring/pbkdf2/fn.verify.html which is in use. This is not so simple, needs to be features.
Cheeky idea: turn iterations
into a NonZeroU32
and use self.iterations.into()
. This should have the correct behaviour in all versions. Once it is a no-op and for 0.13
it selects From<NonZeroU32> for u32
.
I've gotten rid of the workspace entirely on my box and i'm still not able to compile a package that optionally depends on ring13 and ring14
Cargo doesn't seem to like it, can't have conflicting optional dependencies it seems. The whole workspace needs to resolve to the same ring version though, as well. Maybe a separate directory just for testing both/all possible ring versions should be added outside the workspace. Then at least the current setup can stay.
Any update on this issue ?
@gzp-crey there's a branch of this repo supporting actix 1.0, but it hasn't been released yet
Specifically, you can specify these dependencies for the moment:
[dependencies]
oxide-auth = { git = "https://github.com/HeroicKatora/oxide-auth/", branch = "next" }
oxide-auth-actix = { git = "https://github.com/HeroicKatora/oxide-auth/", branch = "next" }
In the actix examples there is a Vacant solicitor for the endpoint and some custom
solicitor in the Actor handler.
What is the use of this Extra type that cannot be achieved through the solicitor of the endpoint?
It also seems to be actix specific and as such I'm not sure if it is a good idea to add framework specific extensions. If it is valid only for one framework one cannot achieve the same with another framework and makes the featureset of the core lib diverge for each frontend.
For me it is quite confusing.
The reason there is a vacancy for the solicitor is that it needs to be different depending on the route. This is the same pattern as seen with the Rocket example: https://github.com/HeroicKatora/oxide-auth/blob/next/oxide-auth-rocket/examples/rocket.rs#L142
Ok, but if it is different by route, than why is there no distinction between the two calls ?
As I've checked all the frameworks provide a different method for the two calls.
I mean for me it means that, there is no single solicitor for the endpoint but at least two for get authorize, post authorize.
I haven't checked where the solicitor is triggered, but by this at least there are two distinct role for the same object. It does not gets the context why and when was it called. Wouldn't it be better to have
- one solicitor with multiple function at trait level
- one solicitor with an additional "reason" parameter for the call.
- or have two distinct solicitor (less preferred)
This way there is no need to differentiate the frontends and solicitor invocation can be made general.
No need to call with_solicitor function (iron, rocket), no need to implement handlers (actix).
The reason State implements Actor in the actix version is because we need to be able to modify a shared state. We don't do this with a Mutex because we don't want to block threads that should be processing requests asynchronously. Serializing through an actor makes some amount of sense for a simple example.
Once actix-web updates to futures 0.3, the example could be updated to use a futures::lock::Mutex
instead of an actor.
I'm not agains actor and handler. They are great. (And the new async rust feature is great as well and i like them a lot).
I just don't like the current solicitor implementation (or there are ather invocations i don't understand yet)
The one associated to the endpoint is not used, but a completly different one have to be created for the authorize routes.
Is this solicitor triggered from other places? If so why aren't these invocations customized by the route ?
- they don't need customization. Than a context won't really bother them. And the whole functionality can be implemented in a single struct.
- or the example is too simple and it did not require customization. Than the global solicitor ha no use since in a real app they have to be replaced anyway.
edit
The other traits (registrar, issuer, etc) are great and they perform their task from end to end. This solicitor just simple does not fits into this concept and the logic is split up in the source in the customized version
The only time a Solicitor is used is during the Authorization operation, which requires a solicitor, but the solicitor may be different depending on what route is hit.
It might be reasonable to implement a single solicitor that itself contains Option types or something to produce different behavior. I'm not exactly sure how this would be implemented
It might also be reasonable to look more closely at the authorization operation to determine if we could split the single solicitor into multiple concepts to remove the need for with_solicitor
I wonder if instead of Generic
taking a Solicitor
, it took a Fn(T) -> Box<dyn Solicitor>
, which could allow for a single solicitor to handle all cases. I'm not a huge fan of that pattern, though.
What is the use of this Extra type that cannot be achieved through the solicitor of the endpoint? It also seems to be actix specific and as such I'm not sure if it is a good idea to add framework specific extensions.
It is indeed actix
specific but only to integrate with the message system in the actor part. So I wouldn't consider it a bad thing. It only ensures that the oxide-auth-actix
crate can provide a struct that has an implementation of actix::Message
, nothing more. You can use your own messages or no message at all and choose to ignore it.
Vacant
as Solicitor
is a bit of an abuse of the type system, but the best one I found to make Generic
work as an endpoint implementation. In any system that is more extensive than the example you could use a proper implementation of oxide_auth::Endpoint
to fully replace the Generic
struct. It just, due to Rust's trait system, adds quite a bit of boilerplate code that would make the example harder to read.
I've meant something like. I'm not an expert, so it might not make too much sense :).
enum SolicitReason {
AuthorizeRequest, //(form data)
Authorize //(the real user info)
}
pub trait OwnerSolicitor<Request: WebRequest> {
fn check_consent(&mut self, reason: SolicitReason, &mut Request, pre_grant: &PreGrant) -> OwnerConsent<Request::Response>;
}
Thanks for the oxide_auth::Endpoint
i'll take a look at it.
I've played with this solicitor and now I think I understand it a little bit better.
The solicitor in the "global endpoint state" (web::data) is not really used at all. The authorize endpoints generates a new endpoint with a replaced solicitor and invokes the flow on this new one.
In the example maybe it'd be better to drop the "Extras" enum and make the Solicitor the extra parameter for the wrap and handler:
impl<Op, S> Handler<OAuthMessage<Op, S>> for AuthState
where
Op: OAuthOperation,
S: OwnerSolicitor<OAuthRequest>
{
type Result = Result<Op::Item, Op::Error>;
fn handle(&mut self, msg: OAuthMessage<Op, S>, _: &mut Self::Context) -> Self::Result {
let (op, solicitor) = msg.into_inner();
op.run(self.endpoint.with_solicitor(solicitor))
}
}
Also I'd consider creating a Generic and a GenericWithSolicitor (created from Generic using the with_solicitor() builder function).
Actix recently released a 2.0 alpha based on async/await so that may be something to look into, also
@asonix I've tried to port it to actix_web 2.0 but it seems as actix is not std::future compatible yet. Thus either the actor pattern have to be dropped or we have to wait for actix: actix/actix#298 (comment)
After checking the actix and future 0.3 related tickets it seems as the owner of actix_web is not too willing to update actix and don't provide alternative solutions. So here is my draft to fix the issue.
New hope for actix with actix-web 2.0:
actix/actix#300
oxide-auth-actix
has been published with version 0.0.1
. This supports version 1.0
of actix-web
and thus closes this issue. Discussion around 2.0
should move to #71 instead.