Skip to content

fix authorization limit-permission case mismatch #693

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

Open
wants to merge 2 commits into
base: 1.6.x
Choose a base branch
from

Conversation

dwsutherland
Copy link
Member

@dwsutherland dwsutherland commented May 23, 2025

closes #692

Before this change a config like this:

c.CylcUIServer.site_authorization = {
    "*": {  # For all ui-server owners,
        "*": {  # Any authenticated user
            "limit": ["READ", "CONTROL"],
        },
    },
}

c.CylcUIServer.user_authorization = {
    'other-suther': ['READ', 'Poll', 'Play', 'Pause', 'Stop'],
}

Would internally be, with reference to:

def _get_permitted_operations(self, access_user: str):

def expand_and_process_access_groups(self, permission_set: set) -> set:

expanded as:

limits_owner_can_give: {'CONTROL', 'READ'}
user_conf_permitted_ops: {'Play', 'Stop', 'READ', 'Poll', 'Pause'}
expanded user_conf_permitted_ops: {'Play', 'read', 'Stop', 'Poll', 'Pause'}
expanded limits_owner_can_give: {'trigger', 'pause', 'scan', 'set_verbosity', 'play', 'ext_trigger', 'set', 'read', 'hold', 'release_hold_point', 'message', 'clean', 'stop', 'kill', 'resume', 'set_graph_window_extent', 'release', 'remove', 'poll', 'reload', 'set_hold_point'}

then via:

        # subtract permissions that the site does not permit to be granted
        allowed_operations = limits_owner_can_give.intersection(
            user_conf_permitted_ops
        )

gives read

[I 2025-05-23 14:27:42.958 CylcUIServer] User other-suther authorized permissions: ['read']
{'read'}

With this small fix, the permissions are correct:

[I 2025-05-23 16:09:28.239 CylcUIServer] User other-suther authorized permissions: ['pause', 'play', 'poll', 'read', 'stop']
{'poll', 'play', 'stop', 'pause', 'read'}

And UI menu correctly making available the corresponding options:
image

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Applied any dependency changes to both setup.cfg (and conda-environment.yml if present).
  • Tests are included (or explain why tests are not needed).
  • Changelog entry included if this is a change that can affect users
  • Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@dwsutherland dwsutherland added this to the 1.6.x milestone May 23, 2025
@dwsutherland dwsutherland self-assigned this May 23, 2025
@dwsutherland dwsutherland mentioned this pull request May 23, 2025
6 tasks
@dwsutherland dwsutherland modified the milestones: 1.6.x, 1.6.2 May 27, 2025
@oliver-sanders
Copy link
Member

Dammit, I've just realised my own stupidity.

I think I must have tried:

# all upper
"default": ["READ", "PLAY", "PAUSE", ...]

And

# all lower
"default": ["read", "play", "pause", ...]

Rather than:

# mixed
"default": ["READ", "play", "pause", ...]

READ, CONTROL and ALL are groups (in upper) that get expanded to permissions (in lower).

We should probably just validate the config to prevent this mistake being made?

@dwsutherland
Copy link
Member Author

dwsutherland commented May 28, 2025

We should probably just validate the config to prevent this mistake being made?

Well, it's documented as first-letter-capitalized (i.e. Play or Stop) but the grouped aliases get_list_of_mutations returns all lowercase (as you noted):

    @property
    def ALL_OPS(self) -> List[str]:
        """ALL OPS constant, returns list of all mutations."""
        return get_list_of_mutations()
    @property
    def CONTROL_OPS(self) -> List[str]:
        """CONTROL OPS constant, returns list of all control mutations."""
        return get_list_of_mutations(control=True)

At least with this PR it is essentially case insensitive (aside from the grouped i.e. CONTROL etc), which shouldn't be a problem?

Also the other good thing about forcing lowercase at this point in the code is you won't miss a negative, i.e ["CONTROL", "!Stop", "!reload"]

@oliver-sanders
Copy link
Member

oliver-sanders commented May 30, 2025

Thanks for looking into this. I've taken a look and I think this is just a documentation error, looks like it's been lowercase since auth weas introduced in 0.6.0.

I think we should probably:

  • Correct the documentation.
  • Reject unknown items.

Converting to lowercase is slightly dubious as the same thing can be configured multiple ways (e.g. Play and play), the difference between item and group is blurred (e.g. READ and read) and potential for misunderstanding increased.

Also, the docs give camel case, but the UIS uses snake case, so we can't get away without a docs change.

@dwsutherland
Copy link
Member Author

dwsutherland commented Jun 5, 2025

Thanks for looking into this. I've taken a look and I think this is just a documentation error, looks like it's been lowercase since auth weas introduced in 0.6.0.

So you want to make lowercase the documented standard and spit out a validation warning in the UIS logs? (i.e. if Play instead of play is used)

the difference between item and group is blurred (e.g. READ and read) and potential for misunderstanding increased.

This lowercasing happens after group resolving but before negation, so shouldn't be blurred .. READ is a special case where it has (at present) one resolve item read.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants