Add switch orga to legacy routes #8257
Conversation
|
Warning Rate limit exceeded@MichaelBuessemeyer has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 43 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant modifications across several files, primarily focusing on the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@daniel-wer Please ignore the request that I assigned you for review. I intended to ask @philippotto but first misclicked and the was unable to reassign you in the view where one can prepare a PR :/ So sorry for the ping In case you prefer to take over the frontend review part fo this PR, feel free to do so :) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
frontend/javascripts/router.tsx (1)
161-164: Consider parameterizing the toast identifierCurrently, the toast identifier
"404"is hardcoded inToast.close("404"). Consider defining this identifier as a constant or ensuring it aligns with toast IDs used elsewhere to maintain consistency and ease future updates.app/models/dataset/Dataset.scala (1)
437-445: Clarify method purpose or consider renamingThe method
findOneByNameAndOrganizationnow queries bydirectoryNameinstead ofnameto maintain backward compatibility with legacy links:WHERE (directoryName = $name)Since the method name implies that it searches by
name, consider renaming it to accurately reflect its functionality, such asfindOneByDirectoryNameForLegacySupport, or update its documentation to clearly indicate that it matches ondirectoryName.frontend/javascripts/components/redirect.tsx (1)
43-45: Consider handling errors without rethrowingIn the
performRedirectfunction, when an error occurs, the code sets thehasErrorstate totrueand rethrows the error:} catch (e) { setHasError(true); throw e; }Rethrowing the error after updating the state may prevent the
errorComponentfrom rendering if an error boundary catches the error. Consider removingthrow e;to allow the component to render theerrorComponentwithout interruption, ensuring a smoother user experience.frontend/javascripts/admin/admin_rest_api.ts (1)
Line range hint
1781-1803: Add JSDoc comments to document the new parameter type.The implementation correctly handles the new case for switching organizations based on dataset directory name and organization ID. However, the function would benefit from JSDoc comments explaining the different parameter types and their use cases.
+/** + * Checks if a dataset or annotation is accessible by switching to another organization. + * @param commandType - One of: + * - TraceOrViewCommand with annotationId for tracing mode + * - TraceOrViewCommand with datasetId for viewing mode + * - Object with directoryName and organizationId for legacy route support + * @returns The organization to switch to if the resource is accessible, null/undefined otherwise + */ export async function isDatasetAccessibleBySwitching( commandType: TraceOrViewCommand | { directoryName: string; organizationId: string; type: "VIEW" }, ): Promise<APIOrganization | null | undefined> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
app/controllers/AuthenticationController.scala(3 hunks)app/models/dataset/Dataset.scala(1 hunks)conf/webknossos.latest.routes(1 hunks)frontend/javascripts/admin/admin_rest_api.ts(2 hunks)frontend/javascripts/components/redirect.tsx(1 hunks)frontend/javascripts/router.tsx(5 hunks)
🔇 Additional comments (4)
frontend/javascripts/router.tsx (3)
5-5: Imports added correctly
The necessary imports of isDatasetAccessibleBySwitching, useSelector, useFetch, BrainSpinnerWithError, CoverWithLogin, and Toast are appropriately added to support the new functionality in the component.
Also applies to: 52-52, 77-79
139-169: Well-implemented LegacyLinkDisambiguateErrorView component
The LegacyLinkDisambiguateErrorView component is correctly implemented as a React Functional Component. It effectively uses useSelector to access the activeUser from the Redux store and useFetch to determine if the dataset is accessible by switching organizations. Conditional rendering based on the presence of user is appropriately handled.
240-257: Proper use of errorComponent in AsyncRedirect
In tracingViewModeLegacy, the errorComponent prop is correctly utilized in AsyncRedirect to render LegacyLinkDisambiguateErrorView when an error occurs during redirection. This enhances error handling by providing a user-friendly interface.
conf/webknossos.latest.routes (1)
30-30: Route definition updated appropriately
The route for /auth/accessibleBySwitching is updated to include the datasetDirectoryName and organizationId parameters, matching the updated method signature in AuthenticationController:
GET /auth/accessibleBySwitching controllers.AuthenticationController.accessibleBySwitching(datasetId: Option[String], datasetDirectoryName: Option[String], organizationId: Option[String], annotationId: Option[String], workflowHash: Option[String])
| 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, 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 accessibleBySwitchingForSuperUser and canAccessDatasetOrAnnotationOrWorkflow, annotationIdOpt is incorrectly assigned to datasetDirectoryNameOpt. For example:
case (None, Some(annotationId), None, None, None) => // ...Here, Some(annotationId) is mapped to datasetDirectoryNameOpt, which is incorrect. This misalignment can lead to unintended behavior and logical errors.
Apply this diff to correct the parameter mapping:
- case (None, Some(annotationId), None, None, None) =>
+ case (None, None, None, Some(annotationId), None) =>Ensure that annotationIdOpt is correctly positioned in the pattern matching to accurately reflect the function parameters. Repeat this correction in both accessibleBySwitchingForSuperUser and canAccessDatasetOrAnnotationOrWorkflow methods.
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?
Thank you for using CodeRabbit!
| datasetDirectoryName, | ||
| organizationId, | ||
| annotationId, | ||
| workflowHash) |
There was a problem hiding this comment.
🛠️ 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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/controllers/AuthenticationController.scala (3)
244-247: Consider combining the parameter validation checks.The validation logic for
datasetDirectoryNameandorganizationIdparameters could be more concise.Consider this alternative implementation:
- _ <- Fox - .runIf(datasetDirectoryName.isDefined)(bool2Fox(organizationId.isDefined)) ?~> "Organization id must be defined if dataset directory name is defined." - _ <- Fox - .runIf(organizationId.isDefined)(bool2Fox(datasetDirectoryName.isDefined)) ?~> "Directory name must be defined if dataset organization id is defined." + _ <- bool2Fox(datasetDirectoryName.isDefined == organizationId.isDefined) ?~> "Dataset directory name and organization id must be either both defined or both undefined."
273-284: Improve error message for invalid parameter combinations.The error message "Can either test access for dataset or annotation or workflow, not a combination" could be more specific about which parameters were provided and what combinations are valid.
Consider enhancing the error message:
- case _ => Fox.failure("Can either test access for dataset or annotation or workflow, not a combination") + case _ => Fox.failure("Invalid parameter combination. Valid combinations are: (datasetId), (datasetDirectoryName, organizationId), (annotationId), or (workflowHash)")
344-349: Enhance error handling and debugging capabilities.Consider adding:
- Validation for the organization ID format
- Debug logging to help troubleshoot access issues
Consider this enhanced implementation:
private def canAccessDatasetByDirectoryNameAndOrganization(ctx: DBAccessContext, datasetDirectoryName: String, organizationId: String): Fox[Boolean] = { + logger.debug(s"Checking access to dataset $datasetDirectoryName in organization $organizationId") + for { + _ <- ObjectId.fromString(organizationId).toFox + foundFox = datasetDAO.findOneByDirectoryNameAndOrganization(datasetDirectoryName, organizationId)(ctx) + result <- foundFox.futureBox.map(_.isDefined) + _ = logger.debug(s"Access result for dataset $datasetDirectoryName: $result") + } yield result - val foundFox = datasetDAO.findOneByDirectoryNameAndOrganization(datasetDirectoryName, organizationId)(ctx) - foundFox.futureBox.map(_.isDefined) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/controllers/AuthenticationController.scala(3 hunks)
🔇 Additional comments (1)
app/controllers/AuthenticationController.scala (1)
261-261: Re-evaluate the organization comparison logic.
The check prevents switching organizations if the user is already in the correct organization, which might prevent legitimate access scenarios where a user lacks access to a dataset within their current organization due to permission restrictions.
…-switch-orga-for-legacy-links
frontend/javascripts/router.tsx
Outdated
| const organizationToSwitchTo = useFetch( | ||
| async () => { | ||
| return user | ||
| ? isDatasetAccessibleBySwitching({ directoryName, organizationId, type: "VIEW" }) |
There was a problem hiding this comment.
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:
- there is a conversion from legacy to non legacy and afterwards the accessibility check is done
- 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?
There was a problem hiding this comment.
- 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?
- 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
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.
Maybe internally we could create a DatasetDescriptor or so that has |
…-switch-orga-for-legacy-links
|
Hmm, I’m not sure. It would be yet another descriptor/id/selector thingy floating around. I’d rather hope that we can unify this so that only one disambiguate route can serve all purposes where this is needed. But that’s not for this PR. |
…-switch-orga-for-legacy-links
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/controllers/AuthenticationController.scala (1)
244-244: Consider improving the error messageWhile the validation logic is correct, the error message could be more descriptive to help API consumers understand the requirement better.
- _ <- bool2Fox(datasetDirectoryName.isDefined == organizationId.isDefined) ?~> "Organization id and directory name must be defined together." + _ <- bool2Fox(datasetDirectoryName.isDefined == organizationId.isDefined) ?~> "Dataset directory name and organization ID must either both be provided or both be omitted."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/controllers/AuthenticationController.scala(3 hunks)
🔇 Additional comments (5)
app/controllers/AuthenticationController.scala (5)
237-238: LGTM: Method signature changes align with requirements
The addition of datasetDirectoryName and organizationId parameters enables support for legacy routes, which is consistent with the PR objectives.
247-251: LGTM: Super user access logic properly updated
The parameter passing to accessibleBySwitchingForSuperUser is correctly ordered and consistent with the method signature.
258-258: 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.
270-281: LGTM: Pattern matching correctly implemented
The pattern matching has been properly structured with the new case for directory name and organization ID. The parameter mapping issues identified in previous reviews have been addressed.
341-346: Verify the usage of the new access check method
The implementation looks correct, but let's verify its usage across the codebase.
✅ Verification successful
Let me gather more context about the usage to ensure proper validation.
Let me check the DAO method implementation to ensure complete verification.
Let me check the implementation in Dataset.scala to verify the DAO method.
New access check method is properly implemented and used
The verification shows that:
- The method is used in a pattern-matched context ensuring mutually exclusive access checks
- Parameters are properly validated through the DAO layer
- The implementation follows the same pattern as other access check methods
- The DAO method is consistently used across the codebase with proper context and organization validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for all calls to the new access check method
# and verify that the parameters are properly validated.
rg -A 2 "canAccessDatasetByDirectoryNameAndOrganization"
Length of output: 828
Script:
#!/bin/bash
# Get more context around the usage to verify parameter validation
rg -B 10 -A 5 "canAccessDatasetByDirectoryNameAndOrganization\(" app/controllers/AuthenticationController.scala
Length of output: 1959
Script:
#!/bin/bash
# Check the DAO implementation for findOneByDirectoryNameAndOrganization
ast-grep --pattern 'findOneByDirectoryNameAndOrganization'
Length of output: 1455
Script:
#!/bin/bash
# Get the implementation of findOneByDirectoryNameAndOrganization from Dataset.scala
rg -B 2 -A 6 "def findOneByDirectoryNameAndOrganization" app/models/dataset/Dataset.scala
Length of output: 437
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
app/controllers/DatasetController.scala(3 hunks)app/models/dataset/Dataset.scala(1 hunks)frontend/javascripts/components/redirect.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/dataset/Dataset.scala
🔇 Additional comments (4)
app/controllers/DatasetController.scala (3)
23-23: Import necessary classes from LiftWeb Common
The import statement import net.liftweb.common.{Failure, Full, Empty} brings in the Failure, Full, and Empty classes used for pattern matching with Box types below.
88-88: Inject AuthenticationController into DatasetController
Adding authenticationController: AuthenticationController to the constructor parameters allows the controller to utilize authentication methods required for the new logic in dataset retrieval.
410-433:
Ensure secure access control when switching organizations
The method attempts to access datasets from other organizations using GlobalAccessContext and authenticationController.accessibleBySwitching. While this facilitates access to datasets via organization switching, it's crucial to ensure that this does not expose datasets to unauthorized users.
Please verify that appropriate access controls are in place within accessibleBySwitching to prevent security vulnerabilities.
As a follow-up, consider adding tests to ensure that datasets from other organizations cannot be accessed without proper permissions.
frontend/javascripts/components/redirect.tsx (1)
12-39: Refactor to functional component using React hooks
Converting AsyncRedirect to a functional component with hooks simplifies the component structure and improves code readability.
| datasetBox <- datasetDAO.findOneByNameAndOrganization(datasetName, organizationId).futureBox | ||
| result <- (datasetBox match { | ||
| case Full(dataset) => | ||
| Fox.successful( | ||
| Ok( | ||
| Json.obj("id" -> dataset._id, | ||
| "name" -> dataset.name, | ||
| "organization" -> dataset._organization, | ||
| "directoryName" -> dataset.directoryName))) | ||
| case Empty => | ||
| for { | ||
| dataset <- datasetDAO.findOneByNameAndOrganization(datasetName, organizationId)(GlobalAccessContext) | ||
| isAccessibleResult <- authenticationController.accessibleBySwitching(Some(dataset._id.toString), None, None)(request) | ||
| result <- isAccessibleResult.header.status match { | ||
| case 200 => Fox.successful(Ok(Json.obj("id" -> dataset._id, | ||
| "name" -> dataset.name, | ||
| "organization" -> dataset._organization, | ||
| "directoryName" -> dataset.directoryName))) | ||
| case _ => Fox.failure(notFoundMessage(datasetName)) | ||
| } | ||
| } yield result | ||
| case _ => Fox.failure(notFoundMessage(datasetName)) | ||
| }) ?~> notFoundMessage(datasetName) ~> NOT_FOUND | ||
| } yield result |
There was a problem hiding this comment.
Handle missing datasets when accessing with GlobalAccessContext
In the case Empty branch of the getDatasetIdFromNameAndOrganization method, if datasetDAO.findOneByNameAndOrganization returns None, attempting to access dataset._id will result in a NullPointerException.
Please add error handling to manage scenarios where the dataset is not found in GlobalAccessContext to prevent potential exceptions.
Apply this diff to handle the missing dataset case:
case Empty =>
for {
- dataset <- datasetDAO.findOneByNameAndOrganization(datasetName, organizationId)(GlobalAccessContext)
+ datasetOption <- datasetDAO.findOneByNameAndOrganization(datasetName, organizationId)(GlobalAccessContext)
+ dataset <- datasetOption ?~> notFoundMessage(datasetName)
isAccessibleResult <- authenticationController.accessibleBySwitching(Some(dataset._id.toString), None, None)(request)
result <- isAccessibleResult.header.status match {
case 200 => Fox.successful(Ok(Json.obj("id" -> dataset._id,
"name" -> dataset.name,
"organization" -> dataset._organization,
"directoryName" -> dataset.directoryName)))
case _ => Fox.failure(notFoundMessage(datasetName))
}
} yield result📝 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.
| datasetBox <- datasetDAO.findOneByNameAndOrganization(datasetName, organizationId).futureBox | |
| result <- (datasetBox match { | |
| case Full(dataset) => | |
| Fox.successful( | |
| Ok( | |
| Json.obj("id" -> dataset._id, | |
| "name" -> dataset.name, | |
| "organization" -> dataset._organization, | |
| "directoryName" -> dataset.directoryName))) | |
| case Empty => | |
| for { | |
| dataset <- datasetDAO.findOneByNameAndOrganization(datasetName, organizationId)(GlobalAccessContext) | |
| isAccessibleResult <- authenticationController.accessibleBySwitching(Some(dataset._id.toString), None, None)(request) | |
| result <- isAccessibleResult.header.status match { | |
| case 200 => Fox.successful(Ok(Json.obj("id" -> dataset._id, | |
| "name" -> dataset.name, | |
| "organization" -> dataset._organization, | |
| "directoryName" -> dataset.directoryName))) | |
| case _ => Fox.failure(notFoundMessage(datasetName)) | |
| } | |
| } yield result | |
| case _ => Fox.failure(notFoundMessage(datasetName)) | |
| }) ?~> notFoundMessage(datasetName) ~> NOT_FOUND | |
| } yield result | |
| datasetBox <- datasetDAO.findOneByNameAndOrganization(datasetName, organizationId).futureBox | |
| result <- (datasetBox match { | |
| case Full(dataset) => | |
| Fox.successful( | |
| Ok( | |
| Json.obj("id" -> dataset._id, | |
| "name" -> dataset.name, | |
| "organization" -> dataset._organization, | |
| "directoryName" -> dataset.directoryName))) | |
| case Empty => | |
| for { | |
| datasetOption <- datasetDAO.findOneByNameAndOrganization(datasetName, organizationId)(GlobalAccessContext) | |
| dataset <- datasetOption ?~> notFoundMessage(datasetName) | |
| isAccessibleResult <- authenticationController.accessibleBySwitching(Some(dataset._id.toString), None, None)(request) | |
| result <- isAccessibleResult.header.status match { | |
| case 200 => Fox.successful(Ok(Json.obj("id" -> dataset._id, | |
| "name" -> dataset.name, | |
| "organization" -> dataset._organization, | |
| "directoryName" -> dataset.directoryName))) | |
| case _ => Fox.failure(notFoundMessage(datasetName)) | |
| } | |
| } yield result | |
| case _ => Fox.failure(notFoundMessage(datasetName)) | |
| }) ?~> notFoundMessage(datasetName) ~> NOT_FOUND | |
| } yield result |
| @@ -1,46 +1,41 @@ | |||
| import type { RouteComponentProps } from "react-router-dom"; | |||
| import { withRouter } from "react-router-dom"; | |||
| import React from "react"; | |||
| import type React from "react"; | |||
There was a problem hiding this comment.
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.
| import type React from "react"; | |
| import React from "react"; |
| const redirect = async () => { | ||
| 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 (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); | ||
| if (pushToHistory) { | ||
| history.push(newPath); | ||
| } else { | ||
| location.replace(newPath); | ||
| history.replace(newPath); | ||
| } | ||
| return; | ||
| } | ||
| }; | ||
|
|
||
| if (this.props.pushToHistory) { | ||
| this.props.history.push(newPath); | ||
| } else { | ||
| this.props.history.replace(newPath); | ||
| } | ||
| } | ||
| redirect(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to the asynchronous redirect function
In the redirect async function, any errors thrown by redirectTo() or during the redirection process are not currently caught. This can lead to unhandled promise rejections and make debugging difficult.
Consider adding a try-catch block to handle potential errors gracefully.
Apply this diff to add error handling:
const redirect = async () => {
+ try {
const newPath = await redirectTo();
if (newPath.startsWith(location.origin)) {
// The link is absolute which react-router does not support
if (pushToHistory) {
location.assign(newPath);
} else {
location.replace(newPath);
}
return;
}
if (pushToHistory) {
history.push(newPath);
} else {
history.replace(newPath);
}
+ } catch (error) {
+ console.error('Redirect failed:', error);
+ // Optionally, display an error message or navigate to an error page
+ }
};📝 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.
| const redirect = async () => { | |
| 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 (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); | |
| if (pushToHistory) { | |
| history.push(newPath); | |
| } else { | |
| location.replace(newPath); | |
| history.replace(newPath); | |
| } | |
| return; | |
| } | |
| }; | |
| if (this.props.pushToHistory) { | |
| this.props.history.push(newPath); | |
| } else { | |
| this.props.history.replace(newPath); | |
| } | |
| } | |
| redirect(); | |
| }); | |
| const redirect = 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 (error) { | |
| console.error('Redirect failed:', error); | |
| // Optionally, display an error message or navigate to an error page | |
| } | |
| }; | |
| redirect(); | |
| }); |
There was a problem hiding this comment.
That's how it has always been @philippotto. Do you think we should catch errors here?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Ok I gave this PR a whole new rework.
There are nearly no frontend changes left. I only kept the refactoring of the AsyncRedirect component to make it a functional hook-based component.
The original issue / bug is now solved backend. The disambiguate route now checks whether the dataset is accessible by the current user if that user would to an organization change, in case the dataset could not be found by a simple user context based db request. If the user is able to view the dataset when switching the organization, the route still returns the id of the dataset.
=> This lets the frontend change the route from the old schema (orgaId/datasetDirectoryName) to the new format (name-datasetId) and at that point the frontend automatically realizes that the user needs to perform an orga switch (because the backend disallows access to the DS but returns a positive result on the orga switching route request).
This change results in less edited code lines but the changes in the backend might need some more hands on. Depending on whether it is ok to have the DatasetController have the AuthenticationController as an injected dependency @fm3. Maybe most of the function used from the AuthenticationController can be put into the service class.
Besides these thoughts, this pr is ready for another review round
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
CHANGELOG.unreleased.md (1)
24-24: Enhance the changelog entry to be more descriptive.While the current entry correctly identifies the fix, it could be more descriptive about the specific issue and technical changes. Consider expanding it to:
-- Fixed the organization switching feature for datasets opened via old links. [#8257](https://github.com/scalableminds/webknossos/pull/8257) +- Fixed the organization switching feature for datasets accessed through legacy routes by ensuring proper resolution using directoryName instead of datasetId. [#8257](https://github.com/scalableminds/webknossos/pull/8257)This provides more context about:
- The specific component affected (legacy routes)
- The technical change made (using directoryName)
- The reason for the change (proper resolution)
|
Sweet! I think that’s a good approach! (Did not go through the code again yet). |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/controllers/DatasetController.scala (2)
410-438: Consider adding logging for debugging organization switching.Since this code handles organization switching functionality, adding debug logging would help track and diagnose issues in production.
Add logging statements:
case Empty => for { + _ = logger.debug(s"Dataset not found in current context, attempting organization switch for dataset: $datasetName") datasetOption <- datasetDAO.findOneByNameAndOrganization(datasetName, organizationId)(GlobalAccessContext) dataset <- datasetOption ?~> notFoundMessage(datasetName) + _ = logger.debug(s"Found dataset in global context, attempting to switch organization for dataset: ${dataset._id}") isAccessibleResult <- authenticationController.accessibleBySwitching(Some(dataset._id.toString), None, None)(request) result <- isAccessibleResult.header.status match { case 200 => + _ = logger.debug(s"Successfully switched organization for dataset: ${dataset._id}") Fox.successful(Ok(Json.obj("id" -> dataset._id, "name" -> dataset.name, "organization" -> dataset._organization, "directoryName" -> dataset.directoryName))) case _ => + _ = logger.debug(s"Failed to switch organization for dataset: ${dataset._id}") Fox.failure(notFoundMessage(datasetName)) } } yield result
410-438: Consider extracting the JSON response into a helper method.The JSON response structure is duplicated in both the
FullandEmptycases. Consider extracting it into a helper method to improve maintainability and reduce code duplication.Create a helper method:
+ private def datasetToJson(dataset: Dataset): JsObject = { + Json.obj( + "id" -> dataset._id, + "name" -> dataset.name, + "organization" -> dataset._organization, + "directoryName" -> dataset.directoryName + ) + } def getDatasetIdFromNameAndOrganization(datasetName: String, organizationId: String): Action[AnyContent] = sil.UserAwareAction.async { implicit request => for { datasetBox <- datasetDAO.findOneByNameAndOrganization(datasetName, organizationId).futureBox result <- (datasetBox match { case Full(dataset) => - Fox.successful(Ok(Json.obj("id" -> dataset._id, - "name" -> dataset.name, - "organization" -> dataset._organization, - "directoryName" -> dataset.directoryName))) + Fox.successful(Ok(datasetToJson(dataset))) case Empty => for { datasetOption <- datasetDAO.findOneByNameAndOrganization(datasetName, organizationId)(GlobalAccessContext) dataset <- datasetOption ?~> notFoundMessage(datasetName) isAccessibleResult <- authenticationController.accessibleBySwitching(Some(dataset._id.toString), None, None)(request) result <- isAccessibleResult.header.status match { - case 200 => Fox.successful(Ok(Json.obj("id" -> dataset._id, - "name" -> dataset.name, - "organization" -> dataset._organization, - "directoryName" -> dataset.directoryName))) + case 200 => Fox.successful(Ok(datasetToJson(dataset))) case _ => Fox.failure(notFoundMessage(datasetName)) } } yield result case _ => Fox.failure(notFoundMessage(datasetName)) }) ?~> notFoundMessage(datasetName) ~> NOT_FOUND } yield result }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/controllers/DatasetController.scala(3 hunks)
🔇 Additional comments (1)
app/controllers/DatasetController.scala (1)
410-438:
Potential null pointer exception in the Empty case.
In the Empty case, if findOneByNameAndOrganization returns None, accessing dataset._id will result in a NullPointerException. The code needs to handle the case where the dataset is not found in GlobalAccessContext.
Apply this diff to handle the missing dataset case:
case Empty =>
for {
- dataset <- datasetDAO.findOneByNameAndOrganization(datasetName, organizationId)(GlobalAccessContext)
+ datasetOption <- datasetDAO.findOneByNameAndOrganization(datasetName, organizationId)(GlobalAccessContext)
+ dataset <- datasetOption ?~> notFoundMessage(datasetName)
isAccessibleResult <- authenticationController.accessibleBySwitching(Some(dataset._id.toString), None, None)(request)
result <- isAccessibleResult.header.status match {
case 200 => Fox.successful(Ok(Json.obj("id" -> dataset._id,
"name" -> dataset.name,
"organization" -> dataset._organization,
"directoryName" -> dataset.directoryName)))
case _ => Fox.failure(notFoundMessage(datasetName))
}
} yield resultLikely invalid or redundant comment.
Excellent 👍 frontend lgtm to me. leaving final review to @fm3. |
…-switch-orga-for-legacy-links
There was a problem hiding this comment.
I would like to avoid the controller classes depending on each other. Maybe you could extract the relevant logic into a service?
I tired to do this and it works but I am not sure, whether the location where I extracted it to is good.
I could not find an AuthenticationService and the functionality did not match for going into the User/Dataset/OrganizationService. Thus, I created a AuthenticationService and moved it to app/security as I could not find a suitable place for this service. I am very open to suggestion here :)
Besides that, everything should be working smoothly :D
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/security/AuthenticationService.scala (2)
108-108: Handle compound annotations incanAccessAnnotationThe comment indicates that the current implementation does not support compound annotations:
// Note: this does not work for compound annotations.Consider extending support for compound annotations or explicitly handling this case to prevent unexpected behavior for users who might work with compound annotations.
88-97: Refactor to reduce code duplication in pattern matchingThe pattern matching logic in
canAccessDatasetOrAnnotationOrWorkflowis similar to that inaccessibleBySwitchingForSuperUser. To improve maintainability and reduce code duplication, consider abstracting the common pattern matching into a helper method.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
CHANGELOG.unreleased.md(1 hunks)app/controllers/AuthenticationController.scala(3 hunks)app/controllers/DatasetController.scala(3 hunks)app/security/AuthenticationService.scala(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CHANGELOG.unreleased.md
- app/controllers/DatasetController.scala
🔇 Additional comments (3)
app/security/AuthenticationService.scala (1)
44-44: 🛠️ Refactor suggestion
Re-evaluate the organization comparison logic
The condition:
_ <- bool2Fox(selectedOrganization._id != user._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 user's current organization. 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.
app/controllers/AuthenticationController.scala (2)
40-40: Dependency injection of AuthenticationService is correctly implemented
The AuthenticationService is properly injected into the AuthenticationController, replacing the annotationProvider. This update centralizes authentication logic and promotes better code organization.
222-225: Parameters are correctly aligned in the method call
The parameters passed to getOrganizationToSwitchTo are in the correct order, matching the method signature in AuthenticationService. This ensures that the correct identifiers are used during organization retrieval.
| private def canAccessAnnotation(user: User, ctx: DBAccessContext, annotationId: String): Fox[Boolean] = { | ||
| val foundFox = for { | ||
| annotationIdParsed <- ObjectId.fromString(annotationId) | ||
| annotation <- annotationDAO.findOne(annotationIdParsed)(GlobalAccessContext) |
There was a problem hiding this comment.
Avoid using GlobalAccessContext to ensure proper access control
In the canAccessAnnotation method, using GlobalAccessContext bypasses authorization checks:
annotation <- annotationDAO.findOne(annotationIdParsed)(GlobalAccessContext) // Note: this does not work for compound annotations.This may lead to security issues by allowing access to annotations without proper permissions. Consider using the provided ctx (the authorized access context) to ensure that access control is correctly enforced.
Apply this diff to fix the issue:
- annotation <- annotationDAO.findOne(annotationIdParsed)(GlobalAccessContext) // Note: this does not work for compound annotations.
+ annotation <- annotationDAO.findOne(annotationIdParsed)(ctx) // Note: this does not work for compound annotations.
There was a problem hiding this comment.
Nice! Since the service does not currently contain any other authentication logic, how about we name it AccessibleBySwitchingService for the time being? if/when we move more logic there we cann call it AuthenticationService again. Also, I think it does not need to extend SqlEscaping as it does not interact with postgres directly. security package is fine by me.
| @@ -317,7 +318,7 @@ class DatasetController @Inject()(userService: UserService, | |||
| def update(datasetId: String): Action[JsValue] = | |||
| sil.SecuredAction.async(parse.json) { implicit request => | |||
| withJsonBodyUsing(datasetPublicReads) { | |||
| case (description, datasetName, legacyDatasetDisplayName, sortingKey, isPublic, tags, metadata, folderId) => { | |||
| case (description, datasetName, legacyDatasetDisplayName, sortingKey, isPublic, tags, metadata, folderId) => | |||
There was a problem hiding this comment.
My IDE complained about redundant brackets. Thus, I removed them
Thanks and all done :) |
On it :) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
app/security/AccessibleBySwitchingService.scala (3)
43-43: Clarify the comment to match the condition logicThe comment may be confusing as it suggests the user is already in the correct organization, but the condition checks if the selected organization is different from the user's current organization. Consider rephrasing the comment for clarity.
Apply this diff to improve the comment:
- _ <- bool2Fox(selectedOrganization._id != user._organization) // User is already in correct orga, but still could not see dataset. Assume this had a reason. + _ <- bool2Fox(selectedOrganization._id != user._organization) // User is not in the correct organization, proceed to switch organization.
59-59: Consider handling compound annotationsCurrently, the methods do not support compound annotations, as noted in the code comments. To improve functionality and ensure full compatibility, consider adding support for compound annotations.
Also applies to: 106-106
1-122: Add unit tests forAccessibleBySwitchingServiceConsider adding unit tests to cover different scenarios and ensure the correctness of the access control logic implemented in this service.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
app/controllers/AuthenticationController.scala(3 hunks)app/controllers/DatasetController.scala(4 hunks)app/security/AccessibleBySwitchingService.scala(1 hunks)
👮 Files not reviewed due to content moderation or server errors (2)
- app/controllers/DatasetController.scala
- app/controllers/AuthenticationController.scala
| private def canAccessAnnotation(user: User, ctx: DBAccessContext, annotationId: String): Fox[Boolean] = { | ||
| val foundFox = for { | ||
| annotationIdParsed <- ObjectId.fromString(annotationId) | ||
| annotation <- annotationDAO.findOne(annotationIdParsed)(GlobalAccessContext) |
There was a problem hiding this comment.
Use ctx instead of GlobalAccessContext when fetching annotation
To ensure proper access control based on the user's permissions, fetch the annotation using the authorized context ctx rather than GlobalAccessContext.
Apply this diff to fix the issue:
- annotation <- annotationDAO.findOne(annotationIdParsed)(GlobalAccessContext)
+ annotation <- annotationDAO.findOne(annotationIdParsed)(ctx)📝 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.
| annotation <- annotationDAO.findOne(annotationIdParsed)(GlobalAccessContext) | |
| annotation <- annotationDAO.findOne(annotationIdParsed)(ctx) |
Done and worked. |
Follow up bug introduced by #8075. In #8075 we missed to support the switch orga functionality for legacy routes. This pr add this functionally now.
URL of deployed dev instance (used for testing):
Steps to test:
isWkOrgInstanceinapplication.confDataset Configuration (as stored on datastore localhost at 15ca057a580b27dc/png_rgb). Copy theorgaId/directoryPathpart.<wk>/datasets/<orgaId/directoryPath>/view. The page should automatically redirect to<wk>/datasets/<name-datasetId>/viewand then fail. The user should be prompted to switch the organization. Upon switching the DS should loadIssues:
(Please delete unneeded items, merge only when none are left open)