Log Authenticated User from UI-Server in Workflow Log #4522

datamel · 2021-11-17T10:46:01Z

Log the authenticated user executing the mutation command from the UI.

A test user pausing the workflow of another user, will result in a log message below in the workflow log:

The owner then resuming the workflow, will result in a log message..

These changes close #4294 and cylc/cylc-uiserver#224 by its sibling: cylc/cylc-uiserver#288

Requirements 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.py and
conda-environment.yml.
Appropriate tests are not included (unit and/or functional). - Only manually tested. This will require multi user testing setup to get this into the automated test battery.
No change log entry required (why? Logging change).
No documentation update required.

cylc/flow/network/client.py

oliver-sanders · 2021-11-18T14:47:21Z

cylc/flow/network/client.py

+        if auth_user:
+            set_authenticated_user(self.header, auth_user)


This is updating the client's header which is shared between multiple requests (so potentially for multiple authenticated users). We can delay this a couple of lines and set it on the message rather than the shared header.

oliver-sanders · 2021-11-18T14:48:02Z

cylc/flow/network/client.py

@@ -175,6 +184,8 @@ async def async_request(self, command, args=None, timeout=None):
        # there is no need to encrypt messages ourselves before sending.

        # send message
+        if auth_user:
+            set_authenticated_user(self.header, auth_user)
        msg = {'command': command, 'args': args}
        msg.update(self.header)


We can delay this a couple of lines

Here:

msg.update(self.header) msg['meta']['auth_user'] = auth_user

cylc/flow/network/client.py

oliver-sanders · 2021-11-18T16:05:01Z

cylc/flow/network/client.py

+        command,
+        args=None,
+        timeout=None,
+        auth_user=None


If it's quick, it might be a good idea to generalise this to handle arbitrary metadata in a dictionary.

We could do:

# add the shared header msg.update(self.header) # add the request metadata msg['meta'].update(request_meta)

This would make it easier to pass through additional information e.g. the app that issued the command or the GraphQL operation name (could be useful if the request contains multiple mutations).

hjoliver · 2021-12-07T00:46:02Z

@datamel - I'm quite keen to get this in! Are you planning to address @oliver-sanders 's comments above? (Pinging you just in case you missed them).

datamel · 2021-12-07T09:22:02Z

@datamel - I'm quite keen to get this in! Are you planning to address @oliver-sanders 's comments above? (Pinging you just in case you missed them).

Yes, on it today. Sorry for the delay. Had paused for the multi-mutation fix.

cylc/flow/scheduler.py

hjoliver · 2021-12-16T04:28:38Z

Tested both branches, looks good 👍

What gets logged for direct commands (not via the UIS) looks dodgy however:

INFO - Authenticated user ? has submitted request kill_tasks

vs

INFO - Authenticated user oliverh has submitted request force_trigger_tasks

Perhaps we could say owner has submitted request... for direct commands?

Log the authenticated user executing the command

cylc/flow/network/resolvers.py

MetRonnie · 2022-01-19T12:20:39Z

cylc/flow/network/resolvers.py

-            result = (True, 'Command queued')
-        return [{'id': w_id, 'response': result}]
-
-    async def nodes_mutator(self, *m_args):


Why is nodes_mutator being removed?

This was picked up when adding the unpacking of the meta and was discussed with @oliver-sanders, who looked at codecov for verification. It is seemingly dead code and was asked to remove it. Pinged @dwsutherland on the sibling to verify it is indeed unused code.

It was in use, until:
#3469
specifically:
https://github.com/cylc/cylc-flow/pull/3469/files#diff-97f4d62be41218e2d70a203edfca6d44d7c0046b6908cefa8e3af7c02cd31920

And the idea behind it was that mutations being multi-workflow could be given node ids from different workflows, and then these ids would be parsed and mutations sent to the appropriate workflow..

The fact that we moved away from this, means we are happy with not using this functionality... i.e. a node mutation will always be associated with only one workflow and/or the workflow parsing happens somewhere else.

datamel · 2022-01-19T12:52:44Z

cylc/flow/network/authorisation.py

-
-            # Hardcoded, for new - but much of this functionality can be
-            # removed more swingingly.
-            LOG.debug(


@oliver-sanders, just thinking about removing this line. It does remove the comms_method which may be useful for debugging in future? Want me to put any logging information about the comms method to replace this, or are you happy with the info loss from the removal?

We can pass the comms_method down with the metadata and log it with the command.

cylc/flow/network/resolvers.py

oliver-sanders · 2022-01-19T13:22:37Z

tests/functional/cylc-message/00-ssh.t

-grep_ok "\[client-command\] graphql ssh" \
-    "$RUN_DIR/${WORKFLOW_NAME}/log/workflow/log" 
-


This was a safety test to ensure that the comms method is logged.

Perhaps we could add a Cylc client command into the workflow e.g. cylc hold $CYLC_WORKFLOW_ID//1/elephant which should give us a [command] entry that we can check.

Yes, are you happy if I do this in the ssh async request PR as this test is broken anyway?

oliver-sanders · 2022-01-19T13:23:55Z

Looking good, will have a quick test soon.

Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>

oliver-sanders · 2022-01-25T15:15:51Z

Tested, works wonderfully 🎉

datamel self-assigned this Nov 17, 2021

datamel added this to the cylc-8.0rc1 milestone Nov 17, 2021

datamel mentioned this pull request Nov 17, 2021

Pass authenticated user to cylc-flow cylc/cylc-uiserver#288

Merged

7 tasks

oliver-sanders reviewed Nov 18, 2021

View reviewed changes

hjoliver mentioned this pull request Dec 7, 2021

2021 Cylc Meetings cylc/cylc-admin#139

Closed

datamel force-pushed the pass-user-to-cylc-flow branch from f888dc2 to d887e49 Compare December 15, 2021 10:07

hjoliver reviewed Dec 16, 2021

View reviewed changes

cylc/flow/scheduler.py Outdated Show resolved Hide resolved

datamel added 5 commits January 17, 2022 10:54

Auth user to cylc-flow

fffb911

Log the authenticated user executing the command

flake8

304de78

Test fix

70115dc

facepalm

6bf4444

Change ? for Owner for logging user requesting commands

91d86d6

datamel force-pushed the pass-user-to-cylc-flow branch from 4a806e0 to 91d86d6 Compare January 17, 2022 11:06

oliver-sanders reviewed Jan 17, 2022

View reviewed changes

cylc/flow/network/resolvers.py Show resolved Hide resolved

datamel added 4 commits January 17, 2022 12:50

back to wip

1568cd0

remove unused code and log auth user to command

a59bcf8

Remove redundant test

cb41941

Tweak log message

baab059

MetRonnie self-requested a review January 19, 2022 12:17

MetRonnie reviewed Jan 19, 2022

View reviewed changes

datamel commented Jan 19, 2022

View reviewed changes

oliver-sanders reviewed Jan 19, 2022

View reviewed changes

Update cylc/flow/network/resolvers.py

fbe1768

Co-authored-by: Oliver Sanders <oliver.sanders@metoffice.gov.uk>

oliver-sanders approved these changes Jan 25, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/master' into HEAD

75d6799

MetRonnie approved these changes Jan 25, 2022

View reviewed changes

MetRonnie merged commit f6f6c14 into cylc:master Jan 25, 2022

MetRonnie mentioned this pull request Aug 15, 2022

Cleaner log messages. #3647

Open

datamel deleted the pass-user-to-cylc-flow branch September 1, 2022 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log Authenticated User from UI-Server in Workflow Log #4522

Log Authenticated User from UI-Server in Workflow Log #4522

datamel commented Nov 17, 2021 •

edited by MetRonnie

Loading

oliver-sanders Nov 18, 2021

oliver-sanders Nov 18, 2021

oliver-sanders Nov 18, 2021

hjoliver commented Dec 7, 2021

datamel commented Dec 7, 2021 •

edited

Loading

hjoliver commented Dec 16, 2021

MetRonnie Jan 19, 2022

datamel Jan 19, 2022

dwsutherland Feb 1, 2022

datamel Jan 19, 2022

oliver-sanders Jan 20, 2022 •

edited

Loading

oliver-sanders Jan 19, 2022

datamel Jan 19, 2022

oliver-sanders commented Jan 19, 2022

oliver-sanders commented Jan 25, 2022

		grep_ok "\[client-command\] graphql ssh" \
		"$RUN_DIR/${WORKFLOW_NAME}/log/workflow/log"

Log Authenticated User from UI-Server in Workflow Log #4522

Log Authenticated User from UI-Server in Workflow Log #4522

Conversation

datamel commented Nov 17, 2021 • edited by MetRonnie Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hjoliver commented Dec 7, 2021

datamel commented Dec 7, 2021 • edited Loading

hjoliver commented Dec 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver-sanders Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver-sanders commented Jan 19, 2022

oliver-sanders commented Jan 25, 2022

datamel commented Nov 17, 2021 •

edited by MetRonnie

Loading

datamel commented Dec 7, 2021 •

edited

Loading

oliver-sanders Jan 20, 2022 •

edited

Loading