fix: isWhitelistedEmail bug

opengovsg · Sep 25, 2023 · 92c1e06 · 92c1e06
1 parent 1d4038f
commit 92c1e06
Show file tree

Hide file tree

Showing 7 changed files with 78 additions and 57 deletions.
diff --git a/.secrets.baseline b/.secrets.baseline
@@ -209,7 +209,7 @@
         "filename": "backend/src/core/services/auth.service.ts",
         "hashed_secret": "f114703480996b273d7e57cbd195b4ab1e70a21b",
         "is_verified": false,
-        "line_number": 63
+        "line_number": 65
       }
     ],
     "backend/src/email/services/tests/email-template.service.test.ts": [
@@ -365,5 +365,5 @@
       }
     ]
   },
-  "generated_at": "2023-09-17T14:25:47Z"
+  "generated_at": "2023-09-25T09:48:43Z"
 }
diff --git a/backend/src/core/middlewares/auth.middleware.ts b/backend/src/core/middlewares/auth.middleware.ts
@@ -262,7 +262,9 @@ export const InitAuthMiddleware = (authService: AuthService) => {
         logger.error({ message: sgidUserInfo.reason, ...logMeta })
         return res.status(401).json({ message: sgidUserInfo.reason })
       }
-      const userProfiles = authService.getSgidUserProfiles(sgidUserInfo.data)
+      const userProfiles = await authService.getSgidUserProfiles(
+        sgidUserInfo.data
+      )
       // Set user profiles in the session object so we can verify the profile selected by the user
       req.session.sgid = {
         ...req.session.sgid,

diff --git a/backend/src/core/services/auth.service.ts b/backend/src/core/services/auth.service.ts
@@ -31,7 +31,9 @@ export interface AuthService {
     | { authenticated: true; data: UserInfoReturn }
     | { authenticated: false; reason: string }
   >
-  getSgidUserProfiles(userInfo: UserInfoReturn): SgidPublicOfficerEmployment[]
+  getSgidUserProfiles(
+    userInfo: UserInfoReturn
+  ): Promise<SgidPublicOfficerEmployment[]>
 }
 
 export const InitAuthService = (redisService: RedisService): AuthService => {
@@ -408,13 +410,13 @@ export const InitAuthService = (redisService: RedisService): AuthService => {
    * Helper method to retrieve the user's valid profiles from their singpass info.
    * @param userInfo
    */
-  const getSgidUserProfiles = (
+  const getSgidUserProfiles = async (
     userInfo: UserInfoReturn
-  ): SgidPublicOfficerEmployment[] => {
+  ): Promise<SgidPublicOfficerEmployment[]> => {
     const profiles = JSON.parse(
       userInfo.data[SGID_PUBLIC_OFFICER_EMPLOYMENT_SCOPE]
     ) as SgidPublicOfficerEmployment[]
-    const validProfiles = validateSgidUserProfiles(profiles)
+    const validProfiles = await validateSgidUserProfiles(profiles)
     const cleanedProfiles = cleanSgidUserProfiles(validProfiles)
     return cleanedProfiles
   }
@@ -424,31 +426,32 @@ export const InitAuthService = (redisService: RedisService): AuthService => {
    * A profile is valid only if the user's work email exists and is whitelisted by Postman
    * @param userProfiles
    */
-  const validateSgidUserProfiles = (
+  const validateSgidUserProfiles = async (
     userProfiles: SgidPublicOfficerEmployment[]
-  ): SgidPublicOfficerEmployment[] => {
+  ): Promise<SgidPublicOfficerEmployment[]> => {
     const logMeta = { action: 'validateSgidUserProfiles' }
     // Only the value of workEmail is important for access to Postman.
-    const validProfiles = userProfiles.filter((profile) => {
+    const validProfiles = []
+    for (const profile of userProfiles) {
       // We want to log the absence of workEmail to measure the data completeness from SGID.
       if (profile.workEmail === SGID_FIELD_EMPTY) {
         logger.warn({
           message: 'Work email is missing from SGID data',
           ...logMeta,
           profile,
         })
-        return false
+        continue
       }
-      if (!isWhitelistedEmail(profile.workEmail)) {
+      if (!(await isWhitelistedEmail(profile.workEmail))) {
         logger.warn({
           message: 'Work email is not a whitelisted email',
           ...logMeta,
           profile,
         })
-        return false
+        continue
       }
-      return true
-    })
+      validProfiles.push(profile)
+    }
     return validProfiles
   }
 

diff --git a/frontend/src/components/common/confirm-modal/ConfirmModal.tsx b/frontend/src/components/common/confirm-modal/ConfirmModal.tsx
@@ -74,39 +74,41 @@ const ConfirmModal = ({
     }
   }
   return (
-    <div className={styles.confirm}>
-      {disableImage ? (
-        ''
-      ) : (
-        <div className={styles.modalImg}>
-          <img src={alternateImage ?? ConfirmImage} alt="Modal graphic"></img>
-        </div>
-      )}
-      <h2 className={styles.title}>{title}</h2>
-      {subtitle && <h4 className={styles.subtitle}>{subtitle}</h4>}
-      {subtitleElement}
-      <div className={styles.options}>
-        <PrimaryButton
-          className={
-            primary
-              ? styles.blueButton
-              : destructive
-              ? styles.redButton
-              : styles.greenButton
-          }
-          onClick={onConfirmedClicked}
-        >
-          {buttonText}
-          {buttonIcon && <i className={cx('bx', styles.icon, buttonIcon)}></i>}
-        </PrimaryButton>
-        {cancelText && (
-          <TextButton minButtonWidth onClick={onCancelClicked}>
-            {cancelText}
-          </TextButton>
+    <>
+      {buttonIcon && <i className={cx('bx', styles.icon, buttonIcon)}></i>}
+      <div className={styles.confirm}>
+        {disableImage ? (
+          ''
+        ) : (
+          <div className={styles.modalImg}>
+            <img src={alternateImage ?? ConfirmImage} alt="Modal graphic"></img>
+          </div>
         )}
+        <h2 className={styles.title}>{title}</h2>
+        {subtitle && <h4 className={styles.subtitle}>{subtitle}</h4>}
+        {subtitleElement}
+        <div className={styles.options}>
+          <PrimaryButton
+            className={
+              primary
+                ? styles.blueButton
+                : destructive
+                ? styles.redButton
+                : styles.greenButton
+            }
+            onClick={onConfirmedClicked}
+          >
+            {buttonText}
+          </PrimaryButton>
+          {cancelText && (
+            <TextButton minButtonWidth onClick={onCancelClicked}>
+              {cancelText}
+            </TextButton>
+          )}
+        </div>
+        <ErrorBlock>{errorMessage}</ErrorBlock>
       </div>
-      <ErrorBlock>{errorMessage}</ErrorBlock>
-    </div>
+    </>
   )
 }
 

diff --git a/frontend/src/components/common/modal/Modal.module.scss b/frontend/src/components/common/modal/Modal.module.scss
@@ -42,8 +42,8 @@ $title-height: $height-4;
 
 .close {
   position: absolute;
-  top: 2rem;
-  right: $spacing-1;
+  top: 1rem;
+  right: 1rem;
   &.modalTitleClose {
     color: $green-900;
     top: calc(#{$title-height} / 2 - 1.5rem); // close button has height of 3rem

diff --git a/frontend/src/components/login/login-callback/LoginCallback.module.scss b/frontend/src/components/login/login-callback/LoginCallback.module.scss
@@ -36,6 +36,7 @@
 
 // Every subsequent block only has border-bottom
 .profileBlock ~ .profileBlock {
+  border-top: 0px;
   border-bottom: 1px solid $primary-light;
 }
 
@@ -53,6 +54,7 @@
 .bottomBlock {
   width: 640px;
   padding-top: 32px;
+  font-size: 14px;
   display: flex;
   justify-content: center;
   color: #2c2cdc;

diff --git a/frontend/src/components/login/login-input/LoginInput.tsx b/frontend/src/components/login/login-input/LoginInput.tsx
@@ -45,7 +45,6 @@ const Login = () => {
   const [otpSent, setOtpSent] = useState(false)
   const [email, setEmail] = useState('')
   const [otp, setOtp] = useState('')
-  const [errorMsg, setErrorMsg] = useState('')
   const [canResend, setCanResend] = useState(false)
   const [isResending, setIsResending] = useState(false)
   const modalContext = useContext(ModalContext)
@@ -58,6 +57,20 @@ const Login = () => {
   const SINGPASS_ERROR_CODE = 'SingpassError'
   const NO_EMPLOYEE_PROFILE_ERROR_CODE = 'NoSingpassProfile'
 
+  const openErrorModal = (errorMessage: string) =>
+    modalContext.setModalContent(
+      <ConfirmModal
+        title={`Oops, something went wrong. Please try again later.`}
+        subtitleElement={
+          <h4 className={styles.subtitleElement}>{errorMessage}</h4>
+        }
+        buttonText="Okay"
+        alternateImage={ErrorImage}
+        primary={true}
+        onConfirm={() => modalContext.close()}
+      />
+    )
+
   useEffect(() => {
     const params = new URL(window.location.href).searchParams
     const errorCode = params.get('errorCode')
@@ -71,6 +84,8 @@ const Login = () => {
               <a
                 style={{ textDecoration: 'underline' }}
                 href={SGID_VALID_ORGANISATIONS_PAGE}
+                target="_blank"
+                rel="noreferrer"
               >
                 here
               </a>{' '}
@@ -118,13 +133,12 @@ const Login = () => {
       }, RESEND_WAIT_TIME)
     } catch (err) {
       setCanResend(true)
-      setErrorMsg((err as Error).message)
+      openErrorModal((err as Error).message)
       sendException((err as Error).message)
     }
   }
 
   async function login() {
-    setErrorMsg('')
     try {
       await loginWithOtp(email, otp)
       setAuthenticated(true)
@@ -135,26 +149,24 @@ const Login = () => {
       )
       setUserAnalytics(user)
     } catch (err) {
-      setErrorMsg((err as Error).message)
+      openErrorModal((err as Error).message)
       sendException((err as Error).message)
     }
   }
 
   async function sgidLogin() {
-    setErrorMsg('')
     try {
       const authUrl = await getSgidUrl()
       if (authUrl) {
         window.location.assign(authUrl)
       }
     } catch (err) {
-      setErrorMsg((err as Error).message)
+      openErrorModal((err as Error).message)
       sendException((err as Error).message)
     }
   }
 
   function resetButton() {
-    setErrorMsg('')
     setCanResend(false)
     setOtp('')
   }
@@ -199,7 +211,6 @@ const Login = () => {
           onClick={sendOtp}
           buttonLabel={<Trans>Get OTP</Trans>}
           loadingButtonLabel={<Trans>Sending OTP...</Trans>}
-          errorMessage={errorMsg}
         />
       ) : (
         <TextInputWithButton
@@ -211,7 +222,6 @@ const Login = () => {
           onClick={login}
           buttonLabel={<Trans>Sign In</Trans>}
           loadingButtonLabel={<Trans>Verifying OTP...</Trans>}
-          errorMessage={errorMsg}
         />
       )}
       {/* This feature is experimental and should only be rendered on the demo URL (/sgid-login) */}
@@ -228,6 +238,8 @@ const Login = () => {
             <a
               style={{ textDecoration: 'underline' }}
               href={SGID_VALID_ORGANISATIONS_PAGE}
+              target="_blank"
+              rel="noreferrer"
             >
               here
             </a>