Bugfix | Web UI server-side storage #50
Conversation
…ed few endpoints and improved styling
There was a problem hiding this comment.
There's some changes needed so that these changes respect the class interfaces for M1Session instead of trying to bypass them. Bypassing them could cause misbehaviour in the class as your bypass is not maintaining the internal data integrity maintained by the class itself. It also add the risk that future changes to the internals of the M1Session class break your code when internal data structures are changed.
management-ui/server.py
Outdated
| @app.delete("/remove_all_sessions") | ||
| async def remove_all_sessions(): | ||
| session = await get_session(config) | ||
| session._M1Session__provisioning_sessions.clear() |
There was a problem hiding this comment.
You should not be accessing protected members of a class from outside the class, please stick to using M1Session public methods to manipulate the session.
Also clearing the list of known provisioning sessions like this is a bad idea (it won't update the persistent data store for one), if you want to remove provisioning sessions then use session.provisioningSessionDestroy() on each known provisioning session id (session.provisioningSessionIds()).
management-ui/server.py
Outdated
|
|
||
| session_data = session._M1Session__provisioning_sessions.get(provisioning_session_id) | ||
| if session_data is not None: | ||
| session_data['certificate_id'] = cert_id |
There was a problem hiding this comment.
The certificate Id will already be in the list of certificate Ids in the provisioning session after the call to session.createNewCertificate(), you shouldn't be setting strange dictionary members.
management-ui/server.py
Outdated
| if cert_id is None: | ||
| raise HTTPException(status_code=400, detail='Failed to create certificate') | ||
|
|
||
| session_data = session._M1Session__provisioning_sessions.get(provisioning_session_id) |
There was a problem hiding this comment.
Again, there are methods to access and manipulate provisioning sessions in M1Session. There is no need to bypass the class and access member variables directly.
management-ui/server.py
Outdated
| session_data = session._M1Session__provisioning_sessions.get(provisioning_session_id) | ||
| if session_data and 'certificate_id' in session_data: | ||
| return {"certificate_id": session_data['certificate_id']} |
There was a problem hiding this comment.
You should be using session.provisioningSessionCertificateIds(provisioning_session_id) to retrieve a list of certificates, not inventing a single certificate store.
There was a problem hiding this comment.
There's been an unclear issue, where I receive empty list of a certificate Ids when using session.provisioningSessionCertificateIds(provisioning_session_id), even though the Certificate is created.
Another possibility is to fetch this ID directly from the AF response for provisioning session:
"serverCertificateIds": [
"f9a4cf16-58dc-41ef-8bfc-d7aaf8f810a4"
]
There was a problem hiding this comment.
In other words, session.provisioningSessionCertificateIds might have memory issue at some point along the way, resulting always with the empty list.
Instead I call session.certificateIds()
There was a problem hiding this comment.
That's a bug that should be fixed then. If something is not working as expected then feel free to raise an issue.
Description
This PR enhances session data storage by moving it from the browser cache to server-side storage. It introduces several auxiliary endpoints and tweaks frontend functionalities to allign with the in-memory writing of the Python classes. Minor styling changes are done, as well.
The changes have been tested from two different devices to confirm consistency.
Related issue: #49