From 298924fef5fef8f1bc2e6134fa69c38437d5ceb5 Mon Sep 17 00:00:00 2001 From: bmccandless Date: Mon, 17 Aug 2020 13:41:03 -0700 Subject: [PATCH] Separate userinfo from the config endpoint (#1728) * Separate userinfo from the config endpoint previously information about if the user was logged in and their username was part of the config endpoint. However, the config endpoint was previously static, and has a cache control. Rather than not caching the config, a new endpoint called "userinfo" is created to handle that information. The config endpoint still has the non-changing part of the authentication: config: authentication: requires_client_login: True/False login: logout: The userinfo endpoint returns this information: userinfo: is_authenticated: True/False username: if authentication is not enabled then the config does not have an authentication key, and userinfo returns None. Also in the PR are a few minor code improvements and bug fixes Co-authored-by: Colin Megill --- client/src/actions/index.js | 12 ++ .../src/components/autosave/filenameDialog.js | 10 +- client/src/components/categorical/index.js | 33 ++++- client/src/components/menubar/authButtons.js | 8 +- client/src/components/menubar/index.js | 4 +- client/src/reducers/index.js | 2 + client/src/reducers/userinfo.js | 27 ++++ server/app/app.py | 8 ++ server/auth/auth_oauth.py | 31 +++-- server/common/app_config.py | 26 +++- server/common/rest.py | 5 + server/test/unit/auth/test_auth.py | 124 +++++++++--------- 12 files changed, 199 insertions(+), 91 deletions(-) create mode 100644 client/src/reducers/userinfo.js diff --git a/client/src/actions/index.js b/client/src/actions/index.js index a829b45b..4c39f86d 100644 --- a/client/src/actions/index.js +++ b/client/src/actions/index.js @@ -41,6 +41,17 @@ async function configFetch(dispatch) { }); } +async function userInfoFetch(dispatch) { + return fetchJson("userinfo").then((response) => { + const userinfo = { ...response.userinfo }; + dispatch({ + type: "userinfo load complete", + userinfo, + }); + return userinfo; + }); +} + function prefetchEmbeddings(annoMatrix) { /* prefetch requests for all embeddings @@ -62,6 +73,7 @@ const doInitialDataLoad = () => configFetch(dispatch), schemaFetch(dispatch), userColorsFetchAndLoad(dispatch), + userInfoFetch(dispatch), ]); const baseDataUrl = `${globals.API.prefix}${globals.API.version}`; diff --git a/client/src/components/autosave/filenameDialog.js b/client/src/components/autosave/filenameDialog.js index 9b67dc17..ebec1fd1 100644 --- a/client/src/components/autosave/filenameDialog.js +++ b/client/src/components/autosave/filenameDialog.js @@ -14,6 +14,7 @@ import { idhash: state.config?.parameters?.["annotations-user-data-idhash"] ?? null, annotations: state.annotations, auth: state.config?.authentication, + userinfo: state.userinfo, writableCategoriesEnabled: state.config?.parameters?.annotations ?? false, })) class FilenameDialog extends React.Component { @@ -91,13 +92,18 @@ class FilenameDialog extends React.Component { }; render() { - const { writableCategoriesEnabled, annotations, idhash, auth } = this.props; + const { + writableCategoriesEnabled, + annotations, + idhash, + userinfo, + } = this.props; const { filenameText } = this.state; return writableCategoriesEnabled && !annotations.dataCollectionNameIsReadOnly && !annotations.dataCollectionName && - auth.is_authenticated ? ( + userinfo.is_authenticated ? ( - - + + ) : null} ); diff --git a/client/src/components/menubar/authButtons.js b/client/src/components/menubar/authButtons.js index af7a2a93..9323d1e3 100644 --- a/client/src/components/menubar/authButtons.js +++ b/client/src/components/menubar/authButtons.js @@ -4,7 +4,7 @@ import * as globals from "../../globals"; import styles from "./menubar.css"; const Auth = React.memo((props) => { - const { auth } = props; + const { auth, userinfo } = props; if (!auth || (auth && !auth.requires_client_login)) return null; @@ -19,10 +19,10 @@ const Auth = React.memo((props) => { type="button" data-testid="auth-button" disabled={false} - icon={!auth.is_authenticated ? "log-in" : "log-out"} - href={!auth.is_authenticated ? auth.login : auth.logout} + icon={!userinfo["is_authenticated"] ? "log-in" : "log-out"} + href={!userinfo.is_authenticated ? auth.login : auth.logout} > - {!auth.is_authenticated ? "Log In" : "Log Out"} + {!userinfo.is_authenticated ? "Log In" : "Log Out"} diff --git a/client/src/components/menubar/index.js b/client/src/components/menubar/index.js index 1dc8a36a..c06ece70 100644 --- a/client/src/components/menubar/index.js +++ b/client/src/components/menubar/index.js @@ -42,6 +42,7 @@ import { getEmbSubsetView } from "../../util/stateManager/viewStackHelpers"; celllist2: state.differential.celllist2, libraryVersions: state.config?.["library_versions"], auth: state.config?.authentication, + userinfo: state.userinfo, undoDisabled: state["@@undoable/past"].length === 0, redoDisabled: state["@@undoable/future"].length === 0, aboutLink: state.config?.links?.["about-dataset"], @@ -221,6 +222,7 @@ class MenuBar extends React.PureComponent { subsetResetPossible, enableReembedding, auth, + userinfo, } = this.props; const { pendingClipPercentiles } = this.state; @@ -246,7 +248,7 @@ class MenuBar extends React.PureComponent { zIndex: 3, }} > - + { + switch (action.type) { + case "initial data load start": + return { + ...state, + loading: true, + error: null, + }; + case "userinfo load complete": + return { + ...state, + loading: false, + error: null, + ...action.userinfo, + }; + case "initial data load error": + return { + ...state, + error: action.error, + }; + default: + return state; + } +}; + +export default UserInfo; diff --git a/server/app/app.py b/server/app/app.py index d01b1964..06612a1c 100644 --- a/server/app/app.py +++ b/server/app/app.py @@ -240,6 +240,13 @@ class ConfigAPI(DatasetResource): return common_rest.config_get(current_app.app_config, data_adaptor) +class UserInfoAPI(DatasetResource): + @cache_control_always(no_store=True) + @rest_get_data_adaptor + def get(self, data_adaptor): + return common_rest.userinfo_get(current_app.app_config, data_adaptor) + + class AnnotationsObsAPI(DatasetResource): @cache_control(public=True, max_age=ONE_WEEK) @rest_get_data_adaptor @@ -308,6 +315,7 @@ def get_api_resources(bp_api, url_dataroot=None): # Initialization routes add_resource(SchemaAPI, "/schema") add_resource(ConfigAPI, "/config") + add_resource(UserInfoAPI, "/userinfo") # Data routes add_resource(AnnotationsObsAPI, "/annotations/obs") add_resource(AnnotationsVarAPI, "/annotations/var") diff --git a/server/auth/auth_oauth.py b/server/auth/auth_oauth.py index 8ed52036..7f35d43f 100644 --- a/server/auth/auth_oauth.py +++ b/server/auth/auth_oauth.py @@ -1,4 +1,4 @@ -from flask import session, request, redirect, current_app, after_this_request, has_request_context, g +from flask import session, request, redirect, current_app, has_request_context, g from server.auth.auth import AuthTypeClientBase, AuthTypeFactory from server.common.errors import AuthenticationError, ConfigurationError from urllib.parse import urlencode @@ -15,6 +15,7 @@ except ModuleNotFoundError: try: from jose import jwt + from jose.exceptions import ExpiredSignatureError, JWTError, JWTClaimsError except ModuleNotFoundError: missingimport.append("jose") @@ -137,18 +138,15 @@ class AuthTypeOAuth(AuthTypeClientBase): return response def logout(self): + params = {'returnTo' : self.callback_base_url, 'client_id' : self.client_id} + response = redirect(self.client.api_base_url + '/v2/logout?' + urlencode(params)) + if self.session_cookie: if self.CXG_ID_TOKEN in session: del session[self.CXG_ID_TOKEN] else: - @after_this_request - def remove_cookie(response): - response.set_cookie(self.cookie_params["key"], "", expires=0) - self.update_response(response) - return response + response.set_cookie(self.cookie_params["key"], "", expires=0) - params = {'returnTo' : self.callback_base_url, 'client_id' : self.client_id} - response = redirect(self.client.api_base_url + '/v2/logout?' + urlencode(params)) self.update_response(response) return response @@ -178,7 +176,7 @@ class AuthTypeOAuth(AuthTypeClientBase): def get_login_url(self, data_adaptor): """Return the url for the login route""" if current_app.app_config.is_multi_dataset(): - return f"/login?dataset={data_adaptor.uri_path}" + return f"/login?dataset={data_adaptor.uri_path}/" else: return "/login" @@ -205,7 +203,11 @@ class AuthTypeOAuth(AuthTypeClientBase): if token is None: return None - unverified_header = jwt.get_unverified_header(token) + try: + unverified_header = jwt.get_unverified_header(token) + except JWTError: + return None + rsa_key = {} for key in self.jwks['keys']: if key['kid'] == unverified_header['kid']: @@ -227,11 +229,12 @@ class AuthTypeOAuth(AuthTypeClientBase): ) return payload - except jwt.JWTError as e: + except JWTError as e: raise AuthenticationError(f"invalid signature: {str(e)}") - except jwt.ExpiredSignatureError as e: - raise AuthenticationError(f"token expired: {str(e)}") - except jwt.JWTClaimsError as e: + except ExpiredSignatureError: + # TODO, handle expired sessions by refreshing the token + return None + except JWTClaimsError as e: raise AuthenticationError(f"invalid claims {str(e)}") raise AuthenticationError("Unable to find the appropriate key") diff --git a/server/common/app_config.py b/server/common/app_config.py index b3715ecc..e262bc49 100644 --- a/server/common/app_config.py +++ b/server/common/app_config.py @@ -275,10 +275,7 @@ class AppConfig(object): if dataset_config.app__authentication_enable and auth.is_valid_authentication_type(): config["authentication"] = { - "is_authenticated": auth.is_user_authenticated(), "requires_client_login": auth.requires_client_login(), - "username": auth.get_user_name(), - "user_id": auth.get_user_id() } if auth.requires_client_login(): config["authentication"].update({ @@ -288,6 +285,29 @@ class AppConfig(object): return c + def get_client_userinfo(self, data_adaptor): + """ + Return the userinfo as required by the /userinfo REST route + """ + + server_config = self.server_config + dataset_config = data_adaptor.dataset_config + auth = server_config.auth + + # make sure the configuration has been checked. + self.check_config() + + if dataset_config.app__authentication_enable and auth.is_valid_authentication_type(): + userinfo = {} + userinfo["userinfo"] = { + "is_authenticated": auth.is_user_authenticated(), + "username": auth.get_user_name(), + "user_id": auth.get_user_id() + } + return userinfo + else: + return None + class BaseConfig(object): """This class handles the mechanics of updating and checking attributes. diff --git a/server/common/rest.py b/server/common/rest.py index b2e30305..be099f94 100644 --- a/server/common/rest.py +++ b/server/common/rest.py @@ -121,6 +121,11 @@ def config_get(app_config, data_adaptor): return make_response(jsonify(config), HTTPStatus.OK) +def userinfo_get(app_config, data_adaptor): + config = app_config.get_client_userinfo(data_adaptor) + return make_response(jsonify(config), HTTPStatus.OK) + + def annotations_obs_get(request, data_adaptor): fields = request.args.getlist("annotation-name", None) num_columns_requested = len(data_adaptor.get_obs_keys()) if len(fields) == 0 else len(fields) diff --git a/server/test/unit/auth/test_auth.py b/server/test/unit/auth/test_auth.py index 5d6eb748..b07e38d3 100644 --- a/server/test/unit/auth/test_auth.py +++ b/server/test/unit/auth/test_auth.py @@ -21,9 +21,10 @@ class AuthTest(unittest.TestCase): with test_server(app_config=c) as server: session = requests.Session() - r = session.get(f"{server}/d/pbmc3k.cxg/api/v0.2/config") - data_config = r.json() - assert "authentication" not in data_config["config"] + config = session.get(f"{server}/d/pbmc3k.cxg/api/v0.2/config").json() + userinfo = session.get(f"{server}/d/pbmc3k.cxg/api/v0.2/userinfo").json() + self.assertNotIn("authentication", config["config"]) + self.assertIsNone(userinfo) def test_auth_session(self): c = AppConfig() @@ -35,11 +36,12 @@ class AuthTest(unittest.TestCase): with test_server(app_config=c) as server: session = requests.Session() - r = session.get(f"{server}/d/pbmc3k.cxg/api/v0.2/config") - data_config = r.json() - assert data_config["config"]["authentication"]["is_authenticated"] - assert not data_config["config"]["authentication"]["requires_client_login"] - assert data_config["config"]["authentication"]["username"] == "anonymous" + config = session.get(f"{server}/d/pbmc3k.cxg/api/v0.2/config").json() + userinfo = session.get(f"{server}/d/pbmc3k.cxg/api/v0.2/userinfo").json() + + self.assertFalse(config["config"]["authentication"]["requires_client_login"]) + self.assertTrue(userinfo["userinfo"]["is_authenticated"]) + self.assertEqual(userinfo["userinfo"]["username"], "anonymous") def test_auth_test(self): c = AppConfig() @@ -61,45 +63,46 @@ class AuthTest(unittest.TestCase): session = requests.Session() # auth datasets - r = session.get(f"{server}/auth/pbmc3k.cxg/api/v0.2/config") - data_config = r.json() - assert not data_config["config"]["authentication"]["is_authenticated"] - assert data_config["config"]["authentication"]["requires_client_login"] - assert data_config["config"]["authentication"]["username"] is None - assert data_config["config"]["parameters"]["annotations"] + config = session.get(f"{server}/auth/pbmc3k.cxg/api/v0.2/config").json() + userinfo = session.get(f"{server}/auth/pbmc3k.cxg/api/v0.2/userinfo").json() - login_uri = data_config["config"]["authentication"]["login"] - logout_uri = data_config["config"]["authentication"]["logout"] + self.assertFalse(userinfo["userinfo"]["is_authenticated"]) + self.assertIsNone(userinfo["userinfo"]["username"]) + self.assertTrue(config["config"]["authentication"]["requires_client_login"]) + self.assertTrue(config["config"]["parameters"]["annotations"]) - assert login_uri == "/login?dataset=auth/pbmc3k.cxg" - assert logout_uri == "/logout?dataset=auth/pbmc3k.cxg" + login_uri = config["config"]["authentication"]["login"] + logout_uri = config["config"]["authentication"]["logout"] + + self.assertEqual(login_uri, "/login?dataset=auth/pbmc3k.cxg") + self.assertEqual(logout_uri, "/logout?dataset=auth/pbmc3k.cxg") r = session.get(f"{server}/{login_uri}") # check that the login redirect worked - assert r.history[0].status_code == 302 - assert r.url == f"{server}/auth/pbmc3k.cxg/" + self.assertEqual(r.history[0].status_code, 302) + self.assertEqual(r.url, f"{server}/auth/pbmc3k.cxg/") - r = session.get(f"{server}/auth/pbmc3k.cxg/api/v0.2/config") - data_config = r.json() - assert data_config["config"]["authentication"]["is_authenticated"] - assert data_config["config"]["authentication"]["username"] == "test_account" - assert data_config["config"]["parameters"]["annotations"] + config = session.get(f"{server}/auth/pbmc3k.cxg/api/v0.2/config").json() + userinfo = session.get(f"{server}/auth/pbmc3k.cxg/api/v0.2/userinfo").json() + self.assertTrue(userinfo["userinfo"]["is_authenticated"]) + self.assertEqual(userinfo["userinfo"]["username"], "test_account") + self.assertTrue(config["config"]["parameters"]["annotations"]) r = session.get(f"{server}/{logout_uri}") # check that the logout redirect worked - assert r.history[0].status_code == 302 - assert r.url == f"{server}/auth/pbmc3k.cxg/" - r = session.get(f"{server}/auth/pbmc3k.cxg/api/v0.2/config") - data_config = r.json() - assert not data_config["config"]["authentication"]["is_authenticated"] - assert data_config["config"]["authentication"]["username"] is None - assert data_config["config"]["parameters"]["annotations"] + self.assertEqual(r.history[0].status_code, 302) + self.assertEqual(r.url, f"{server}/auth/pbmc3k.cxg/") + config = session.get(f"{server}/auth/pbmc3k.cxg/api/v0.2/config").json() + userinfo = session.get(f"{server}/auth/pbmc3k.cxg/api/v0.2/userinfo").json() + self.assertFalse(userinfo["userinfo"]["is_authenticated"]) + self.assertIsNone(userinfo["userinfo"]["username"]) + self.assertTrue(config["config"]["parameters"]["annotations"]) # no-auth datasets - r = session.get(f"{server}/no-auth/pbmc3k.cxg/api/v0.2/config") - data_config = r.json() - assert "authentication" not in data_config["config"] - assert not data_config["config"]["parameters"]["annotations"] + config = session.get(f"{server}/no-auth/pbmc3k.cxg/api/v0.2/config").json() + userinfo = session.get(f"{server}/no-auth/pbmc3k.cxg/api/v0.2/userinfo").json() + self.assertIsNone(userinfo) + self.assertFalse(config["config"]["parameters"]["annotations"]) def test_auth_test_single(self): c = AppConfig() @@ -111,37 +114,36 @@ class AuthTest(unittest.TestCase): with test_server(app_config=c) as server: session = requests.Session() + config = session.get(f"{server}/api/v0.2/config").json() + userinfo = session.get(f"{server}/api/v0.2/userinfo").json() + self.assertFalse(userinfo["userinfo"]["is_authenticated"]) + self.assertIsNone(userinfo["userinfo"]["username"]) + self.assertTrue(config["config"]["authentication"]["requires_client_login"]) + self.assertTrue(config["config"]["parameters"]["annotations"]) - r = session.get(f"{server}/api/v0.2/config") - data_config = r.json() - assert not data_config["config"]["authentication"]["is_authenticated"] - assert data_config["config"]["authentication"]["requires_client_login"] - assert data_config["config"]["authentication"]["username"] is None - assert data_config["config"]["parameters"]["annotations"] + login_uri = config["config"]["authentication"]["login"] + logout_uri = config["config"]["authentication"]["logout"] - login_uri = data_config["config"]["authentication"]["login"] - logout_uri = data_config["config"]["authentication"]["logout"] - - assert login_uri == "/login" - assert logout_uri == "/logout" + self.assertEqual(login_uri, "/login") + self.assertEqual(logout_uri, "/logout") r = session.get(f"{server}/{login_uri}") # check that the login redirect worked - assert r.history[0].status_code == 302 - assert r.url == f"{server}/" + self.assertEqual(r.history[0].status_code, 302) + self.assertEqual(r.url, f"{server}/") - r = session.get(f"{server}/api/v0.2/config") - data_config = r.json() - assert data_config["config"]["authentication"]["is_authenticated"] - assert data_config["config"]["authentication"]["username"] == "test_account" - assert data_config["config"]["parameters"]["annotations"] + config = session.get(f"{server}/api/v0.2/config").json() + userinfo = session.get(f"{server}/api/v0.2/userinfo").json() + self.assertTrue(userinfo["userinfo"]["is_authenticated"]) + self.assertEqual(userinfo["userinfo"]["username"], "test_account") + self.assertTrue(config["config"]["parameters"]["annotations"]) r = session.get(f"{server}/{logout_uri}") # check that the logout redirect worked - assert r.history[0].status_code == 302 - assert r.url == f"{server}/" - r = session.get(f"{server}/api/v0.2/config") - data_config = r.json() - assert not data_config["config"]["authentication"]["is_authenticated"] - assert data_config["config"]["authentication"]["username"] is None - assert data_config["config"]["parameters"]["annotations"] + self.assertEqual(r.history[0].status_code, 302) + self.assertEqual(r.url, f"{server}/") + config = session.get(f"{server}/api/v0.2/config").json() + userinfo = session.get(f"{server}/api/v0.2/userinfo").json() + self.assertFalse(userinfo["userinfo"]["is_authenticated"]) + self.assertIsNone(userinfo["userinfo"]["username"]) + self.assertTrue(config["config"]["parameters"]["annotations"])