IdentityServer / IdentityServer3.WsFederation

WS-Federation Plugin for IdentityServer v3
Apache License 2.0
25 stars 36 forks source link

Custom SignInValidation/SignInValidator equivalent to ICustomRequestValidator #10

Closed VictorBlomberg closed 9 years ago

VictorBlomberg commented 9 years ago

Hi,

If you want to restrict user access for some clients, i.e. only allow authorize or token requests for some users to certain clients, you can use the ICustomRequestValidator for non-WS-Federation requests (it gives you both subject and client, which is enough to make such a decision).

But the WsFederation plugin seems to lack such extension points. While replacing SignInValidator would be enough (with subject and realm, there is enough information to make a decision), that seems to be impossible to do from the outside, i.e. without forking?

Would it make sense to add an optional extension point like ICustomSignInValidator, or something similar? That would be very helpful for my usage scenario. Or at least make the SignInValidator overridable?

Thank you for this immensely useful project!

brockallen commented 9 years ago

Possibly. We'll leave this open to discuss.

VictorBlomberg commented 9 years ago

Making the method ValidateAsync in SignInValidator virtual (SignInValidator.cs:40), would allow this functionality using the DI/Registrations feature (while still not making a big fuzz or doing anything that would increase the support burden).

VictorBlomberg commented 9 years ago

Hi,

As it is now SignInValidator is impossible to replace without forking the whole repository, which is a mess. Is there anyway I can elaborate to persuade you to make the change stated above, making ValidateAsync virtual?

Or let SignInValidator implement an ISignInValidator interface?

I would be happy to create a PR if it would be of any help.

(I'm also on gitter in case you want to discuss there)

leastprivilege commented 9 years ago

What do you want to achieve?

leastprivilege commented 9 years ago

Sorry I missed the first part.

What would you prefer - an extensibility point or make the validator replaceable (with decent base class) ?

VictorBlomberg commented 9 years ago

(Sorry for the silence!)

Well, for me an extensibility point with subject and realm allowing to allow or deny the subject to the realm would be enough, and thus the easiest for me.

But a replaceable validator would do the job too, still being really simple. It really doesn't matter that much for my use case, both would do a great job.

leastprivilege commented 9 years ago

ok - will do something. put it on my todo.

VictorBlomberg commented 9 years ago

Wonderful! Just tell me if I can help with anything.

leastprivilege commented 9 years ago

Well - if you want open a PR with a custom request validator that runs at the end of the internal validator. Similar to the one in core.

VictorBlomberg commented 9 years ago

Sure thing!

VictorBlomberg commented 9 years ago

Hi, should I PR against master or dev?

leastprivilege commented 9 years ago

dev

leastprivilege commented 9 years ago

done