Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add switch orga to legacy routes #8257

Merged
merged 17 commits into from
Dec 11, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 53 additions & 12 deletions app/controllers/AuthenticationController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -234,39 +234,61 @@ class AuthenticationController @Inject()(
*/

def accessibleBySwitching(datasetId: Option[String],
datasetDirectoryName: Option[String],
organizationId: Option[String],
annotationId: Option[String],
workflowHash: Option[String]): Action[AnyContent] = sil.SecuredAction.async {
implicit request =>
for {
datasetIdValidated <- Fox.runOptional(datasetId)(ObjectId.fromString(_))
_ <- Fox
.runIf(datasetDirectoryName.isDefined)(bool2Fox(organizationId.isDefined)) ?~> "Organization id must be defined if dataset directory name is defined."
MichaelBuessemeyer marked this conversation as resolved.
Show resolved Hide resolved
_ <- Fox
.runIf(organizationId.isDefined)(bool2Fox(datasetDirectoryName.isDefined)) ?~> "Directory name must be defined if dataset organization id is defined."
isSuperUser <- multiUserDAO.findOne(request.identity._multiUser).map(_.isSuperUser)
selectedOrganization <- if (isSuperUser)
accessibleBySwitchingForSuperUser(datasetIdValidated, annotationId, workflowHash)
accessibleBySwitchingForSuperUser(datasetIdValidated,
datasetDirectoryName,
organizationId,
annotationId,
workflowHash)
else
accessibleBySwitchingForMultiUser(request.identity._multiUser, datasetIdValidated, annotationId, workflowHash)
accessibleBySwitchingForMultiUser(request.identity._multiUser,
datasetIdValidated,
datasetDirectoryName,
organizationId,
annotationId,
workflowHash)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Re-evaluate the organization comparison logic

The check:

_ <- bool2Fox(selectedOrganization._id != request.identity._organization) // User is already in correct orga, but still could not see dataset. Assume this had a reason.

Prevents switching organizations if the selected organization is the same as the current one. However, there may be valid scenarios where a user lacks access to a dataset within their current organization due to permission restrictions. Consider revising this logic to allow for permission checks even when the organization is the same, or provide a more specific error message to guide the user.

_ <- bool2Fox(selectedOrganization._id != request.identity._organization) // User is already in correct orga, but still could not see dataset. Assume this had a reason.
selectedOrganizationJs <- organizationService.publicWrites(selectedOrganization)
} yield Ok(selectedOrganizationJs)
}

private def accessibleBySwitchingForSuperUser(datasetIdOpt: Option[ObjectId],
datasetDirectoryNameOpt: Option[String],
organizationIdOpt: Option[String],
annotationIdOpt: Option[String],
workflowHashOpt: Option[String]): Fox[Organization] = {
implicit val ctx: DBAccessContext = GlobalAccessContext
(datasetIdOpt, annotationIdOpt, workflowHashOpt) match {
case (Some(datasetId), None, None) =>
(datasetIdOpt, datasetDirectoryNameOpt, organizationIdOpt, annotationIdOpt, workflowHashOpt) match {
case (Some(datasetId), None, None, None, None) =>
for {
dataset <- datasetDAO.findOne(datasetId)
organization <- organizationDAO.findOne(dataset._organization)
} yield organization
case (None, Some(annotationId), None) =>
case (None, Some(datasetDirectoryName), Some(organizationId), None, None) =>
for {
dataset <- datasetDAO.findOneByDirectoryNameAndOrganization(datasetDirectoryName, organizationId)
organization <- organizationDAO.findOne(dataset._organization)
} yield organization
case (None, None, None, Some(annotationId), None) =>
for {
annotationObjectId <- ObjectId.fromString(annotationId)
annotation <- annotationDAO.findOne(annotationObjectId) // Note: this does not work for compound annotations.
user <- userDAO.findOne(annotation._user)
organization <- organizationDAO.findOne(user._organization)
} yield organization
case (None, None, Some(workflowHash)) =>
case (None, None, None, None, Some(workflowHash)) =>
for {
workflow <- voxelyticsDAO.findWorkflowByHash(workflowHash)
organization <- organizationDAO.findOne(workflow._organization)
Expand All @@ -277,26 +299,38 @@ class AuthenticationController @Inject()(

private def accessibleBySwitchingForMultiUser(multiUserId: ObjectId,
datasetIdOpt: Option[ObjectId],
datasetDirectoryNameOpt: Option[String],
organizationIdOpt: Option[String],
annotationIdOpt: Option[String],
workflowHashOpt: Option[String]): Fox[Organization] =
for {
identities <- userDAO.findAllByMultiUser(multiUserId)
selectedIdentity <- Fox.find(identities)(identity =>
canAccessDatasetOrAnnotationOrWorkflow(identity, datasetIdOpt, annotationIdOpt, workflowHashOpt))
selectedIdentity <- Fox.find(identities)(
identity =>
canAccessDatasetOrAnnotationOrWorkflow(identity,
datasetIdOpt,
datasetDirectoryNameOpt,
organizationIdOpt,
annotationIdOpt,
workflowHashOpt))
selectedOrganization <- organizationDAO.findOne(selectedIdentity._organization)(GlobalAccessContext)
} yield selectedOrganization

private def canAccessDatasetOrAnnotationOrWorkflow(user: User,
datasetIdOpt: Option[ObjectId],
datasetDirectoryNameOpt: Option[String],
organizationIdOpt: Option[String],
annotationIdOpt: Option[String],
workflowHashOpt: Option[String]): Fox[Boolean] = {
val ctx = AuthorizedAccessContext(user)
(datasetIdOpt, annotationIdOpt, workflowHashOpt) match {
case (Some(datasetId), None, None) =>
(datasetIdOpt, datasetDirectoryNameOpt, organizationIdOpt, annotationIdOpt, workflowHashOpt) match {
case (Some(datasetId), None, None, None, None) =>
canAccessDataset(ctx, datasetId)
case (None, Some(annotationId), None) =>
case (None, Some(datasetDirectoryName), Some(organizationId), None, None) =>
canAccessDatasetByDirectoryNameAndOrganization(ctx, datasetDirectoryName, organizationId)
case (None, None, None, Some(annotationId), None) =>
canAccessAnnotation(user, ctx, annotationId)
case (None, None, Some(workflowHash)) =>
case (None, None, None, None, Some(workflowHash)) =>
canAccessWorkflow(user, workflowHash)
case _ => Fox.failure("Can either test access for dataset or annotation or workflow, not a combination")
}
Expand All @@ -307,6 +341,13 @@ class AuthenticationController @Inject()(
foundFox.futureBox.map(_.isDefined)
}

private def canAccessDatasetByDirectoryNameAndOrganization(ctx: DBAccessContext,
datasetDirectoryName: String,
organizationId: String): Fox[Boolean] = {
val foundFox = datasetDAO.findOneByDirectoryNameAndOrganization(datasetDirectoryName, organizationId)(ctx)
foundFox.futureBox.map(_.isDefined)
}

private def canAccessAnnotation(user: User, ctx: DBAccessContext, annotationId: String): Fox[Boolean] = {
val foundFox = for {
annotationIdParsed <- ObjectId.fromString(annotationId)
Expand Down
9 changes: 4 additions & 5 deletions app/models/dataset/Dataset.scala
Original file line number Diff line number Diff line change
Expand Up @@ -434,16 +434,15 @@ class DatasetDAO @Inject()(sqlClient: SqlClient, datasetLayerDAO: DatasetLayerDA
exists <- r.headOption
} yield exists

// Datasets are looked up by name and directoryName, as datasets from before dataset renaming was possible
// should have their directory name equal to their name during the time the link was created. This heuristic should
// have the best expected outcome as it expect to find the dataset by directoryName and it to be the oldest. In case
// someone renamed a dataset and created the link with a tool that uses the outdated dataset identification, the dataset should still be found.
// Legacy links to Datasets used their name and organizationId as identifier. In #8075 name was changed to directoryName.
// Thus, interpreting the name as the directory name should work, as changing the directory name is not possible.
// This way of looking up datasets should only be used for backwards compatibility.
def findOneByNameAndOrganization(name: String, organizationId: String)(implicit ctx: DBAccessContext): Fox[Dataset] =
for {
accessQuery <- readAccessQuery
r <- run(q"""SELECT $columns
FROM $existingCollectionName
WHERE (directoryName = $name OR name = $name)
WHERE (directoryName = $name)
AND _organization = $organizationId
AND $accessQuery
ORDER BY created ASC
Expand Down
2 changes: 1 addition & 1 deletion conf/webknossos.latest.routes
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ GET /auth/token
DELETE /auth/token controllers.AuthenticationController.deleteToken()
GET /auth/switch controllers.AuthenticationController.switchMultiUser(to: String)
POST /auth/switchOrganization/:organizationId controllers.AuthenticationController.switchOrganization(organizationId: String)
GET /auth/accessibleBySwitching controllers.AuthenticationController.accessibleBySwitching(datasetId: Option[String], annotationId: Option[String], workflowHash: Option[String])
GET /auth/accessibleBySwitching controllers.AuthenticationController.accessibleBySwitching(datasetId: Option[String], datasetDirectoryName: Option[String], organizationId: Option[String], annotationId: Option[String], workflowHash: Option[String])
POST /auth/sendInvites controllers.AuthenticationController.sendInvites()
POST /auth/startResetPassword controllers.AuthenticationController.handleStartResetPassword()
POST /auth/changePassword controllers.AuthenticationController.changePassword()
Expand Down
11 changes: 9 additions & 2 deletions frontend/javascripts/admin/admin_rest_api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1778,7 +1778,7 @@ export async function updateOrganization(
}

export async function isDatasetAccessibleBySwitching(
commandType: TraceOrViewCommand,
commandType: TraceOrViewCommand | { directoryName: string; organizationId: string; type: "VIEW" },
): Promise<APIOrganization | null | undefined> {
if (commandType.type === ControlModeEnum.TRACE) {
return Request.receiveJSON(
Expand All @@ -1787,13 +1787,20 @@ export async function isDatasetAccessibleBySwitching(
showErrorToast: false,
},
);
} else {
} else if ("datasetId" in commandType) {
return Request.receiveJSON(
`/api/auth/accessibleBySwitching?datasetId=${commandType.datasetId}`,
{
showErrorToast: false,
},
);
} else {
return Request.receiveJSON(
`/api/auth/accessibleBySwitching?datasetDirectoryName=${commandType.directoryName}&organizationId=${commandType.organizationId}`,
{
showErrorToast: false,
},
);
}
}

Expand Down
73 changes: 40 additions & 33 deletions frontend/javascripts/components/redirect.tsx
Original file line number Diff line number Diff line change
@@ -1,46 +1,53 @@
import type { RouteComponentProps } from "react-router-dom";
import { withRouter } from "react-router-dom";
import React from "react";
import type React from "react";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Correct the import statement for React

The import statement import type React from "react"; imports React as a type-only import, which does not include the runtime value of React. Since React is needed for JSX transformation and to use React.FC, importing it as a type-only import may cause runtime errors.

Please update the import statement to include the React namespace:

-import type React from "react";
+import React from "react";
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import type React from "react";
import React from "react";

import { useState } from "react";
import { useEffectOnlyOnce } from "libs/react_hooks";

type Props = {
redirectTo: () => Promise<string>;
history: RouteComponentProps["history"];
pushToHistory?: boolean;
errorComponent?: React.ReactNode;
};

class AsyncRedirect extends React.PureComponent<Props> {
static defaultProps = {
pushToHistory: true,
};

componentDidMount() {
this.redirect();
}

async redirect() {
const newPath = await this.props.redirectTo();

if (newPath.startsWith(location.origin)) {
// The link is absolute which react-router does not support
// apparently. See https://stackoverflow.com/questions/42914666/react-router-external-link
if (this.props.pushToHistory) {
location.assign(newPath);
} else {
location.replace(newPath);
const AsyncRedirect: React.FC<Props> = ({
redirectTo,
history,
pushToHistory = true,
errorComponent,
}: Props) => {
const [hasError, setHasError] = useState(false);
useEffectOnlyOnce(() => {
const performRedirect = async () => {
try {
const newPath = await redirectTo();

if (newPath.startsWith(location.origin)) {
// The link is absolute which react-router does not support
// apparently. See https://stackoverflow.com/questions/42914666/react-router-external-link
if (pushToHistory) {
location.assign(newPath);
} else {
location.replace(newPath);
}
return;
}

if (pushToHistory) {
history.push(newPath);
} else {
history.replace(newPath);
}
} catch (e) {
setHasError(true);
throw e;
}
return;
}
};
performRedirect();
});

if (this.props.pushToHistory) {
this.props.history.push(newPath);
} else {
this.props.history.replace(newPath);
}
}

render() {
return null;
}
}
return hasError && errorComponent ? errorComponent : null;
};

export default withRouter<RouteComponentProps & Props, any>(AsyncRedirect);
71 changes: 59 additions & 12 deletions frontend/javascripts/router.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import { createExplorational, getAnnotationInformation, getShortLink } from "admin/admin_rest_api";
import {
createExplorational,
getAnnotationInformation,
getShortLink,
isDatasetAccessibleBySwitching,
} from "admin/admin_rest_api";
import AcceptInviteView from "admin/auth/accept_invite_view";
import AuthTokenView from "admin/auth/auth_token_view";
import ChangePasswordView from "admin/auth/change_password_view";
Expand Down Expand Up @@ -44,7 +49,7 @@ import type { OxalisState } from "oxalis/store";
import HelpButton from "oxalis/view/help_modal";
import TracingLayoutView from "oxalis/view/layouting/tracing_layout_view";
import React from "react";
import { connect } from "react-redux";
import { connect, useSelector } from "react-redux";
// @ts-expect-error ts-migrate(2305) FIXME: Module '"react-router-dom"' has no exported member... Remove this comment to see the full error message
import { type ContextRouter, Link, type RouteProps } from "react-router-dom";
import { Redirect, Route, Router, Switch } from "react-router-dom";
Expand All @@ -69,6 +74,9 @@ import {
getOrganizationForDataset,
} from "admin/api/disambiguate_legacy_routes";
import { getDatasetIdOrNameFromReadableURLPart } from "oxalis/model/accessors/dataset_accessor";
import { useFetch } from "libs/react_helpers";
import { BrainSpinnerWithError, CoverWithLogin } from "components/brain_spinner";
import Toast from "libs/toast";

const { Content } = Layout;

Expand Down Expand Up @@ -128,6 +136,37 @@ const SecuredRouteWithErrorBoundary: React.FC<GetComponentProps<typeof SecuredRo
);
};

const LegacyLinkDisambiguateErrorView: React.FC<{
directoryName: string;
organizationId: string;
}> = ({ directoryName, organizationId }) => {
const user = useSelector((state: OxalisState) => state.activeUser);
const organizationToSwitchTo = useFetch(
async () => {
return user
? isDatasetAccessibleBySwitching({ directoryName, organizationId, type: "VIEW" })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, now isDatasetAccessibleBySwitching is used in router.tsx for the legacy links and model.ts for the new links, right? can we avoid that this happens at two places. ideally, i would have hoped that one of these two options would work:

  1. there is a conversion from legacy to non legacy and afterwards the accessibility check is done
  2. the accessibility check can work with the old and new format so that it is only called at one code location

but maybe this isn't possible? wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. there is a conversion from legacy to non legacy and afterwards the accessibility check is done

Sadly that's not possible in the current model of the backend. @fm3 what do you think about adjusting the permissions for the disambiguation route to include positive answers in case the user can access the dataset when they would switch into the correct organization?

  1. the accessibility check can work with the old and new format so that it is only called at one code location

Working with the old format is technically possible but here we have the reverse problem: In case the user opens a URL with the new schema .../datasets/ds_name-12390u1902u31u2301u20/view and is not in the orga of theaccessibleBySwitching route only works on dataset ids, legacy links do not work, and if the route works on organization id and directory name, the new links do not work anymore. Therefore, I'd say both variations must be supported or we do your 1. suggestion

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine by me, don’t have a strong opinion here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I'll implement 1.

: null;
},
null,
[directoryName, organizationId, user],
);
return user ? (
<BrainSpinnerWithError
gotUnhandledError={false}
organizationToSwitchTo={organizationToSwitchTo}
/>
) : (
<CoverWithLogin
onLoggedIn={() => {
// Close existing error toasts for "Not Found" errors before trying again.
// If they get relevant again, they will be recreated anyway.
Toast.close("404");
location.reload();
}}
/>
);
};

class ReactRouter extends React.Component<Props> {
tracingView = ({ match }: ContextRouter) => {
const initialMaybeCompoundType =
Expand Down Expand Up @@ -198,16 +237,24 @@ class ReactRouter extends React.Component<Props> {
);
};

tracingViewModeLegacy = ({ match, location }: ContextRouter) => (
<AsyncRedirect
redirectTo={async () => {
const datasetName = match.params.datasetName || "";
const organizationId = match.params.organizationId || "";
const datasetId = await getDatasetIdFromNameAndOrganization(datasetName, organizationId);
return `/datasets/${datasetName}-${datasetId}/view${location.search}${location.hash}`;
}}
/>
);
tracingViewModeLegacy = ({ match, location }: ContextRouter) => {
const datasetName = match.params.datasetName || "";
const organizationId = match.params.organizationId || "";
return (
<AsyncRedirect
redirectTo={async () => {
const datasetId = await getDatasetIdFromNameAndOrganization(datasetName, organizationId);
return `/datasets/${datasetName}-${datasetId}/view${location.search}${location.hash}`;
}}
errorComponent={
<LegacyLinkDisambiguateErrorView
directoryName={datasetName}
organizationId={organizationId}
/>
}
/>
);
};

tracingViewMode = ({ match }: ContextRouter) => {
const { datasetId, datasetName } = getDatasetIdOrNameFromReadableURLPart(
Expand Down