Add switch orga to legacy routes #8257
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -234,39 +234,57 @@ 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(_)) | ||
| isSuperUser <- multiUserDAO.findOne(request.identity._multiUser).map(_.isSuperUser) | ||
| selectedOrganization <- if (isSuperUser) | ||
| accessibleBySwitchingForSuperUser(datasetIdValidated, | ||
| datasetDirectoryName, | ||
| organizationId, | ||
| annotationId, | ||
| workflowHash) | ||
| else | ||
| accessibleBySwitchingForMultiUser(request.identity._multiUser, | ||
| datasetIdValidated, | ||
| datasetDirectoryName, | ||
| organizationId, | ||
| annotationId, | ||
| workflowHash) | ||
| _ <- 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, 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(datasetDirectoryName), Some(organizationId), None, None) => | ||
| for { | ||
| dataset <- datasetDAO.findOneByDirectoryNameAndOrganization(datasetDirectoryName, organizationId) | ||
| organization <- organizationDAO.findOne(dataset._organization) | ||
| } yield organization | ||
| case (None, Some(annotationId), None, None, None) => | ||
|
There was a problem hiding this comment. Critical issue: Incorrect parameter mapping in pattern matching In the pattern matching expressions of case (None, Some(annotationId), None, None, None) => // ...Here, Apply this diff to correct the parameter mapping: - case (None, Some(annotationId), None, None, None) =>
+ case (None, None, None, Some(annotationId), None) =>Ensure that Also applies to: 323-327 There was a problem hiding this comment. oh good catch. That's what I get for using ai auto completion There was a problem hiding this comment. @MichaelBuessemeyer, I'm glad I could help catch that issue! Let me know if there's anything else I can assist with. If you found this review helpful, would you consider giving us a shout-out on X? |
||
| 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, None, None, Some(workflowHash)) => | ||
| for { | ||
| workflow <- voxelyticsDAO.findWorkflowByHash(workflowHash) | ||
| organization <- organizationDAO.findOne(workflow._organization) | ||
|
|
@@ -277,26 +295,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, | ||
| 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, datasetDirectoryNameOpt, organizationIdOpt, annotationIdOpt, workflowHashOpt) match { | ||
| case (Some(datasetId), None, None, None, None) => | ||
| canAccessDataset(ctx, datasetId) | ||
| case (None, Some(datasetDirectoryName), Some(organizationId), None, None) => | ||
| canAccessDatasetByDirectoryNameAndOrganization(ctx, datasetDirectoryName, organizationId) | ||
| case (None, Some(annotationId), None, None, None) => | ||
| canAccessAnnotation(user, ctx, annotationId) | ||
| 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") | ||
| } | ||
|
|
@@ -307,6 +337,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) | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| 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 type React from "react"; | ||||||
|
There was a problem hiding this comment. Correct the import statement for React The import statement Please update the import statement to include the React namespace: -import type React from "react";
+import React from "react";📝 Committable suggestion
Suggested change
|
||||||
| import { useState } from "react"; | ||||||
| import { useEffectOnlyOnce } from "libs/react_hooks"; | ||||||
|
|
||||||
| type Props = { | ||||||
| redirectTo: () => Promise<string>; | ||||||
| history: RouteComponentProps["history"]; | ||||||
| pushToHistory?: boolean; | ||||||
| errorComponent?: React.ReactNode; | ||||||
| }; | ||||||
|
|
||||||
| 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; | ||||||
| } | ||||||
| }; | ||||||
| performRedirect(); | ||||||
| }); | ||||||
|
|
||||||
| return hasError && errorComponent ? errorComponent : null; | ||||||
| }; | ||||||
|
|
||||||
| export default withRouter<RouteComponentProps & Props, any>(AsyncRedirect); | ||||||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,9 @@ | ||
| 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"; | ||
|
|
@@ -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, 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"; | ||
|
|
@@ -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; | ||
|
|
||
|
|
@@ -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" }) | ||
|
There was a problem hiding this comment. hmm, now
but maybe this isn't possible? wdyt? There was a problem hiding this comment.
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?
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 There was a problem hiding this comment. fine by me, don’t have a strong opinion here There was a problem hiding this comment. 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 = | ||
|
|
@@ -198,16 +237,24 @@ class ReactRouter extends React.Component<Props> { | |
| ); | ||
| }; | ||
|
|
||
| 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( | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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:
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.