Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More extensive check between allowed rules vs. protected rules #147

Merged
merged 2 commits into from
Oct 15, 2019

Conversation

alexAubin
Copy link
Member

This is a follow-up of this discussion to try to fix this thing where if you have these settings :

    "protected_urls": [
        "yolo.test/foo", 
        "yolo.test/foo/admin"
    ], 
    "users": {
        "alice": {
            "yolo.test/foo": "Foo Test", 
            "yolo.test/foo/admin": "Foo Test (Admin)"
        }, 
        "bob": {
            "yolo.test/foo": "Foo Test"
        }
    }

then currently bob has access to /foo/admin despite the fact that the intention would be that only alice should ...

This draft allow to perform a more extensive check, and by this I mean checking what's the rule that has the longest match with respect to the requested uri...

This seem to be kinda working but gotta run moar test to validate the thousand of different use case to see that this does not cause any regression

@alexAubin alexAubin changed the base branch from stretch-unstable to logging-reloaded October 3, 2019 21:16
@alexAubin alexAubin added this to the 3.7.x milestone Oct 9, 2019
@alexAubin alexAubin changed the title WIP: More extensive check between allowed rules vs. protected rules More extensive check between allowed rules vs. protected rules Oct 9, 2019
@alexAubin
Copy link
Member Author

So that's kinda working, still would be nice if anybody has feedback if there's a better way to implement this ...

Copy link
Contributor

@Josue-T Josue-T left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me it look like good. But I didn't a really deep review...

I still just not really like the idea to merge the full url and the "url without the domain".

Maybe there are a better way to do this (maybe as discussed here ) but we still can improve this later. I think it's just some internal change. So it won't impact the apps.

@alexAubin alexAubin changed the base branch from logging-reloaded to stretch-unstable October 15, 2019 21:40
@alexAubin alexAubin merged commit 4643b75 into stretch-unstable Oct 15, 2019
@alexAubin alexAubin deleted the allowed-vs-protected branch October 15, 2019 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants