Add retry on network error capabilities to finish signing / authentication process#72
Add retry on network error capabilities to finish signing / authentication process#72
Conversation
Allows to query the finish_session endpoint multiple times, for example to recover from the network failure
iharthi
changed the title
iharthi
force-pushed
the
retry-on-network-error
branch
12 times, most recently
from
90e7ca4 to
0f8806b
Compare
iharthi
force-pushed
the
retry-on-network-error
branch
2 times, most recently
from
6a94d3a to
1539133
Compare
| # --enable=similarities". If you want to run only the classes checker, but have | ||
| # no Warning level messages displayed, use "--disable=all --enable=classes | ||
| # --disable=W". | ||
| disable=apply-builtin, |
There was a problem hiding this comment.
Those all are now deprecated options, and pylint warns they are gone and do nothing in the config file
| @@ -1,4 +1,3 @@ | |||
| # pragma: no cover | |||
There was a problem hiding this comment.
Can apply to code blocks, but not the whole file. Whole file can be excluded in setup.cfg
| # we can do. | ||
| # | ||
| # See condition in login() | ||
| if request.session.get(SESSION_KEY) is None: |
There was a problem hiding this comment.
There are multiple ways to achieve what we want to achieve here (without refactoring the whole django-esteid to not use django sessions for authentication phase):
- Write custom login function from scratch by copy-pasting django login function. This is not too future-proof.
- Monkey-patch
cycle_keyon session, to prevent it from being called. This is not a good practice. - Add a context variable that would allow to temporary disable session key cycling, make a mixin that prevents cycle_key from actually doing anything when called if context variable is set, advice using custom session in the project with this mixin added. This is proper, we can implement it
- Current way, which takes advantage of conditional logic in django
loginfunction (which isn't well documented so hard to say why it is there and if it will be gone in future) to pre-set some things on session that will prevent key being cycled.
Dear reviewer, let me know what do you think. Shall we invest into proper solution with subclassing and context variables, or current way is ok?
There was a problem hiding this comment.
This works, but I dont really like it. We need to invest time in coming up with a better solution as relying on the internal implementation in django (which can change at any point).
I am unable to determine at the moment which way forward is best, but either a custom session class or moving away from django sessions completely (e.g. a jwt thats independently managed) might be the way to go.
Lets not merge this at its current state and work on a more future proof and Nice solution.
esteid/signing/views.py
Outdated
|
|
||
| if self._signer_instance.session_data.status != self.Status.PENDING: | ||
| # Return cached data, if available | ||
| print(self._signer_instance.session_data) |
There was a problem hiding this comment.
| print(self._signer_instance.session_data) |
| """ | ||
| signer_class = self.select_signer_class() | ||
| signer = signer_class.load_session(request.session) | ||
| self._signer_instance = signer_class.load_session(request.session) |
There was a problem hiding this comment.
hmm, wouldn't this break with async workers? e.g. wouldn't self be shared between multiple requests?
(i am a bit rusty on django so just checking)
There was a problem hiding this comment.
Valid point. As we agreed not to merge it yet, I won't jump into doing any changes; once we understand the proper way forward keeping a stateful signer may no longer be required at all.
2 participants
We have encountered the following issue with the signing and authentication flows: when user has the webapp open on same device where they have smart ID app installed, once smart ID session is initiated the browser goes into background and smart ID app opens. Depending on how fast the user returns to the browser, they may get a network error when polling the signing / authentication status endpoint.
In this case, BE has processed the request, but FE never gets the response. Adding a retry (see thorgate/esteid-helper#15) allows the FE to get the response form BE, but changes to BE were necessary to allow querying results of the completed session multiple times.