limosa-io / laravel-scim-server

SCIM 2.0 Server implementation for Laravel
MIT License
51 stars 29 forks source link

API protection for routes in an existing project #15

Closed uberbrady closed 2 years ago

uberbrady commented 2 years ago

Hi there! Thank you so much for this spectacular project! I was able to get it installed into our existing project pretty easily, and was shocked that it managed to figure out how my users were supposed to look with almost no intervention on my part. It's great tech and I can't wait to get it into the hands of our users.

However, where I'm running into a couple of snags is trying to hook it into our existing middleware in order to protect the routes properly. We're already using some API middleware which includes things like API token lookups and throttling and things like that.

My first attempt was to turn off the automatic route injection, and try to insert the routes in myself, with the appropriate middleware - but that started failing with strange dependency injection lookup failures that I wasn't able to really get anywhere with. I still have that on a branch if you tell me that's the right way to go, though.

My second attempt was to insert a global middleware which tries to do the appropriate restriction of the routes (as suggested by: https://github.com/arietimmerman/laravel-scim-server/issues/7 ). I basically look for any route that starts with /scim and then I allow only specific routes and block everything else. But my problem here is that the actual lookup of the API keys I think only happens at the route-middleware level, and not at the global-middleware level, so I'm not sure I'm going to be able to get this one to work via API key. I was able to get it to work via regularly-logged-in-user which was nice to test if my logic was working right, but the API keys are going to be the critical path.

My third attempt is going to be to try a simple static key lookup, and I will have to make my users insert that into their .env (presuming I can pull stuff out of the .env or config() at the global middleware layer?) That still gets this amazing feature into my user's hands, but I already have all of this nice Passport stuff with bearer tokens and permissions and whatnot that I really would love to be able to leverage for SCIM integration. And whenever I ask my users to adjust their .env it usually makes it much harder for my users to be able to use that feature (and it adds some burden to our helpdesk, as well).

Any ideas about any kind of different direction I should take, or does one of these seem like the right way to go? I'll be happy to contribute back to this project in any way with whatever our findings are.

Thanks again for making this, and sharing it. It's amazing that you can get SCIM support up and running so quickly with just a few short steps.

arietimmerman commented 2 years ago

Thanks for informing me that there is an issue here. And good to see all the research you have done.

I will take a look at this later. Probably this evening (CET). The described use case should work, but apparently it doesn't.

uberbrady commented 2 years ago

Thanks! It's also very possible that I'm doing something wrong, and if so I'll be happy to be corrected, of course :)

arietimmerman commented 2 years ago

I believe the problem is routes are always loaded. You can disable this by creating a file config/scim.php with the following contents.

<?php
return [
    "publish_routes" => true
];

Now you can do something like this:

Route::middleware('auth:api')->group(function () {
    ArieTimmerman\Laravel\SCIMServer\RouteProvider::routes(
        [
            'public_routes' => false // do not hide public routes (metadata) behind authentication
        ]
    );

    ArieTimmerman\Laravel\SCIMServer\RouteProvider::meRoutes();
});

ArieTimmerman\Laravel\SCIMServer\RouteProvider::publicRoutes()

This is not documented but it should be.

uberbrady commented 2 years ago

I will give this a shot and if it does work I will definitely get you a PR. If it doesn't, I'll try and share what I have and maybe we can figure it all out.

Thanks for your help! I will get back to you as soon as I can.

uberbrady commented 2 years ago

Did you mean:

<?php
return [
    "publish_routes" => false
];

I think so, but just checking.

uberbrady commented 2 years ago

Okay, I think I've got it - got to do a little bit of cleaning, then I'll submit a PR with my recommended documentation changes. Thanks for the help!!!

uberbrady commented 2 years ago

So a few challenges left - though I'm still poking around on my end to see if I can make any progress -

  1. The public routes are throwing weird Unauthenticated errors. When I look at route:list it doesn't seem like there's any middleware at all on those routes, but something is kicking in and making things break. I have no idea what. What's even weirder - if I turn off the public routes entirely, that still happens. Sounds like some kind of global middleware, but I created a Dummy global middleware that just calls Log and loaded it dead last - and it does end up firing. Meaning the global middleware stack has all run completely, and so the problem can't be there...my current thinking is that maybe it's something in Passport...still poking around and hoping.
  2. I use a directive @routes (from a package called Ziggy) in my default layout blade to spit out a javascript-based route list for my Vue views and stuff like that - but that directive now completely fails, giving a weird message about trying to instantiate an Eloquent 'model' directly. Not so worried about this one because I intended to remove it anyways, and if I can get SCIM support working I sure as hack will accelerate my plans to remove Ziggy.

Anyways I can't tell if these are my fault or something in the package, but I'll keep working on this and report back as soon as I have anything substantive..

Thanks again for all of your help!

uberbrady commented 2 years ago

(Oh, and here's my PR with the changes I have so far in case you see anything particularly glaring: https://github.com/snipe/snipe-it/pull/10802/files )

uberbrady commented 2 years ago

Okay, I'm pretty sure I've definitively fixed problem #1 - you have to do the public routes first - otherwise the fallbacks that are defined in the privateroutes will take over. That was pretty nasty to troubleshoot. Whew. I'll commit what I have, push, and then do some cleanup so I can submit a nice PR for how others can do something similar to what I'm doing here.

Any advice for problem number two there? Definitely open to advice on that one - but that sounds very much like a 'me problem' and not a 'you problem'. The error I'm getting specifically is:

Target [Illuminate\Database\Eloquent\Model] is not instantiable. (View: /Users/uberbrady/Documents/grokability/snipe-it/resources/views/layouts/default.blade.php)
uberbrady commented 2 years ago

Hey there - I know you must be slammed with other stuff - but I'm wondering if that error means anything to anyone? I am using this Laravel package called tightenco/Ziggy (it spits out Laravel routes into Javascript), and Ziggy's @routes directive for Blade errors out with the above-mentioned error whenever I hit any page with @routes anywhere in it. Which, unfortunately for me, is in our default layout blade.

I'm trying to work-around the problem and get rid of any of those calls, but it's slow going and I didn't quite want to get rid of this package yet.

Any ideas about that message; any thoughts as to places I might be able to check out to try and resolve them?

uberbrady commented 2 years ago

I ended up yanking out Ziggy and my implementation works fine now. I'll be happy to close this issue and open a new one just about Ziggy. Or just close this and move along, that's fine too :)

arietimmerman commented 2 years ago

Great to hear it works now. Let's close this issue for now.