Skip to content
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

Session handling #865

Merged
merged 36 commits into from
Aug 10, 2018
Merged

Session handling #865

merged 36 commits into from
Aug 10, 2018

Conversation

bjoernricks
Copy link
Contributor

@bjoernricks bjoernricks commented Aug 9, 2018

Renew user session only if:

  • the page (in react router terms location) has changed
  • a POST HTTP request has been send to gsad

Missing renews:

  • a user opens a dialog (should the session be renewed in this case? Yes it should)
    • Task dialogs
  • the pagination is used at the list pages
  • filter is changed at the list pages
  • Dashboard changes
  • Entity is changed on details pages

Missing features:

  • OBSOLETE session timeout isn't updated in the redux store after POST requests Web code should always renew the session via api explicitly

@codecov
Copy link

codecov bot commented Aug 9, 2018

Codecov Report

Merging #865 into master will increase coverage by 0.01%.
The diff coverage is 8.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #865      +/-   ##
=========================================
+ Coverage    7.57%   7.58%   +0.01%     
=========================================
  Files         822     824       +2     
  Lines       26113   26236     +123     
  Branches     5834    5864      +30     
=========================================
+ Hits         1978    1991      +13     
- Misses      21756   21859     +103     
- Partials     2379    2386       +7
Impacted Files Coverage Δ
gsa/src/gmp/utils/event.js 100% <ø> (ø) ⬆️
gsa/src/web/components/link/userlink.js 0% <0%> (ø)
gsa/src/web/components/dashboard/dashboard.js 0% <0%> (ø) ⬆️
gsa/src/web/components/bar/titlebar.js 0% <0%> (ø) ⬆️
gsa/src/web/entity/container.js 0% <0%> (ø) ⬆️
gsa/src/gmp/gmp.js 1.51% <0%> (ø) ⬆️
gsa/src/web/entities/withEntitiesContainer.js 0% <0%> (ø) ⬆️
gsa/src/web/pages/login/loginpage.js 0% <0%> (ø) ⬆️
gsa/src/web/components/observer/sessionobserver.js 0% <0%> (ø)
gsa/src/web/components/dashboard/controls.js 0% <0%> (ø) ⬆️
... and 12 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a0a3475...fc72925. Read the comment docs.

@swaterkamp
Copy link
Member

Opening a dialog is a user action indicating that she is active. This should imho renew the session just like using the pagination and a filter change should. I don't know right now, though, how much of an effort implementing this would be and if it's ultimately worth it...

@bjoernricks
Copy link
Contributor Author

@swaterkamp thanks for you opinion. It's not much more effort. I'll add another action creator function that renews the session and updates the session timestamp in the store. This action creator function has to be called in all open dialog methods afterwards.

swaterkamp
swaterkamp previously approved these changes Aug 10, 2018
Copy link
Member

@swaterkamp swaterkamp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The JS parts look good to me.

Add user_get_session_timeout function to return the session timeout as a
unix timestamp.
Don't renew the user session if user_find is called. This is currently
the case for all http requests with required user authentication.
When calling user_find avoid comparing against NULL string.
Add a http post gmp cmd to renew the user sesssion.
The promise returned by gmp.user.renewSession() returns a moment date
when the user session ends.
They aren't used in GSA so we should removed the params from the
response. In future the params parameter should be removed too.
Using moment from our own module allows to use the imported
translations.
Return the session timeout in every response containing an envelope.
Every post request should be triggerd by user interaction. Therefore the
user session should be extended on such requests.
Add reducer, selector and action for putting the session timeout into
the redux store.
We don't need to dispatch actions if the variables are not defined.
Initialize the sessionTimeout in the store after a successful login.
The SessionObserver component is responsibe to renew the use session if
the user changes the location and to initalize the session timeout in
the store for reloads.
The function calls the gmp api to renew the users session in the backend
and updates the session timeout in the store on success.
Clarify the unit of the wait parameter.
Comparing the location object for identity isn't enough. The session
should only be renewed if the current page has changed. This is the case
if the pathname or the query parameters have changed.
There are still some interactions missing which should renew the session
on specific list pages like opening the create and edit dialogs (or the
task wizards).
This also removes the withUserName HOC from Titlebar.
This avoids clearing the store before the components are unmounted
because gmp.logout will call all logout listeners which will clear the
store in app.js.
Setting the autorefresh interval to zero should deactivate the timers.
Don't use componentWillMount anymore. This api is deprecated.
Don't handle renewing the session on successfull changing data. This
will be done at the containers.
Ensure gmp.isLoggedIn return false before the location is changed.
Renew session if the entity or corresponding data (like references tags)
have changed.
@bjoernricks bjoernricks merged commit fc76b1b into greenbone:master Aug 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants