Falco icon indicating copy to clipboard operation
Falco copied to clipboard

Change `isInRole` to accept a generic list instead of a list of strings.

Open ericsampson opened this issue 4 years ago • 5 comments

Allow passing in enum-style DUs etc, gives a nice typed experience for the user.

ericsampson avatar Dec 07 '20 23:12 ericsampson

What's up Eric?!

First off, amazing PR. Truly. The quality of this is phenomenal.

I like where the idea is going, I agree that the `is it's currently a little low-level/less then elegant. However, I wonder if we can maybe arrange this a bit differently.

The one thing I love about how the framework has come together so far, is that it does very little out of the ordinary. Leaving a slim margin for error to occur, mostly by not trying to do too much. In this case, since the end game is string existence. I prefer the outer/Falco api to do the same.

With that said, I wonder if we maybe add a handler, either in the Request or Auth module, called Auth.mapPrincipal with a rough shape of:

let mapPrincipal (authScheme : string) (map : ClaimsPrincipal -> 'a) (next : 'a -> HttpHandler) = 
    fun ctx -> next (ctx.GetUser() |> map) ctx

With that in place, you're free to project the "dumb" Principal, into an enhanced F# type with whatever capabilities you need which I'd argue is quite project-specific (as auth tends to be). Leaving you free to do something like:

type MyUserRoles = Admin | SuperUser | RegularUser

type MyUserType = { Role : MyUserRoles // ... }

let myUserInRole (roles : MyUserRoles list) (u : MyUserType) = // ..

pimbrouwers avatar Dec 08 '20 14:12 pimbrouwers

It's your baby, so whatever you think makes sense I'm game!

Re your suggestion, if you don't think this general avenue is worth investigating/supporting, don't worry about saying so. I don't want to add complexity that doesn't seem to add value, just because I created an initial PR :)

If you do like the idea of the approach that you mentioned, I can create a new PR, just LMK!

Cheers

ericsampson avatar Dec 09 '20 01:12 ericsampson

Morning Eric!

Above all else I appreciate that you're interested enough in the project to be thinking of ways to enhance it to suit your needs.

My fear with supporting DUs in this spot is that it's a slippery slope, given how complex they can become and the reflective nature of having to introspect details about them.

I think if we support a simple mechanism for projecting the "dumb" Principal & Claims, into a rich F# object, we're on a good trajectory. I'm even thinking that we could leverage one of my favourite little pieces in the project, the StringCollectionReader to provide a method of obtaining values from the Principal, in a way that's uniform with the rest of the framework. Ultimately, the Principal is nothing more than a fancy abstraction atop a collection of string key/values.

Something like:

type ClaimsPrincipalReader (principal: ClaimsPrincipal) =
  let principalMap = //...
    inherit StringCollectionReader (principalMap)

module Request =
    let bindPrincipal 
        (authScheme : string) 
        (map : ClaimsPrincipalReader -> Result<'a>) 
        (handleOk : 'a -> HttpHandler)
        (handleError : 'a -> HttpHandler) = 
        fun ctx -> // ...

Thoughts?

pimbrouwers avatar Dec 12 '20 12:12 pimbrouwers

Sounds good, I'll give it a play over the next couple weeks. Do you have the ability to convert this PR into a draft? I don't.

ericsampson avatar Dec 12 '20 19:12 ericsampson

Sounds great. Converted to draft!

pimbrouwers avatar Dec 13 '20 12:12 pimbrouwers