square / rails-auth

Modular resource-based authentication and authorization for Rails/Rack
Apache License 2.0
291 stars 26 forks source link

Add linting for the ACL file #46

Closed cthornton closed 3 years ago

cthornton commented 7 years ago

This adds a linter for the ACL file. Sometimes when deleting large amounts of ACL changes, it may be easy to accidentally forget to remove something, specifically duplicate keys.

For example, if you're removing a bunch of old ACL entries, at a glance this change seems okay:

image

But once it's actually merged, it ends up like:

---
- resources:
  - method: ALL
    path: /foo/bar/.*
    path: /
  allow_all: true

Here, you'll have a duplicate path key, and you will end up restricting a previously valid endpoint.

nerdrew commented 7 years ago

Hmmm. Rather than do this in production code, should this get added as a lint step we can invoke from CI?

zanker commented 7 years ago

@nerdrew @tarcieri do you have opinions on this? I don't generally think of an API init call should be responsible for the linting. It's really more of an app level check at test time, since this means you can merge code, and then have it fail after it's deployed.

zanker commented 7 years ago

haha

cthornton commented 7 years ago

I think another consideration is whether we should allow a yaml file with duplicate keys to block a deploy or not (i.e. tests fail, but can still be deployed). If we don't want it to block, then a linter sounds like a good choice.

Also fwiw, we do some lint-like checks today in the initialize method (i.e. make sure required keys are present). But, I'm also happy to make this into a linter during test time.

claassistantio commented 4 years ago

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.