Skip to content

Commit

Permalink
Fix Android Logout (#1233)
Browse files Browse the repository at this point in the history
# Fix Android Logout

## JIRA Ticket

[BSS-484](https://jira.csiro.com/browse/BSS-484)

## Description

Logout didn't work on Android, well it almost worked but the UI still
thought we were logged in.

## Proposed Changes

Changed the use of useQuery in relation to user tokens, it was being too
aggressive in caching and handed back an old token after logout (only on
Android, suspect a timing issue).

## How to Test

Try logging in and logging out again on web and Android.

## Additional Information

Bug was because getAnyToken returned undefined when there was no token
which useQuery treats as an error. Replaced this with an object value
and we're sweet as.

## Checklist

- [x] I have confirmed all commits have been signed.
- [x] I have added JSDoc style comments to any new functions or classes.
- [x] Relevant documentation such as READMEs, guides, and class comments
are updated.
  • Loading branch information
stevecassidy authored Nov 13, 2024
2 parents 90a862e + afbb8be commit f06b228
Show file tree
Hide file tree
Showing 15 changed files with 28 additions and 129 deletions.
6 changes: 0 additions & 6 deletions api/src/auth_routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,6 @@ const AVAILABLE_AUTH_PROVIDER_DISPLAY_INFO: {[name: string]: any} = {
},
};

const HANDLER_OPTIONS: {[name: string]: any} = {
google: {
prompt: 'select_account',
},
};

passport.serializeUser((user: Express.User, done: DoneFunction) => {
done(null, user.user_id);
});
Expand Down
1 change: 0 additions & 1 deletion api/src/couchdb/notebooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -763,7 +763,6 @@ export const streamNotebookRecordsAsCSV = async (
while (!done) {
// record might be null if there was an invalid db entry
if (record) {

const hrid = getRecordHRID(record);
const row = [
hrid,
Expand Down
6 changes: 5 additions & 1 deletion api/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,11 @@
import PouchDB from 'pouchdb';
import PouchDBFind from 'pouchdb-find';

import {CONDUCTOR_INTERNAL_PORT, CONDUCTOR_PUBLIC_URL, COUCHDB_INTERNAL_URL} from './buildconfig';
import {
CONDUCTOR_INTERNAL_PORT,
CONDUCTOR_PUBLIC_URL,
COUCHDB_INTERNAL_URL,
} from './buildconfig';

import {app} from './routes';

Expand Down
2 changes: 1 addition & 1 deletion app/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ import {AuthReturn} from './gui/components/authentication/auth_return';
import CreateNewSurvey from './gui/components/workspace/CreateNewSurvey';
import NotFound404 from './gui/pages/404';
import {AppUrlListener} from './native_hooks';
import { NotificationProvider } from './context/popup';
import {NotificationProvider} from './context/popup';

// type AppProps = {};

Expand Down
50 changes: 0 additions & 50 deletions app/src/buildconfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,28 +56,6 @@ function commit_version(): string {
}
}

function prod_build(): boolean {
const productionBuild = import.meta.env.VITE_PRODUCTION_BUILD;
if (
productionBuild === '' ||
productionBuild === undefined ||
TRUTHY_STRINGS.includes(productionBuild.toLowerCase())
) {
return true;
} else if (FALSEY_STRINGS.includes(productionBuild.toLowerCase())) {
return false;
} else {
logError('VITE_PRODUCTION_BUILD badly defined, assuming false');
return false;
}
}
/*
* This isn't exported, instead to help reduce the number of environment
* variables to set to get a production build for real users. Can be used in the
* rest of the configuration.
*/
const PROD_BUILD = prod_build();

function include_pouchdb_debugging(): boolean {
const debug_pouch = import.meta.env.VITE_DEBUG_POUCHDB;
if (debug_pouch === '' || debug_pouch === undefined) {
Expand Down Expand Up @@ -215,32 +193,6 @@ function cluster_admin_group_name(): string {
return name;
}

function disable_signin_redirect(): boolean {
const disable_signin = import.meta.env.VITE_DISABLE_SIGNIN_REDIRECT;
if (disable_signin === '' || disable_signin === undefined) {
return false;
}
if (FALSEY_STRINGS.includes(disable_signin.toLowerCase())) {
return false;
} else if (TRUTHY_STRINGS.includes(disable_signin.toLowerCase())) {
return true;
} else {
logError('VITE_DISABLE_SIGNIN_REDIRECT badly defined, assuming false');
return false;
}
}

function get_login_token(): string | undefined {
const login_token = import.meta.env.VITE_LOGIN_TOKEN;
if (login_token === '' || login_token === undefined) {
return undefined;
}
if (PROD_BUILD) {
logError('Production builds should not set login token, except under test');
}
return login_token;
}

// If VITE_BUGSNAG_KEY is not defined then we don't use Bugsnag
function get_bugsnag_key(): string | false {
const bugsnag_key = import.meta.env.VITE_BUGSNAG_KEY;
Expand Down Expand Up @@ -394,8 +346,6 @@ export const CLUSTER_ADMIN_GROUP_NAME = cluster_admin_group_name();
export const SHOW_MINIFAUXTON = show_minifauxton();
export const SHOW_WIPE = show_wipe();
export const SHOW_NEW_NOTEBOOK = show_new_notebook();
export const DISABLE_SIGNIN_REDIRECT = disable_signin_redirect();
export const BUILT_LOGIN_TOKEN = get_login_token();
export const BUGSNAG_KEY = get_bugsnag_key();
export const NOTEBOOK_LIST_TYPE = get_notebook_list_type();
export const NOTEBOOK_NAME = get_notebook_name();
Expand Down
11 changes: 2 additions & 9 deletions app/src/constants/privateRouter.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import {Navigate} from 'react-router-dom';

import {DISABLE_SIGNIN_REDIRECT} from '../buildconfig';
import * as ROUTES from './routes';
import {useGetAnyToken} from '../utils/tokenHooks';
import LoadingApp from '../gui/components/loadingApp';
Expand All @@ -20,17 +19,11 @@ export const PrivateRoute = (props: PrivateRouteProps): React.ReactElement => {
// Get a default token - this is the first listing's token
// TODO use a context provider for listings instead of getting first entry
const anyToken = useGetAnyToken();
console.log(`Found token ${anyToken.data ?? 'UNDEFINED/loading'}`);

// The token is being retrieved
if (anyToken.isFetching) {
return <LoadingApp />;
}

const {children} = props;
return !!anyToken.data?.token || DISABLE_SIGNIN_REDIRECT ? (
children
) : (
<Navigate to={ROUTES.SIGN_IN} />
);
if (anyToken.data?.token) return props.children;
else return <Navigate to={ROUTES.SIGN_IN} />;
};
1 change: 0 additions & 1 deletion app/src/context/functions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ export const getDefaultToken = async (): Promise<JWTTokenInfo | undefined> => {
export const getAnyToken = async (): Promise<JWTTokenInfo | undefined> => {
// Get listings
const listings = await getListings();
console.log(listings);

// If there is an entry, use first
for (const listing of listings) {
Expand Down
5 changes: 1 addition & 4 deletions app/src/gui/components/authentication/appbarAuth.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,9 @@ import {checkToken} from '../../../utils/helpers';
import {Person} from '@mui/icons-material';
import {theme} from '../../themes';

interface SignInButtonComponentProps {}
const SignInButtonComponent = (props: SignInButtonComponentProps) => {
const SignInButtonComponent = () => {
/**
Component: SignInButtonComponent
*/
return (
<Button
Expand Down Expand Up @@ -69,7 +67,6 @@ const AuthenticatedDisplayComponent = (
) => {
/**
Component: AuthenticatedDisplayComponent
*/

/**
Expand Down
1 change: 0 additions & 1 deletion app/src/gui/components/authentication/shortCodeOnly.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ interface ShortCodeOnlyComponentProps {
export const ShortCodeOnlyComponent = (props: ShortCodeOnlyComponentProps) => {
/**
Component: ShortCodeOnlyComponent
*/

const [shortCode, setShortCode] = useState('');
Expand Down
2 changes: 1 addition & 1 deletion app/src/gui/components/ui/tab-grid.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {useNavigate} from 'react-router-dom';
import * as ROUTES from '../../../constants/routes';
import {useEffect, useState} from 'react';
import {theme} from '../../themes';
import { ACTIVATED_LABEL, NOT_ACTIVATED_LABEL } from '../workspace/notebooks';
import {ACTIVATED_LABEL, NOT_ACTIVATED_LABEL} from '../workspace/notebooks';

/**
* Renders a tabbed grid component.
Expand Down
2 changes: 1 addition & 1 deletion app/src/gui/components/workspace/notebooks.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {grey} from '@mui/material/colors';
import {useTheme} from '@mui/material/styles';
import useMediaQuery from '@mui/material/useMediaQuery';
import {GridColDef} from '@mui/x-data-grid';
import {useContext, useEffect, useState} from 'react';
import {useContext, useState} from 'react';
import {useNavigate} from 'react-router-dom';
import {
NOTEBOOK_LIST_TYPE,
Expand Down
42 changes: 0 additions & 42 deletions app/src/gui/pages/notebook_list.tsx

This file was deleted.

12 changes: 3 additions & 9 deletions app/src/gui/pages/signin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,22 +31,16 @@ import Breadcrumbs from '../components/ui/breadcrumbs';
import {QRCodeRegistration, ShortCodeRegistration} from './shortcode';
import OnboardingComponent from '../components/authentication/oneServerLanding';
import {Capacitor} from '@capacitor/core';
import {useGetDefaultToken} from '../../utils/tokenHooks';
import {getAnyToken, getDefaultToken} from '../../context/functions';
import {useQuery} from '@tanstack/react-query';
import {useGetAnyToken} from '../../utils/tokenHooks';

export function SignIn() {
const [listings, setListings] = useState<ListingsObject[] | null>(null);
const breadcrumbs = [{link: ROUTES.INDEX, title: 'Home'}, {title: 'Sign In'}];
const platform = Capacitor.getPlatform();
const allowQr = platform === 'ios' || platform === 'android';

// This will always fetch the token on first load of component
const tokenQuery = useQuery({
queryKey: ['forceful token fetch'],
queryFn: getAnyToken,
refetchOnMount: true,
});
// Get the current login token if any
const tokenQuery = useGetAnyToken();

useEffect(() => {
getSyncableListingsInfo().then(setListings).catch(logError);
Expand Down
1 change: 1 addition & 0 deletions app/src/gui/themes/themes.d.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// eslint-disable-next-line @typescript-eslint/no-unused-vars
import {PaletteOptions, TypeBackground} from '@mui/material/styles';

declare module '@mui/material/styles' {
Expand Down
15 changes: 13 additions & 2 deletions app/src/utils/tokenHooks.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,22 @@ export const useGetAnyToken = () => {
const query = useQuery({
// The query function that fetches the token
queryFn: async () => {
return await getAnyToken();
const result = await getAnyToken();
// can't return undefined or useQuery thinks it's an error
if (result === undefined) {
// this looks enough like TokenContents to fool the callers
return {token: undefined, parsedToken: undefined};
} else {
return result;
}
},
// The query key, which includes the listing ID to ensure proper caching
// we don't want this to be cached since then logout would break
queryKey: ['get any token'],
refetchOnWindowFocus: true,
refetchOnMount: true,
gcTime: 0, // don't cache it
staleTime: 0, // no really, please don't
enabled: true, // always refetch
});

// Return an object with the token and query state
Expand Down

0 comments on commit f06b228

Please sign in to comment.