DDtKey / protect-endpoints

Authorization extension for popular web-frameworks to protect your endpoints
Apache License 2.0
214 stars 16 forks source link

404 instead of 403 for Scope #7

Closed RoDmitry closed 3 years ago

RoDmitry commented 3 years ago

I know it's not a documented use case, but it works and somehow returns 404 error instead of 403.

.service(
    web::scope("/admin")
        .service(index)
        .service(users)
        .guard(PermissionGuard::new(ROLE_ADMIN.to_string()))
);

Was checked on v3.0.0-beta.1

RoDmitry commented 3 years ago

And the way it returns errors, I think it's double checking roles in this example:

use actix_web::{
    get,
    web,
    web::ServiceConfig,
    Responder,
    HttpResponse,
};
use actix_web_grants::{
    proc_macro::has_permissions,
    PermissionGuard,
};

const ROLE_ADMIN: &str = "ROLE_ADMIN";

#[get("")]
// I know I can type #[has_permissions("ROLE_ADMIN")] on every function, instead of .guard
async fn index() -> impl Responder {
    HttpResponse::Ok().body("index")
}

#[get("/users")]
#[has_permissions("ROLE_SUPER_ADMIN")]
async fn users() -> impl Responder {
    HttpResponse::Ok().body("users")
}

// --------------------- Paths config ------------------------
pub fn configure(cfg: &mut ServiceConfig) {
    cfg
        .service(
            web::scope("/admin")
                .service(index)
                .service(users)
                .guard(PermissionGuard::new(ROLE_ADMIN.to_string()))
        );
}

At first it checks if you have ROLE_ADMIN, and then if you are also ROLE_SUPER_ADMIN. Is that possible to avoid double checking, and prioritize on the function's #[has_permissions()]?

DDtKey commented 3 years ago

I know it's not a documented use case, but it works and somehow returns 404 error instead of 403.

.service(
    web::scope("/admin")
        .service(index)
        .service(users)
        .guard(PermissionGuard::new(ROLE_ADMIN.to_string()))
);

Was checked on v3.0.0-beta.1

Yes it is, the reason is in the way Guard is processed by the actix-web itself (it only expects a bool) actix guard doc

RoDmitry commented 3 years ago

So is it just a misuse or it could be fixed? I mean the error code and the double checking.

DDtKey commented 3 years ago

At first it checks if you have ROLE_ADMIN, and then if you are also ROLE_SUPER_ADMIN. Is that possible to avoid double checking, and prioritize on the function's #[has_permissions()]?

You can separate services (or scopes), for example:

    cfg
        .service(
            web::scope("/admin")
                .service(web::to(index).guard(PermissionGuard::new(ROLE_ADMIN.to_string())))
                .service(users)             
        );

But combining them on the same services/scopes will check for both. Guard is more primary.

DDtKey commented 3 years ago

So is it just a misuse or it could be fixed? I mean the error code and the double checking.

The HTTP code in the case of Guard is the expected behavior (on the actix-web lvl). This can be used if you are satisfied with the 404 code. In general, many people prefer to use 404 error codes instead of 403 in such cases.

RoDmitry commented 3 years ago

So the only way to avoid double checking is by repeating PermissionGuard for each service in scope, because of how actix-web handles it, right?

RoDmitry commented 3 years ago

many people prefer to use 404 error

That's strange. I wouldn't. How do I change it to return 403? Avoid using it for scopes I guess?

DDtKey commented 3 years ago

So the only way to avoid double checking is by repeating PermissionGuard for each service in scope, because of how actix-web handles it, right?

Not necessary for every, you can combine a set of services with a common scope and one PermissionGuard 🤔

Also you can use only a macro (on each handler) #[has_permissions(...) (without guard)

DDtKey commented 3 years ago

many people prefer to use 404 error

That's strange. I wouldn't. How do I change it to return 403? Avoid using it for scopes I guess?

I'm afraid the Guard mechanism does not allow us to override the response code 🙁 Actix guards only return 404 or allow access to the resource

So yeah, for now, the best solution would be to use a macro or manual check (of your choice)

RoDmitry commented 3 years ago

But PermissionGuard for each service or macro - return 403, except PermissionGuard on Scope. How so?

DDtKey commented 3 years ago

But PermissionGuard for each service or macro - return 403, except PermissionGuard on Scope. How so?

Macro return 403 - it's defined by current lib (I have plans to allow custom error codes) But PermissionGuard always should return 404 as I know (defined by actix-web). Are you sure? 🤔 As in the example, if we change the role in the guard, it will return 404

RoDmitry commented 3 years ago

I thought it's the same, need to check

DDtKey commented 3 years ago

This is good point 👍 I think need to describe this behavior in the documentation and readme (regarding the response code) 🤔

RoDmitry commented 3 years ago

Yeah, my bad, I didn't check. Попутал немного ☺

RoDmitry commented 3 years ago

This is good point 👍 I think need to describe this behavior in the documentation and readme (regarding the response code) 🤔

Would be great and very useful for newcomers.

RoDmitry commented 3 years ago

Maybe there is a possibility to pass something to get 403 as macro does?

DDtKey commented 3 years ago

Maybe there is a possibility to pass something to get 403 as macro does?

I will study this question a little later, and if possible, we will definitely do it at the library level. But at the moment, I do not know about such a possibility 🙁

RoDmitry commented 3 years ago

I found out how Guards work, and I found the way to send whatever you want. Guard just checks whether that service suits some conditions or not, and in that case just skips it. You can just make another service, with the same link, easy) Check it out. Is that enough?

DDtKey commented 3 years ago

I found out how Guards work, and I found the way to send whatever you want. Guard just checks whether that service suits some conditions or not, and in that case just skips it. You can just make another service, with the same link, easy) Check it out. Is that enough?

Guards can really only be used for routing purposes.

So, yeap, this option will work. Unfortunately this looks like a workaround (not very convenient), but perhaps this is the only option that we have for the Guard way.

RoDmitry commented 3 years ago

Do you need this? #9 I mean is that the way to update readme?)

DDtKey commented 3 years ago

Do you need this? #9 I mean is that the way to update readme?)

I would like to add this, thanks 🙂

RoDmitry commented 3 years ago

We forgot about Scope. I wrote some docks for it. Check here #10. It returns redirect. Is it ok, or should it just return 403? It was easier to explain how it works having redirect there)