feat: add Client.logout and context manager #381

oleobal · 2023-08-09T09:53:18Z

Summary

Add a Client.logout function, which deletes a token obtained by Client.login (if any).

Add a context manager, making the following snippet legal:

with Client(
    client_name="org-1",
    backend_type="remote",
    url="http://substra-backend.org-1.com:8000",
    username="org-1",
    password="p@sswr0d44",
) as client:
    pass

Requires Substra/substra-backend#698

Closes FL-1008

Notes

Please check if the PR fulfills these requirements

If necessary, the changelog has been updated
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)
The commit message follows the conventional commit specification
For any breaking changes, companion PRs have been opened on the following repositories:

Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>

linear · 2023-08-09T09:53:21Z

FL-1008 Add logout function to Substra SDK

Context

Security assessment V7 (the Audit page on Notion has links to the detailed report)

Risk score: 3/12 (low)

After logging in using username and password with the Substra Python library, an API token is issued. However, the library does not offer a logout function, which means that users cannot end their authenticated sessions actively. Authenticated API tokens thus always remain active on the server until the timeout expires.

The Substra Python library should have a logout function allowing users to end sessions actively. The function needs to invalidate the API token on the server side and securely delete related session data on the client.

Specification

Add a new endpoint to the backend that revokes the current ImplicitBearerToken

Add a Client.logout function to Substra SDK that calls that endpoint

It would be nice to also add a context manager because it would look professional:

with Client(username, password) as client:
   pass

Acceptance criteria

Substra SDK has a Client.logout or similar function that revokes the token created by Client.login

Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>

oleobal · 2023-08-09T15:30:41Z

I'm unsure what to document or test exactly. This change is mostly transparent for users, and I would be mocking a lot of stuff.

oleobal · 2023-08-09T15:32:01Z

substra/sdk/client.py

+    def __enter__(self):
+        return self
+
+    def __exit__(self, exc_type, exc_value, traceback):
+        self.logout()
+
+    def __del__(self):
+        self.logout()


Unsure whether context manager is still needed with __del__ defined, but people online say not to rely on it and using with is preferable. However that does mean updating our documentation.

What part of the documentation are you reffering to ?

I mean that in the documentation all examples of the form:

client = Client(...) do_stuff(client)

should be replaced with:

with Client(...) as client: do_stuff(client)

Some scripts (such as the examples) need the client for more than a thousand lines, so I would not replace the old version, maybe just indicate that the context manager is also a valid option ?

I added it in the sdk.Client reference, under the username argument.

Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>

ThibaultFy

Thanks for the PR :)

Concerning the fact that we define a logout function only for the remote backend, I am not 100% convince... There is a login method defined in locals backend too, I think for the hybrid mode. You should take a look at it to ensure what we should do for this mode I think. (cf sdk/backends/local/dal.py.

For the tests, you could check that the token is deleted the using the context manager with or when deleting the object (del client).
This test can be useful to mock minimally

substra/tests/sdk/test_client.py

Line 92 in eae5d52

def test_client_with_password(mocker):

substra/sdk/client.py

ThibaultFy · 2023-08-16T08:42:07Z

substra/sdk/client.py

+    def __enter__(self):
+        return self
+
+    def __exit__(self, exc_type, exc_value, traceback):
+        self.logout()
+
+    def __del__(self):
+        self.logout()


What part of the documentation are you reffering to ?

Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>

ThibaultFy

A few small comments and will be good to me ! do you think we need a run of CI on it before merging ? No strong opinion on it on my side.

CHANGELOG.md

substra/sdk/backends/remote/rest_client.py

substra/sdk/client.py

Co-authored-by: Thibault Fouqueray <thibault.fouqueray@owkin.com> Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>

oleobal · 2023-08-17T08:47:42Z

@ThibaultFy

do you think we need a run of CI on it before merging ? No strong opinion on it on my side.

I ran one yesterday but let's run a new one just to be sure ^^

ThibaultFy · 2023-08-17T08:48:59Z

@ThibaultFy

do you think we need a run of CI on it before merging ? No strong opinion on it on my side.

I ran one yesterday but let's run a new one just to be sure ^^

Ah ok ok :) you're not using the /e2e command ?

oleobal · 2023-08-17T08:56:02Z

you're not using the /e2e command ?

I am, just from the backend PR instead of this one

ThibaultFy

LGTM, thanks a lot :)

- Solve the issue of sometimes issuing tokens that are about to expire, by just issuing an new token every time and relying on the SDK to clean them up - Add a new `server.allowImplicitLogin` option, allowing node admins to disable the option altogether in order to improve security practices. - Extend `/active-api-tokens -X DELETE` to also be able to delete `ImplicitBearerToken`, adding an `id` field to `ImplicitBearerToken` for this purpose. This is to enable users to terminate their sessions, as per security recommendations Closes FL-1067, FL-1140 Companion to Substra/substra-documentation#336 Leveraged by Substra/substra#381 Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>

SdgJlbl · 2023-08-17T10:24:07Z

substra/sdk/backends/remote/rest_client.py

+        except requests.exceptions.Timeout as e:
+            raise exceptions.Timeout.from_request_exception(e)


Do we want to retry on Timeout? Or is it already handled at a higher level?
In all cases, I'm not sure if we want to raise uncaught exceptions to the user, or just have a warning logged 🤔

feat: add Client.logout and context manager

c518351

Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>

Add token deletion in Client destructor

e8c5ac7

Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>

oleobal force-pushed the feat/client-logout branch from e0009db to e8c5ac7 Compare August 9, 2023 15:22

oleobal marked this pull request as ready for review August 9, 2023 15:30

oleobal requested a review from a team as a code owner August 9, 2023 15:30

oleobal commented Aug 9, 2023

View reviewed changes

oleobal mentioned this pull request Aug 10, 2023

feat: improve ImplicitBearerToken Substra/substra-backend#698

Merged

2 tasks

correct typo in readme

01c3d37

Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>

ThibaultFy reviewed Aug 16, 2023

View reviewed changes

oleobal added 3 commits August 16, 2023 13:19

give a logout function to all backends

f8d7f08

Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>

update tests

5a681b7

Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>

Add admonition about context managers in sdk.Client reference

1370803

Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>

oleobal force-pushed the feat/client-logout branch from c59391b to 1370803 Compare August 16, 2023 15:22

oleobal requested a review from ThibaultFy August 17, 2023 08:14

ThibaultFy reviewed Aug 17, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

substra/sdk/backends/remote/rest_client.py Outdated Show resolved Hide resolved

substra/sdk/client.py Outdated Show resolved Hide resolved

improve style and documentation

c809973

Co-authored-by: Thibault Fouqueray <thibault.fouqueray@owkin.com> Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>

oleobal force-pushed the feat/client-logout branch from 1f782f6 to c809973 Compare August 17, 2023 08:45

ThibaultFy approved these changes Aug 17, 2023

View reviewed changes

oleobal merged commit e2916b9 into main Aug 17, 2023

oleobal deleted the feat/client-logout branch August 17, 2023 10:18

SdgJlbl reviewed Aug 17, 2023

View reviewed changes

oleobal mentioned this pull request Aug 17, 2023

fix: prevent Client.logout from raising exceptions #382

Merged

7 tasks

Milouu mentioned this pull request Sep 5, 2023

chore: release 0.47.0 #385

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add Client.logout and context manager #381

feat: add Client.logout and context manager #381

oleobal commented Aug 9, 2023 •

edited

Loading

linear bot commented Aug 9, 2023

Context

Specification

Acceptance criteria

oleobal commented Aug 9, 2023

oleobal Aug 9, 2023

ThibaultFy Aug 16, 2023

oleobal Aug 16, 2023 •

edited

Loading

ThibaultFy Aug 16, 2023 •

edited

Loading

oleobal Aug 16, 2023

ThibaultFy left a comment

ThibaultFy Aug 16, 2023

ThibaultFy left a comment

oleobal commented Aug 17, 2023

ThibaultFy commented Aug 17, 2023

oleobal commented Aug 17, 2023

ThibaultFy left a comment

SdgJlbl Aug 17, 2023

		except requests.exceptions.Timeout as e:
		raise exceptions.Timeout.from_request_exception(e)

feat: add Client.logout and context manager #381

feat: add Client.logout and context manager #381

Conversation

oleobal commented Aug 9, 2023 • edited Loading

Summary

Notes

Please check if the PR fulfills these requirements

linear bot commented Aug 9, 2023

Context

Specification

Acceptance criteria

oleobal commented Aug 9, 2023

oleobal Aug 9, 2023

Choose a reason for hiding this comment

ThibaultFy Aug 16, 2023

Choose a reason for hiding this comment

oleobal Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

ThibaultFy Aug 16, 2023 • edited Loading

Choose a reason for hiding this comment

oleobal Aug 16, 2023

Choose a reason for hiding this comment

ThibaultFy left a comment

Choose a reason for hiding this comment

ThibaultFy Aug 16, 2023

Choose a reason for hiding this comment

ThibaultFy left a comment

Choose a reason for hiding this comment

oleobal commented Aug 17, 2023

ThibaultFy commented Aug 17, 2023

oleobal commented Aug 17, 2023

ThibaultFy left a comment

Choose a reason for hiding this comment

SdgJlbl Aug 17, 2023

Choose a reason for hiding this comment

oleobal commented Aug 9, 2023 •

edited

Loading

oleobal Aug 16, 2023 •

edited

Loading

ThibaultFy Aug 16, 2023 •

edited

Loading