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

user.Manager: fix ACL write, read order #917

Merged
merged 7 commits into from
Nov 19, 2023
Merged

user.Manager: fix ACL write, read order #917

merged 7 commits into from
Nov 19, 2023

Conversation

sandman7920
Copy link
Contributor

This should fix read-only access to topic * being applied before read-write access to topic _PREFIX_* Before this if we have:

ntfy access user "mytopic*" rw
ntfy access user "*" ro

read-only access rule was applied first and user couldn't write to mytopic*

See #914

This should fix "read-only access to topic *" being applied before "read-write access to topic _PREFIX_*"
Before this if we have:

ntfy access user "mytopic*" rw
ntfy access user "*" ro

read-only access rule was applied first and user couldn't write to
mytopic*
@sandman7920
Copy link
Contributor Author

This PR still does not solve the following case

ntfy access user "mytopic*" rw
ntfy access user "mytopic_read_only*" ro
ntfy access user "*" ro

The user will still be able to write into mytopic_read_onlyXXX

Maybe query should be something like that

selectTopicPermsQuery = `
  SELECT read, write
  FROM user_access a
  JOIN user u ON u.id = a.user_id
  WHERE (u.user = ? OR u.user = ?) AND ? LIKE a.topic ESCAPE '\'
  ORDER BY u.user DESC, LENGTH(a.topic) DESC, a.write DESC
`

For each user, we should test in order THE_LONGEST_RULE->WRITE_PERMISSION

For each user, we should test in order `THE_LONGEST_RULE`->`WRITE_PERMISSION`
@binwiederhier
Copy link
Owner

Can you add a bunch of unit tests to check these cases? I don't want to just yolo merge this.

@sandman7920
Copy link
Contributor Author

A new test was added, and some old tests had to be rearranged to follow the new rules chain Longest->Shortest.

P.S.
Sometimes TestManager_AddUser_Timing fails

=== RUN   TestManager_AddUser_Timing
    manager_test.go:142:
                Error Trace:    user/manager_test.go:142
                Error:          "49" is not greater than or equal to "50"
                Test:           TestManager_AddUser_Timing

@sandman7920
Copy link
Contributor Author

sandman7920 commented Oct 24, 2023

I don't know how fast the SQLIte.LENGTH function is, maybe it's reasonable to create a topic_length field in the users.db access table.

@binwiederhier
Copy link
Owner

I would like to apologize for not addressing this earlier. I am looking at your solution now, and I'm trying to understand the root cause. Thank you for the tests and all the work. It is much appreciated!!

@binwiederhier binwiederhier merged commit f64dbcb into binwiederhier:main Nov 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants