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

Separated login and login_info routes #1140

Merged
merged 20 commits into from
Dec 14, 2023
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 4 additions & 19 deletions mxcubeweb/core/components/user/usermanager.py
Original file line number Diff line number Diff line change
Expand Up @@ -181,8 +181,6 @@ def login(self, login_id: str, password: str):
msg = "User %s signed in" % user.username
logging.getLogger("MX3.HWR").info(msg)

return login_res["status"]

# Abstract method to be implemented by concrete implementation
def _signout(self):
pass
Expand Down Expand Up @@ -212,6 +210,7 @@ def signout(self):

self.app.server.user_datastore.deactivate_user(user)
flask_security.logout_user()

self.emit_observers_changed()

def is_authenticated(self):
Expand All @@ -227,22 +226,6 @@ def force_signout_user(self, username):
self.app.server.emit("forceSignout", room=socketio_sid, namespace="/hwr")

def login_info(self):
res = {
"synchrotronName": HWR.beamline.session.synchrotron_name,
"beamlineName": HWR.beamline.session.beamline_name,
"loggedIn": False,
"loginType": HWR.beamline.lims.loginType.title(),
"proposalList": [],
"user": {
"username": "",
"email": "",
"isstaff": False,
"nickname": "",
"inControl": False,
"ip": "",
},
}

if not current_user.is_anonymous:
login_info = convert_to_dict(json.loads(current_user.limsdata))

Expand Down Expand Up @@ -275,8 +258,10 @@ def login_info(self):
)

res["selectedProposalID"] = HWR.beamline.session.proposal_id
else:
raise Exception("Not logged in")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe raise a more specific exception type? See mxcubeweb/routes/login.py line 85 where is is caught.

Suggested change
raise Exception("Not logged in")
raise NotAuthenticated("Not logged in")


return current_user, res
return res

def update_user(self, user):
self.app.server.user_datastore.put(user)
Expand Down
36 changes: 11 additions & 25 deletions mxcubeweb/routes/login.py
Original file line number Diff line number Diff line change
@@ -1,23 +1,10 @@
import logging

from flask import (
Blueprint,
request,
jsonify,
make_response,
redirect,
session,
)
from flask import Blueprint, request, jsonify, make_response, session
from mxcubeweb.core.util import networkutils
from flask_login import current_user


def deny_access(msg):
resp = jsonify({"msg": msg})
resp.code = 409
return resp


def init_route(app, server, url_prefix):
bp = Blueprint("login", __name__, url_prefix=url_prefix)

Expand Down Expand Up @@ -45,15 +32,17 @@ def login():
password = params.get("password", "")

try:
res = jsonify(app.usermanager.login(login_id, password))
app.usermanager.login(login_id, password)
except Exception as ex:
msg = "[LOGIN] User %s could not login (%s)" % (
login_id,
str(ex),
)
logging.getLogger("MX3.HWR").exception("")
logging.getLogger("MX3.HWR").info(msg)
res = deny_access("Could not authenticate")
res = make_response(jsonify({"msg": "Could not authenticate"}), 200)
else:
res = make_response(jsonify({"msg": ""}), 200)
fabcor-maxiv marked this conversation as resolved.
Show resolved Hide resolved

session.permanent = True
Copy link
Member Author

Choose a reason for hiding this comment

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

I managed to get the tests working by adding back this line which I initially removed, as I thought that setting SESSION_PERMANENT=True would be enough and certainly as we are also using SESSION_PERMANENT_LIFETIME (and the later is taken into account).

However it seems that the configuration value is not taken into account and we need to set it programmatically, have a look at our mockup server config

Documentation even states that its true by default: https://flask-session.readthedocs.io/en/latest/config.html#configuration

Does someone have an idea of what that could be ?

Copy link
Contributor

@fabcor-maxiv fabcor-maxiv Nov 23, 2023

Choose a reason for hiding this comment

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

@marcus-oscarsson

I recall it not being straightforward to understand. It took me a while to wrap my head around sessions and their expiration. I had written some doc, but obviously this doc is not good enough since reading it back now does not clarify much...

Flask and Flask-session have different behavior, different defaults. Flask does not seem to know about the SESSION_PERMANENT config setting at all.

I made a quick search and I am starting to wonder if flask-session is used at all. I can not seem to find where it is used in the code. Am I missing something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed that explains it, so looking at the flask documentation SESSION_PARMANENT is not an option at all, however SESSION_PERMANENT_LIFETIME is and its used when one does session.permanent=True. So I was simply confused by the Flask-session documentation, we were using it in the past but now we are using Flask-Login

Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably remove line 29 from mxcubeweb/server.py, line 6 from test/HardwareObjectsMockup.xml/mxcube-web/server.yaml, and finally line 20 from mxcubeweb/core/models/configmodels.py.

Copy link
Member Author

Choose a reason for hiding this comment

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

:), Actually I thought if flask does not deal with the SESSION_PERMANENT setting we can :)

Copy link
Member Author

Choose a reason for hiding this comment

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

On a second thought, thats just unnecessary ... Ill remove the config option as we are actually relying on this feature, it does not make any sense to set it to false in our case.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder now if it makes sense (or is needed) to declare the session permanent when authentication failed.

It seems to work as it is so maybe it is fine to leave it like this, but curiosity...


Expand All @@ -66,8 +55,7 @@ def signout():
Signout from MXCuBE Web and reset the session
"""
app.usermanager.signout()

return redirect("/login", code=302)
return make_response(jsonify(""), 200)
fabcor-maxiv marked this conversation as resolved.
Show resolved Hide resolved

@bp.route("/login_info", methods=["GET"])
def login_info():
Expand All @@ -88,16 +76,14 @@ def login_info():

Status code set to:
200: On success
409: Error, could not log in
401: Error, could not log in
"""
user, res = app.usermanager.login_info()

# Redirect the user to login page if for some reason logged out
# i.e. server restart
if not user:
response = redirect("/login", code=302)
else:
try:
res = app.usermanager.login_info()
response = jsonify(res)
except Exception:
Copy link
Contributor

Choose a reason for hiding this comment

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

What about having a more specific Exception type? See mxcubeweb/core/components/user/usermanager.py line 262 where the exception is raised.

Suggested change
except Exception:
except NotAuthenticated:

response = make_response(jsonify(""), 401)

return response

Expand Down
25 changes: 9 additions & 16 deletions test/test_authn.py
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,6 @@ def client(make_client):
def test_authn_signin_good_credentials(client):
resp = client.post(URL_SIGNIN, json=CREDENTIALS_0)
assert resp.status_code == 200
marcus-oscarsson marked this conversation as resolved.
Show resolved Hide resolved
assert resp.json["code"] == "ok"
assert resp.json["msg"] == "Successful login"
fabcor-maxiv marked this conversation as resolved.
Show resolved Hide resolved


def test_authn_signin_wrong_credentials(client):
Expand All @@ -90,11 +88,9 @@ def test_authn_signin_wrong_credentials(client):

def test_authn_signout(client):
resp = client.post(URL_SIGNIN, json=CREDENTIALS_0)
assert resp.json["code"] == "ok"

resp = client.get(URL_SIGNOUT)
assert resp.status_code == 302
assert resp.headers["Location"] == "/login"
assert resp.status_code == 200
marcus-oscarsson marked this conversation as resolved.
Show resolved Hide resolved


def test_authn_info(client, login_type):
Expand All @@ -104,8 +100,7 @@ def test_authn_info(client, login_type):
and true after successful authentication.
"""
resp = client.get(URL_INFO)
assert resp.status_code == 200
assert resp.json["loggedIn"] == False
assert resp.status_code == 401

client.post(URL_SIGNIN, json=CREDENTIALS_0)

Expand All @@ -127,13 +122,14 @@ def test_authn_same_proposal(make_client):

client_0 = make_client()
resp = client_0.post(URL_SIGNIN, json=CREDENTIALS_0)
assert resp.json["code"] == "ok"

assert resp.status_code == 200
resp = client_0.get(URL_INFO)
assert resp.json["user"]["inControl"] == True

client_1 = make_client()
resp = client_1.post(URL_SIGNIN, json=CREDENTIALS_0)
assert resp.json["code"] == "ok"
assert resp.status_code == 200
resp = client_1.get(URL_INFO)
assert resp.json["user"]["inControl"] == False

Expand All @@ -146,12 +142,11 @@ def test_authn_different_proposals(make_client):
If a user signs in for a different proposal than an already signed in user,
this user should not be allowed to sign in.
"""

client_0 = make_client()
resp = client_0.post(URL_SIGNIN, json=CREDENTIALS_0)
assert resp.json["code"] == "ok"
assert resp.status_code == 200
resp = client_0.get(URL_INFO)
assert resp.json["user"]["inControl"] == True
assert resp.status_code == 200

client_1 = make_client()
resp = client_1.post(URL_SIGNIN, json=CREDENTIALS_1)
Expand Down Expand Up @@ -181,19 +176,17 @@ def test_authn_session_timeout(client):

# Check that the session still has not expired
resp = client.get(URL_INFO)
assert resp.status_code == 200
assert resp.json["loggedIn"] == True, "Session did not refresh"

# Let the session expire completely
time.sleep(SESSION_LIFETIME * 1.5)

# Check that the session has expired
resp = client.get(URL_INFO)
assert resp.json["loggedIn"] == False, "Session did not expire"
assert resp.status_code == 401, "Session did not expire"

# Check that it is possible to sign in again
client.post(URL_SIGNIN, json=CREDENTIALS_0)
resp = client.get(URL_INFO)
assert resp.json["loggedIn"] == True, "We can not login again"


# EOF
2 changes: 1 addition & 1 deletion ui/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
"react-redux": "7.2.6",
"react-router": "^6.2.1",
"react-router-bootstrap": "^0.26.1",
"react-router-dom": "^6.2.2",
"react-router-dom": "^6.19.0",
"react-slick": "^0.29.0",
"react-sticky": "^6.0.3",
"redux": "4.1.2",
Expand Down
35 changes: 25 additions & 10 deletions ui/pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 6 additions & 3 deletions ui/src/actions/general.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,12 @@ function notify(error) {
console.error('REQUEST FAILED', error); // eslint-disable-line no-console
}

export function getInitialState(userInControl) {
return (dispatch) => {
export function getInitialState() {
return (dispatch, getState) => {
const state = {};
const { login } = getState();

dispatch(applicationFetched(false));
axelboc marked this conversation as resolved.
Show resolved Hide resolved

const sampleVideoInfo = fetch('mxcube/api/v0.1/sampleview/camera', {
method: 'GET',
Expand Down Expand Up @@ -232,7 +235,7 @@ export function getInitialState(userInControl) {
});

/* don't unselect shapes when in observer mode */
if (userInControl) {
if (login.user.userInControl) {
prom = prom.then(() => {
dispatch(unselectShapes({ shapes: state.shapes }));
});
Expand Down
Loading