Improve error handling during login (#5413)

* Improve error handling during login * Prettify frontend
hotosm · Nov 29, 2022 · b3c5cb1 · b3c5cb1
1 parent 428453e
commit b3c5cb1
Show file tree

Hide file tree

Showing 14 changed files with 77 additions and 47 deletions.
diff --git a/backend/__init__.py b/backend/__init__.py
@@ -38,7 +38,9 @@ def format_url(endpoint):
 
 
 osm = OAuth2Session(
-    client_id=EnvironmentConfig.OAUTH_CLIENT_ID, scope=EnvironmentConfig.OAUTH_SCOPE
+    client_id=EnvironmentConfig.OAUTH_CLIENT_ID,
+    scope=EnvironmentConfig.OAUTH_SCOPE,
+    redirect_uri=EnvironmentConfig.OAUTH_REDIRECT_URI,
 )
 
 # Import all models so that they are registered with SQLAlchemy

diff --git a/backend/api/system/authentication.py b/backend/api/system/authentication.py
@@ -1,5 +1,6 @@
-from flask import current_app, redirect, request
+from flask import current_app, request
 from flask_restful import Resource
+from oauthlib.oauth2.rfc6749.errors import InvalidGrantError
 
 from backend import osm
 from backend.config import EnvironmentConfig
@@ -20,16 +21,16 @@ def get(self):
           - application/json
         parameters:
             - in: query
-              name: callback_url
+              name: redirect_uri
               description: Route to redirect user once authenticated
               type: string
               default: /take/me/here
         responses:
           200:
-            description: oauth token params
+            description: oauth2 params
         """
         redirect_uri = request.args.get(
-            "redirect_uri", current_app.config["APP_BASE_URL"]
+            "redirect_uri", EnvironmentConfig.OAUTH_REDIRECT_URI
         )
         authorize_url = f"{EnvironmentConfig.OSM_SERVER_URL}/oauth2/authorize"
         state = AuthenticationService.generate_random_state()
@@ -50,6 +51,23 @@ def get(self):
           - system
         produces:
           - application/json
+        parameters:
+            - in: query
+              name: redirect_uri
+              description: Route to redirect user once authenticated
+              type: string
+              default: /take/me/here
+              required: false
+            - in: query
+              name: code
+              description: Code obtained after user authorization
+              type: string
+              required: true
+            - in: query
+              name: email_address
+              description: Email address to used for email notifications from TM.
+              type: string
+              required: false
         responses:
           302:
             description: Redirects to login page, or login failed page
@@ -62,26 +80,40 @@ def get(self):
         token_url = f"{EnvironmentConfig.OSM_SERVER_URL}/oauth2/token"
         authorization_code = request.args.get("code", None)
         if authorization_code is None:
-            return {"Error": "Missing code parameter"}, 500
+            return {"Subcode": "InvalidData", "Error": "Missing code parameter"}, 500
 
         email = request.args.get("email_address", None)
-
-        osm_resp = osm.fetch_token(
-            token_url=token_url,
-            client_secret=EnvironmentConfig.OAUTH_CLIENT_SECRET,
-            code=authorization_code,
+        redirect_uri = request.args.get(
+            "redirect_uri", EnvironmentConfig.OAUTH_REDIRECT_URI
         )
-
+        osm.redirect_uri = redirect_uri
+        try:
+            osm_resp = osm.fetch_token(
+                token_url=token_url,
+                client_secret=EnvironmentConfig.OAUTH_CLIENT_SECRET,
+                code=authorization_code,
+            )
+        except InvalidGrantError:
+            return {
+                "Error": "The provided authorization grant is invalid, expired or revoked",
+                "SubCode": "InvalidGrantError",
+            }, 400
         if osm_resp is None:
-            current_app.logger.critical("No response from OSM")
-            return redirect(AuthenticationService.get_authentication_failed_url())
+            current_app.logger.critical("Couldn't obtain token from OSM.")
+            return {
+                "Subcode": "TokenFetchError",
+                "Error": "Couldn't fetch token from OSM.",
+            }, 502
 
         user_info_url = f"{EnvironmentConfig.OAUTH_API_URL}/user/details.json"
         osm_response = osm.get(user_info_url)  # Get details for the authenticating user
 
         if osm_response.status_code != 200:
             current_app.logger.critical("Error response from OSM")
-            return redirect(AuthenticationService.get_authentication_failed_url())
+            return {
+                "Subcode": "OSMServiceError",
+                "Error": "Couldn't fetch user details from OSM.",
+            }, 502
 
         try:
             user_params = AuthenticationService.login_user(osm_response.json(), email)

diff --git a/backend/config.py b/backend/config.py
@@ -123,6 +123,7 @@ class EnvironmentConfig:
     OAUTH_CLIENT_ID = os.getenv("TM_CLIENT_ID", None)
     OAUTH_CLIENT_SECRET = os.getenv("TM_CLIENT_SECRET", None)
     OAUTH_SCOPE = os.getenv("TM_SCOPE", None)
+    OAUTH_REDIRECT_URI = os.getenv("TM_REDIRECT_URI", None)
 
     # Some more definitions (not overridable)
     SQLALCHEMY_ENGINE_OPTIONS = {

diff --git a/backend/services/users/user_service.py b/backend/services/users/user_service.py
@@ -650,7 +650,7 @@ def get_recommended_projects(user_name: str, preferred_locale: str):
         len_projs = len(projs)
         if len_projs < limit:
             remaining_projs = (
-                query.filter(Project.mapper_level == user.mapping_level)
+                query.filter(Project.difficulty == user.mapping_level)
                 .limit(limit - len_projs)
                 .all()
             )

diff --git a/frontend/src/components/dropdown.js b/frontend/src/components/dropdown.js
@@ -175,9 +175,7 @@ export class _Dropdown extends React.PureComponent {
           onClick={this.toggleDropdown}
           className={`blue-dark ${this.props.className || ''}`}
         >
-          <p className="lh-title dib ma0 f6">
-            {this.getActiveOrDisplay()}
-          </p>
+          <p className="lh-title dib ma0 f6">{this.getActiveOrDisplay()}</p>
           <ChevronDownIcon style={{ width: '11px', height: '11px' }} className="pl3 v-mid pr1" />
         </CustomButton>
         {this.state.display && (

diff --git a/frontend/src/components/licenses/index.js b/frontend/src/components/licenses/index.js
@@ -54,13 +54,14 @@ export const LicensesManagement = ({ licenses, userDetails, isLicensesFetched })
         delay={10}
         ready={isLicensesFetched}
       >
-      <div className="w-20-l w-25-m">
-        <TextField
-          value={query}
-          placeholderMsg={messages.searchLicenses}
-          onChange={onSearchInputChange}
-          onCloseIconClick={() => setQuery('')}
-        /></div>
+        <div className="w-20-l w-25-m">
+          <TextField
+            value={query}
+            placeholderMsg={messages.searchLicenses}
+            onChange={onSearchInputChange}
+            onCloseIconClick={() => setQuery('')}
+          />
+        </div>
         {filteredLicenses?.length ? (
           filteredLicenses.map((i, n) => <LicenseCard key={n} license={i} />)
         ) : (

diff --git a/frontend/src/components/projectDetail/header.js b/frontend/src/components/projectDetail/header.js
@@ -21,7 +21,7 @@ export function HeaderLine({ author, projectId, priority, showEditLink, organisa
     <div className="cf">
       <div className="w-70-ns w-100 dib fl pv2">
         <span className="blue-dark">{projectIdLink}</span>
-        {organisation ? <span className='blue-dark'> | {organisation}</span> : null}
+        {organisation ? <span className="blue-dark"> | {organisation}</span> : null}
       </div>
       <div className="w-30-ns w-100 dib fl tr">
         {showEditLink && (

diff --git a/frontend/src/components/projectDetail/index.js b/frontend/src/components/projectDetail/index.js
@@ -37,12 +37,12 @@ const ProjectDetailMap = (props) => {
       },
     ],
   };
-  
+
   const centroidGeoJSON = props.project.areaOfInterest && {
     type: 'FeatureCollection',
     features: [centroid(props.project.areaOfInterest)],
   };
-  
+
   return (
     <div className="relative">
       {

diff --git a/frontend/src/components/projectEdit/messages.js b/frontend/src/components/projectEdit/messages.js
@@ -452,7 +452,8 @@ export default defineMessages({
   },
   difficultyDescription: {
     id: 'projects.formInputs.difficulty.description',
-    defaultMessage: 'Setting the difficulty will help mappers to find suitable projects to work on.',
+    defaultMessage:
+      'Setting the difficulty will help mappers to find suitable projects to work on.',
   },
   perTaskInstructions: {
     id: 'projects.formInputs.per_task_instructions',

diff --git a/frontend/src/components/projects/orderBy.js b/frontend/src/components/projects/orderBy.js
@@ -45,7 +45,7 @@ export function OrderBySelector(props) {
       type: 'DESC',
     },
   ];
-  
+
   const onSortSelect = (arr) => {
     if (arr.length === 1) {
       props.setQuery(
@@ -61,7 +61,7 @@ export function OrderBySelector(props) {
       throw new Error('filter select array is bigger.');
     }
   };
-  
+
   return (
     <Dropdown
       onChange={onSortSelect}

diff --git a/frontend/src/components/teamsAndOrgs/campaigns.js b/frontend/src/components/teamsAndOrgs/campaigns.js
@@ -37,13 +37,13 @@ export function CampaignsManagement({ campaigns, userDetails, isCampaignsFetched
         delay={10}
         ready={isCampaignsFetched}
       >
-      <div className="w-20-l w-25-m">
-        <TextField
-          value={query}
-          placeholderMsg={messages.searchCampaigns}
-          onChange={onSearchInputChange}
-          onCloseIconClick={() => setQuery('')}
-        />
+        <div className="w-20-l w-25-m">
+          <TextField
+            value={query}
+            placeholderMsg={messages.searchCampaigns}
+            onChange={onSearchInputChange}
+            onCloseIconClick={() => setQuery('')}
+          />
         </div>
         {filteredCampaigns?.length ? (
           filteredCampaigns.map((campaign, n) => <CampaignCard campaign={campaign} key={n} />)

diff --git a/frontend/src/components/teamsAndOrgs/tests/campaigns.test.js b/frontend/src/components/teamsAndOrgs/tests/campaigns.test.js
@@ -81,9 +81,7 @@ describe('CampaignsManagement component', () => {
         value: '2',
       },
     });
-    expect(screen.getByRole('heading', { name: 'Campaign 2' })).toHaveTextContent(
-      'Campaign 2',
-    );
+    expect(screen.getByRole('heading', { name: 'Campaign 2' })).toHaveTextContent('Campaign 2');
     fireEvent.change(textField, {
       target: {
         value: 'not 2',

diff --git a/frontend/src/utils/login.js b/frontend/src/utils/login.js
@@ -29,10 +29,7 @@ export const createLoginWindow = (redirectTo) => {
     // Perform token exchange.
 
     window.authComplete = (authCode, state) => {
-      const tokens = new URLSearchParams({
-        redirect_uri: OSM_REDIRECT_URI,
-      }).toString();
-      let callback_url = `system/authentication/callback/?${tokens}&code=${authCode}`;
+      let callback_url = `system/authentication/callback/?redirect_uri=${OSM_REDIRECT_URI}&code=${authCode}`;
       const emailAddress = safeStorage.getItem('email_address');
       if (emailAddress !== null) {
         callback_url += `&email_address=${emailAddress}`;

diff --git a/frontend/src/views/taskAction.js b/frontend/src/views/taskAction.js
@@ -43,7 +43,7 @@ export function TaskAction({ project, action }: Object) {
     if (userDetails.id && token && action && project) {
       getTasks();
     }
-  // eslint-disable-next-line react-hooks/exhaustive-deps
+    // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [action, userDetails.id, token, project, locale]);
 
   if (token) {