feel free to close PR if you don't find it useful

Definitely sounds/looks useful! I can review the code tomorrow.

One thing to note here, you can add fixture tests to test the new behavior.

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (adbd67a) 70.23% compared to head (8d35ca8) 70.16%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master     #199      +/-   ##
==========================================
- Coverage   70.23%   70.16%   -0.08%     
==========================================
  Files          11       11              
  Lines         598      610      +12     
==========================================
+ Hits          420      428       +8     
- Misses        178      182       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Thanks for the reference!

I took a look at fixtures and think it makes sense to add new fixtures for not-exposed endpoints (list, version or delete-tokens not configured)

Otherwise I have mostly kept the behavior and there is coverage of the main cases.

Except probably one more case, which hasn’t been covered before: if auth-tokens unset/not configured - access is allowed (just to clarify, is this intended?)

There are unit tests ofc, but I’ll add fixtures a bit later for all these cases!

fix not-exposed endpoints to return 404 instead of 403 (they literally not intended to be exposed) - the only breaking change, can be extracted to separate PR

disabled endpoints return 403, but from the point of view of security and correctness it is more likely to be 404 - we literally do not expose them

I think it makes sense. We can mention this in the changelog as breaking.

lack of delete_tokens - hides the endpoint

The intention was to require tokens to delete files on the server. Do you think we should change that?

lack of auth_tokens - allows the access (btw, is it intended? because sound like a bug)
Except probably one more case, which hasn’t been covered before: if auth-tokens unset/not configured - access is allowed (just to clarify, is this intended?)

Yeah, this is intended. Do you think we should have an empty list of tokens for allowing access? It is not a big deal imo.

Another part of the changes is the introduction of actix-web-grants

I saw a derive macro on the endpoints that we want to protect but can you tell me how that works? Also it would be nice to get some information in terms of which version of Actix do you support, what are the plans for the future (will there be any breaking changes) etc.

I took a look at fixtures and think it makes sense to add new fixtures for not-exposed endpoints (list, version or delete-tokens not configured)
There are unit tests ofc, but I’ll add fixtures a bit later for all these cases!

Great! Just let me know if you have problems / need guidance while adding them.

lack of delete_tokens - hides the endpoint

The intention was to require tokens to delete files on the server. Do you think we should change that?

Not sure, the main confusion is that we have expose flag for list & version - which is really clear, but in case of delete it depends on delete_tokens. Not so clear when configuring the app, the first thing that comes to mind is that it should be 401 (any token won't match configured ones, they are unset), but in fact 404 seems safer in this case.

So I think we can keep it as is, probably improve the documentation related to the config - if delete_tokens unset then delete endpoint won't be exposed at all.

lack of auth_tokens - allows the access (btw, is it intended? because sound like a bug)

Yeah, this is intended. Do you think we should have an empty list of tokens for allowing access? It is not a big deal imo.

I thought more about explicit configuration flag, like disable_auth. Just looks less fragile when configuring the app, but the behavior itself is ok, just there was no any comments and I have been concerned. I've added comment & logging with this behavior anyway.

I saw a derive macro on the endpoints that we want to protect but can you tell me how that works?

Actually it works pretty straightforward:

• Middleware which extracts some "authority" entity from request (in our case we do extract TokenType)
• it adds specific type (AuthDetails<T>) to extensions
• macro actually helps to get rid of the following boilerplate, it expands to something like that:

async fn endpoint(details: AuthDetails) -> HttpResponse {
    if details.has_authority(TokenType::Auth) {
        // body of our function
    } else {
       // error we want to see, defaults to `Forbidden`
    }
}

But we can write it manually as well

Also it would be nice to get some information in terms of which version of Actix do you support, what are the plans for the future (will there be any breaking changes) etc.

actix-web was the first framework within this ecosystem, there is mention here: https://github.com/DDtKey/protect-endpoints/tree/main/actix-web-grants#supported-actix-web-versions, any major update of the actix-web gonna be supported with new major version of actix-web-grants

Recently I've delivered a huge update and I'd say it's likely long-term vision of API. As part of that update I also switched to release-plz (with cargo-semver-checks & git-cliff under the hood), so we have some validations.

Current macro-syntax flexible enough and should fit a lot of cases, including ABAC & RBAC, for example, you can combine the conditions in Rust-friendly way, using all & any:

// snippet from examples
#[actix_web_grants::protect(any("ADMIN", expr = "user.is_super_user()"), ty = "Role")]
async fn admin_or_super_user(user_id: web::Path<i32>, user: web::Data<User>) -> HttpResponse {
    HttpResponse::Ok().body("some secured response")
}

So I don't expect the breaking changes in the near future, and if they would be - it will be major (semver) update without breaking existing applications, and with back-porting the changes if they're compatible & makes sense.

Great! Just let me know if you have problems / need guidance while adding them.

I've added fixtures, it looks pretty clear to me, thanks!

Some not related observations:

• Would be nice to call teardown even in case of failure, to clean up
• macos: sleep doesn't work with time-units (e.g sleep 1s)
• macos: there is no fallocate (alternative would be to use dd if=/dev/random of=normalfile count=10000 bs=1024 status=none)

I think would be nice to ensure it works on any platform, or provide fast way to run it with docker for example.
See #200

So I think we can keep it as is, probably improve the documentation related to the config - if delete_tokens unset then delete endpoint won't be exposed at all.

Makes sense - would you be interested in making a PR to update README.md about this?

I thought more about explicit configuration flag, like disable_auth [...] I've added comment & logging with this behavior anyway.

👍🏼

Actually it works pretty straightforward:

Thanks for the explanation! It is more clear now.

I also switched to release-plz (with cargo-semver-checks & git-cliff under the hood)

Aye that's me! Let me know if you need help with any configuration.

Current macro-syntax flexible enough and should fit a lot of cases, including ABAC & RBAC, for example, you can combine the conditions in Rust-friendly way, using all & any:

Wow that's nice!

So I don't expect the breaking changes in the near future, and if they would be - it will be major (semver) update without breaking existing applications, and with back-porting the changes if they're compatible & makes sense.

Alright, sounds good.

Makes sense - would you be interested in making a PR to update README.md about this?

Take a look at #202, please

Aye that's me! Let me know if you need help with any configuration.

Yes, thanks for the wonderful tool! 🙏 Actually I've been using it for about a year on different projects! 😅

LGTM! Just one last thing.

I usually put the user-facing changes in the changelog so I guess the only things that have changed are:

• we now return 404 instead of 403 for non-exposed endpoints
• we don't allow blank (whitespace) tokens

Do we need to mention something else in the changelog?

In that case I think these two are enough, I'll update the changelog entry