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

RestInterfaceClient with @requiresAuth service #1648

Closed
tchaloupka opened this Issue Dec 29, 2016 · 3 comments

Comments

Projects
None yet
3 participants
@tchaloupka
Contributor

tchaloupka commented Dec 29, 2016

When trying new vibe.web.auth facility it works ok on the server side, but I've a problem with the client side where it demands:

../../../../../.dub/packages/vibe-d-0.7.30/vibe-d/source/vibe/web/auth.d(238,6): Error: static assert  "IFooService must have an authenticate(...) method that takes HTTPServerRequest/HTTPServerResponse parameters and returns an authentication information object."
../../../../../.dub/packages/vibe-d-0.7.30/vibe-d/source/vibe/web/auth.d(252,51):        instantiated from here

How is this supposed to work? I tried to add the authenticate method to the interface itself, but then there is a problem with registering the REST service because it tries to register route for it.
Also tried the @noroute attribute, but it does not work with REST.

I'm currently setting the authentication header through REST clients requestFilter but new auth is somehow stepping in the way.

Is it possible for RestInterfaceClient to somehow ignore the @requiresAuth on the interface?

Thx

@Extrawurst

This comment has been minimized.

Show comment
Hide comment
@Extrawurst

Extrawurst Dec 30, 2016

Contributor

I think you only need this method on a global static level, see example here: https://github.com/rejectedsoftware/vibe.d/blob/master/examples/rest/source/app.d#L296

Contributor

Extrawurst commented Dec 30, 2016

I think you only need this method on a global static level, see example here: https://github.com/rejectedsoftware/vibe.d/blob/master/examples/rest/source/app.d#L296

@tchaloupka

This comment has been minimized.

Show comment
Hide comment
@tchaloupka

tchaloupka Dec 30, 2016

Contributor

The example shows authentication using the @before / @after attributes but not the new @requiresAuth, @anyauth, etc. in vibe.web.auth.

Tried to move the authenticate method outside the service itself and with it it complains the same way:

../../../../../.dub/packages/vibe-d-0.7.30/vibe-d/source/vibe/web/auth.d(238,6): Error: static assert  "FooService must have an authenticate(...) method that takes HTTPServerRequest/HTTPServerResponse parameters and returns an authentication information object."
../../../../../.dub/packages/vibe-d-0.7.30/vibe-d/source/vibe/web/auth.d(252,51):        instantiated from here: impl!0LU
../../../../../.dub/packages/vibe-d-0.7.30/vibe-d/source/vibe/web/auth.d(246,15):        instantiated from here: AuthInfo!(EventService, IEventService)
../../../../../.dub/packages/vibe-d-0.7.30/vibe-d/source/vibe/web/auth.d(247,45):        instantiated from here: cimpl!1LU
../../../../../.dub/packages/vibe-d-0.7.30/vibe-d/source/vibe/web/auth.d(253,24):        ... (2 instantiations, -v to show) ...
../../../../../.dub/packages/vibe-d-0.7.30/vibe-d/source/vibe/web/rest.d(79,14):        instantiated from here: RestInterface!(FooService)
source/app.d(31,30):        instantiated from here: registerRestInterface!(FooService)

The new auth facility seems to be much cleaner way to handle this. I didn't like the need to add extra parameter using @before attribute to the method even it is of no use there. So we went the way to partition service per authentication/authorization needs and register them separately in router with extra auth handlers between. It works, but bloats the router. So we would like to use the new auth facility for this and client side seems to be the only problem holding this change back.

See here: https://github.com/rejectedsoftware/vibe.d/blob/master/web/vibe/web/auth.d#L39

Contributor

tchaloupka commented Dec 30, 2016

The example shows authentication using the @before / @after attributes but not the new @requiresAuth, @anyauth, etc. in vibe.web.auth.

Tried to move the authenticate method outside the service itself and with it it complains the same way:

../../../../../.dub/packages/vibe-d-0.7.30/vibe-d/source/vibe/web/auth.d(238,6): Error: static assert  "FooService must have an authenticate(...) method that takes HTTPServerRequest/HTTPServerResponse parameters and returns an authentication information object."
../../../../../.dub/packages/vibe-d-0.7.30/vibe-d/source/vibe/web/auth.d(252,51):        instantiated from here: impl!0LU
../../../../../.dub/packages/vibe-d-0.7.30/vibe-d/source/vibe/web/auth.d(246,15):        instantiated from here: AuthInfo!(EventService, IEventService)
../../../../../.dub/packages/vibe-d-0.7.30/vibe-d/source/vibe/web/auth.d(247,45):        instantiated from here: cimpl!1LU
../../../../../.dub/packages/vibe-d-0.7.30/vibe-d/source/vibe/web/auth.d(253,24):        ... (2 instantiations, -v to show) ...
../../../../../.dub/packages/vibe-d-0.7.30/vibe-d/source/vibe/web/rest.d(79,14):        instantiated from here: RestInterface!(FooService)
source/app.d(31,30):        instantiated from here: registerRestInterface!(FooService)

The new auth facility seems to be much cleaner way to handle this. I didn't like the need to add extra parameter using @before attribute to the method even it is of no use there. So we went the way to partition service per authentication/authorization needs and register them separately in router with extra auth handlers between. It works, but bloats the router. So we would like to use the new auth facility for this and client side seems to be the only problem holding this change back.

See here: https://github.com/rejectedsoftware/vibe.d/blob/master/web/vibe/web/auth.d#L39

tchaloupka added a commit to tchaloupka/vibe.d that referenced this issue Dec 31, 2016

@s-ludwig s-ludwig closed this in 992d699 Feb 2, 2017

s-ludwig added a commit that referenced this issue Feb 2, 2017

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig Sep 27, 2017

Member

This needs to be changed a bit again. The current code incorrectly treats authentication information parameters as body parameters in the REST client, so that they get serialized and sent to the server. To avoid this, the type of those parameters needs to be known. Two changes make this possible now:

  • @noRoute is now supported (#1934)
  • @requiresAuth!T can be used to pass just the type without defining an extra method (#1939)
Member

s-ludwig commented Sep 27, 2017

This needs to be changed a bit again. The current code incorrectly treats authentication information parameters as body parameters in the REST client, so that they get serialized and sent to the server. To avoid this, the type of those parameters needs to be known. Two changes make this possible now:

  • @noRoute is now supported (#1934)
  • @requiresAuth!T can be used to pass just the type without defining an extra method (#1939)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment