Separated login and login_info routes #1140

marcus-oscarsson · 2023-11-22T15:40:26Z

Separated login and login_info routes. Returning 401 - Unauthorized from login_info instead of redirect. Removed the call to requireAuth from the PrivateOutlet component as it runs during rendering.

requireAuth is now executed on page load (useEffect of App) and on change of route using the loader functionality of react-router. The later makes the transition from login to application much smoother, as no reload of the page is needed.

The proposal selection dialog was also separated from the Login component and displayed at login when there is no proposal already selected. The user can select a proposal from the menu in the top right corner.

meguiraun · 2023-11-23T08:55:55Z

thanks, just few questions (does not qualify as a proper review)

The user can select a proposal from the menu in the top right corner.

Is there any situation where the user can play around without selecting a proposal?

And, I am not so sure if we want to change proposals in the middle of the beamtime without a proper logout. (ping @JieNanMAXIV )

fabcor-maxiv

I guess we will want to properly test this at MAX IV before this gets merged. That is why I put a provisional negative feedback for now, even though I have not reviewed the code yet. And obviously I understand that it is still a draft.

Rework of the authentication code is definitely welcome, it was not straightforward before, thanks for working on this :)

marcus-oscarsson · 2023-11-23T10:30:29Z

@meguiraun: I don't think that its possible to proceed without selecting a proposal :). The "change" proposal dialog can of-course be made optional. We will need a feature like this for the automated beamlines where you can collect as several proposals during one session.

@fabcor-maxiv: Sure, understandable take your time and test it. This is the first step there will be a few more updates at a later moment. I'm actually struggling a bit with the session timeout test, while it works in the browser it fails in the test. Maybe you have an idea of what it could be ?

mxcubeweb/routes/login.py

ui/src/actions/login.js

ui/src/api/login.js

ui/src/components/App.js

ui/src/actions/general.js

axelboc · 2023-11-23T12:38:19Z

ui/src/reducers/index.js

@@ -40,10 +40,6 @@ const mxcubeReducer = combineReducers({
 });

 const rootReducer = (state, action) => {


This wrapper function is no longer needed I think, you can inline the line below directly.

ui/src/components/App.js

marcus-oscarsson · 2023-11-23T15:06:06Z

mxcubeweb/routes/login.py

-            res = deny_access("Could not authenticate")
+            res = make_response(jsonify({"msg": "Could not authenticate"}), 200)
+        else:
+            res = make_response(jsonify({"msg": ""}), 200)

        session.permanent = True


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 ?

@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?

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

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.

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

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.

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...

…rouces

Removed circular dependency on serverIO

marcus-oscarsson · 2023-11-28T13:18:30Z

Just a few last improvements done after some discussions we had between us here, after this I think we are more or less happy with this one. There will as I mentioned before be one or two more PR's to follow up on this.

Status codes of all unauthorized resources are now 401
login_info always have status code 200 but returns loggedIn=False|True
Moved getLoginInfo call to getInitialState and removed unnecessary navigate
Moved getInitialState to actions/login to remove a circular dependency cycle (And it it also makes sense to have getIntialState in reducers/login
Moved the use of serverIO.disconnect from action/login so that it only contains redux related code and as a consequence avoids a circular dependency

ui/src/actions/login.js

axelboc · 2023-12-11T15:54:59Z

I've refactored things a bit to make it clearer when/where getLoginInfo and getInitialState get called. Here's the gist of it:

getLoginInfo is called from useEffect in App and from the logIn action, which means it gets called:
1. once on initial load, to know if the user is logged in or not;
2. right after a successful login (until such times as the login endpoint returns the login info right away).
getInitialState is called only from componentDidMount in Main, which means that it gets called:
1. on initial load when the user is already logged in;
2. after a successful login.

In other words, getInitialState no longer needs to call getLoginInfo directly, since by the time Main renders, the user's login info have already been fetched (either from useEffect or from login). This means we can move getInitialState out of the login.js actions file without reintroducing a circular import. I'll do it in a separate commit later this week.

fabcor-maxiv · 2023-12-11T15:58:50Z

Would that make sense to add some of these technical details at docs/source/dev/login.rst?

fabcor-maxiv · 2023-12-11T16:00:10Z

For info, I have not been able to work on this these past weeks, but I am planning to test these changes to authentication this week (possibly tomorrow already).

marcus-oscarsson · 2023-12-12T09:07:35Z

@fabcor-maxiv what I suggest is that we merge this as it is now and we fix anything we/you find while testing. Unless you see a fundamental problem with this PR

fabcor-maxiv · 2023-12-12T15:36:42Z

I am investigating a case where I get an error TypeError: t.loginInfo.user is undefined, seemingly in the SET_LOGIN_INFO action. It appears right when loading MXCuBE the first time, before any authentication. It stays stuck on the "Loading, please wait" page. I will provide more details, once I manage to narrow this down further.

False alarm.

fabcor-maxiv · 2023-12-12T16:23:53Z

I figured out the cause of the behavior I initially reported in the previous message, and it was a "user issue" :D

Now it seems to be working fine, comparable to what is demonstrated by the animation in the first post. Nice! :)

I did some preliminary testing and found no obvious issue, so no reason to block. I have not reviewed the code yet though.

fabcor-maxiv

Some comments for the Python part

fabcor-maxiv · 2023-12-13T09:42:23Z

mxcubeweb/core/components/user/usermanager.py

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

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


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")

fabcor-maxiv · 2023-12-13T09:42:45Z

mxcubeweb/routes/login.py

            response = jsonify(res)
+        except Exception:


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:

fabcor-maxiv · 2023-12-13T09:45:21Z

mxcubeweb/routes/login.py

-            res = deny_access("Could not authenticate")
+            res = make_response(jsonify({"msg": "Could not authenticate"}), 200)
+        else:
+            res = make_response(jsonify({"msg": ""}), 200)

        session.permanent = True


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...

test/test_authn.py

mxcubeweb/routes/login.py

test/test_authn.py

fabcor-maxiv

Comment regarding the empty message seen from the front end side.

ui/src/actions/login.js

test/test_authn.py

Co-authored-by: fabcor <[email protected]>

marcus-oscarsson · 2023-12-14T09:26:34Z

Ok, thanks guys (@axelboc and @fabcor-maxiv). The work on some of the details will continue in followup-PRs, and fairly soon one that also introduces OpenID Connect authentication using KeyCloak.

fabcor-maxiv · 2023-12-18T10:18:00Z

@axelboc @marcus-oscarsson Seems like we lost the loading animation on the "Sign in" button in this change. I can open a separate ticket if you prefer.

P.S.: ticket opened: #1148

marcus-oscarsson force-pushed the mo-login-resource branch 4 times, most recently from cccdd30 to e2e2c8c Compare November 22, 2023 16:03

marcus-oscarsson force-pushed the mo-login-resource branch from e2e2c8c to af90b82 Compare November 23, 2023 08:56

fabcor-maxiv previously requested changes Nov 23, 2023

View reviewed changes

axelboc reviewed Nov 23, 2023

View reviewed changes

marcus-oscarsson added 5 commits November 23, 2023 15:26

Separated login and login_info routes

55c029d

Updated tests to use status codes

fc27f47

disconnecting socketio in useEffect cleanup

72f0bee

Added a resetLoginInfo action

81c82a8

Removed stray print

9dd45d4

marcus-oscarsson force-pushed the mo-login-resource branch from 643b3bb to 9dd45d4 Compare November 23, 2023 14:26

marcus-oscarsson added 4 commits November 23, 2023 15:35

Removed deny_access method

6a741a3

removed unnecessary unauthorized handler

531ff73

Added missing session.permanent = True

cecf8cc

Added missing file

b38bb14

marcus-oscarsson commented Nov 23, 2023

View reviewed changes

Removed action creator, startSession

34b1f83

marcus-oscarsson force-pushed the mo-login-resource branch from fb152df to 34b1f83 Compare November 23, 2023 15:30

marcus-oscarsson added 3 commits November 23, 2023 17:08

Removed unused code

1cf98d3

Improved HTTP status code handling, using 401 on all unauthorized res…

d27b122

…rouces

Moved getInitialState and related logic to login

bb5c56d

Removed circular dependency on serverIO

marcus-oscarsson force-pushed the mo-login-resource branch from 768c638 to bb5c56d Compare November 28, 2023 13:07

Fixed doc-string

fe40504

axelboc reviewed Nov 28, 2023

View reviewed changes

ui/src/actions/login.js Outdated Show resolved Hide resolved

ui/src/actions/login.js Outdated Show resolved Hide resolved

Removed unnecessary navigate and removed unused error handling code

17fa748

marcus-oscarsson marked this pull request as ready for review November 29, 2023 09:07

Review when to get getLoginInfo and getInitialState

7bd7a66

axelboc force-pushed the mo-login-resource branch from f320c70 to 7bd7a66 Compare December 11, 2023 15:46

fabcor-maxiv mentioned this pull request Dec 12, 2023

Created AbstractAuthenticator and made necessary updates to LdapLogin mxcube/mxcubecore#822

Merged

fabcor-maxiv reviewed Dec 13, 2023

View reviewed changes

ui/src/actions/login.js Show resolved Hide resolved

fabcor-maxiv reviewed Dec 13, 2023

View reviewed changes

test/test_authn.py Show resolved Hide resolved

marcus-oscarsson and others added 4 commits December 14, 2023 10:14

Update test/test_authn.py

dce890d

Co-authored-by: fabcor <[email protected]>

Update test/test_authn.py

30c7a03

Co-authored-by: fabcor <[email protected]>

Fixed test

9268189

Moved session.permanent to successful login

960470d

marcus-oscarsson merged commit fb728d7 into develop Dec 14, 2023
13 checks passed

marcus-oscarsson deleted the mo-login-resource branch December 14, 2023 09:27

fabcor-maxiv mentioned this pull request Dec 18, 2023

Behavior of login form while credentials are being verified #1148

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separated login and login_info routes #1140

Separated login and login_info routes #1140

marcus-oscarsson commented Nov 22, 2023

meguiraun commented Nov 23, 2023

fabcor-maxiv left a comment

marcus-oscarsson commented Nov 23, 2023 •

edited

Loading

axelboc Nov 23, 2023

marcus-oscarsson Nov 23, 2023

fabcor-maxiv Nov 23, 2023 •

edited

Loading

marcus-oscarsson Nov 23, 2023

fabcor-maxiv Nov 23, 2023

marcus-oscarsson Nov 23, 2023

marcus-oscarsson Nov 23, 2023

fabcor-maxiv Dec 13, 2023

marcus-oscarsson commented Nov 28, 2023

axelboc commented Dec 11, 2023

fabcor-maxiv commented Dec 11, 2023

fabcor-maxiv commented Dec 11, 2023

marcus-oscarsson commented Dec 12, 2023

fabcor-maxiv commented Dec 12, 2023 •

edited

Loading

fabcor-maxiv commented Dec 12, 2023

fabcor-maxiv left a comment

fabcor-maxiv Dec 13, 2023

fabcor-maxiv Dec 13, 2023

fabcor-maxiv Dec 13, 2023

fabcor-maxiv left a comment

marcus-oscarsson commented Dec 14, 2023

fabcor-maxiv commented Dec 18, 2023 •

edited

Loading

		@@ -40,10 +40,6 @@ const mxcubeReducer = combineReducers({
		});

		const rootReducer = (state, action) => {

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

Separated login and login_info routes #1140

Separated login and login_info routes #1140

Conversation

marcus-oscarsson commented Nov 22, 2023

meguiraun commented Nov 23, 2023

fabcor-maxiv left a comment

Choose a reason for hiding this comment

marcus-oscarsson commented Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabcor-maxiv Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcus-oscarsson commented Nov 28, 2023

axelboc commented Dec 11, 2023

fabcor-maxiv commented Dec 11, 2023

fabcor-maxiv commented Dec 11, 2023

marcus-oscarsson commented Dec 12, 2023

fabcor-maxiv commented Dec 12, 2023 • edited Loading

fabcor-maxiv commented Dec 12, 2023

fabcor-maxiv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabcor-maxiv left a comment

Choose a reason for hiding this comment

marcus-oscarsson commented Dec 14, 2023

fabcor-maxiv commented Dec 18, 2023 • edited Loading

marcus-oscarsson commented Nov 23, 2023 •

edited

Loading

fabcor-maxiv Nov 23, 2023 •

edited

Loading

fabcor-maxiv commented Dec 12, 2023 •

edited

Loading

fabcor-maxiv commented Dec 18, 2023 •

edited

Loading