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

Feat: add access request & granting #37

Open
wants to merge 35 commits into
base: development
Choose a base branch
from

Conversation

NuttyShrimp
Copy link
Collaborator

@NuttyShrimp NuttyShrimp commented Nov 8, 2024

Adds access request & inbox tabs to handle:

  • requesting certain permissions from requestable files
  • Visualizing incoming request & logic to accept or reject these request
  • An inbox with the response on the request to get certain permissions

How to test

I've used firefox built-in container tabs to prevent the need of 2 dev server running in the background.

closes #9

@NuttyShrimp NuttyShrimp marked this pull request as ready for review November 22, 2024 08:45
@NuttyShrimp NuttyShrimp requested a review from elsdvlee November 22, 2024 08:45
Copy link
Collaborator

@elsdvlee elsdvlee left a comment

Choose a reason for hiding this comment

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

I have not looked at the code, but intended to test the functionality.
Doing this, I ran into a first issue: it is unclear to me as a user how to request access.
What is expected in the field WebID: the WebID to whom I want to sent an access request?
What is expected in the field next to it?
Where do I add the path of the file to which I request access?
image
See screenshot: I logged in as electronics_user1. The button 'request access' remains disabled and the field above it as well.

@NuttyShrimp
Copy link
Collaborator Author

NuttyShrimp commented Nov 25, 2024

I have not looked at the code, but intended to test the functionality. Doing this, I ran into a first issue: it is unclear to me as a user how to request access. What is expected in the field WebID: the WebID to whom I want to sent an access request? What is expected in the field next to it? Where do I add the path of the file to which I request access? image See screenshot: I logged in as electronics_user1. The button 'request access' remains disabled and the field above it as well.

I've added some extra info pop-ups to the inputs and made the tree input reactive based on the state of the selected pods & their contents
image
image
image

@elsdvlee
Copy link
Collaborator

elsdvlee commented Nov 25, 2024

I have not looked at the code, but intended to test the functionality. Doing this, I ran into a first issue: it is unclear to me as a user how to request access. What is expected in the field WebID: the WebID to whom I want to sent an access request? What is expected in the field next to it? Where do I add the path of the file to which I request access? image See screenshot: I logged in as electronics_user1. The button 'request access' remains disabled and the field above it as well.

I've added some extra info pop-ups to the inputs and made the tree input reactive based on the state of the selected pods & their contents image image image

Thanks, this clarifies a lot.

@NuttyShrimp
Copy link
Collaborator Author

Daarnaast ben ik ook nog bezig met venster toe te voegen waarin je kan selecteren welke permissies je precies wilt aanvragen. Momenteel is dat nog gehardcode op read permissies

@woutslabbinck
Copy link
Collaborator

First, I want to say I like how it works.
Finally (given that both actors use loama), there is an easy way to request and grant access to other agents without having to deal with access control lists.

Currently, I have not checked the code yet as I have some high level questions:

  • Is there a reason why public agents have write access to the inbox container (<storage/public/loama/inbox>)? Would append access be good enough?
    • write access could be abused by any actor to remove all resources there.
  • Using the UI it is only possible to request read access. Would it be possible to also ask for the other types(append, write, control) as well?
  • There is no visualization for pending access requests. How much work would it be to add that to e.g. the request access tab?
  • For all the requests that you have received access, is there a list somewhere of that? If so, how much work would it be to visualize that?
  • Maybe this last question is a bit stupid, but here we go. When an access request is handled, the notification in the inbox is removed. Right?
    • How much work would it be to keep a list somewhere of all historical requests/grants?

Another reason why I did not look into the code is that it seems to work as expected.
But, I'll take a look when these questions are answered as otherwise I might get too deep for this PR. I think that some of them might turn into new issues so we can merge this already to let other people use this version of loama.

Copy link
Collaborator

@elsdvlee elsdvlee left a comment

Choose a reason for hiding this comment

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

Nice work! Tested and approved.

@NuttyShrimp
Copy link
Collaborator Author

NuttyShrimp commented Dec 2, 2024

First, I want to say I like how it works. Finally (given that both actors use loama), there is an easy way to request and grant access to other agents without having to deal with access control lists.

Currently, I have not checked the code yet as I have some high level questions:

* Is there a reason why public agents have `write` access to the inbox container (`<storage/public/loama/inbox>`)? Would `append` access be good enough?
  
  * `write` access could be abused by any actor to remove all resources there.

* Using the UI it is only possible to request `read` access. Would it be possible to also ask for the other types(`append`, `write`, `control`) as well?

* There is no visualization for pending access requests. How much work would it be to add that to e.g. the request access tab?

* For all the requests that you have received access, is there a list somewhere of that? If so, how much work would it be to visualize that?

* Maybe this last question is a bit stupid, but here we go. When an access request is handled, the notification in the inbox is removed. Right?
  
  * How much work would it be to keep a list somewhere of all historical requests/grants?

Another reason why I did not look into the code is that it seems to work as expected. But, I'll take a look when these questions are answered as otherwise I might get too deep for this PR. I think that some of them might turn into new issues so we can merge this already to let other people use this version of loama.

  • I've updated the permission given to the resoources file to Append and everything seem to work
  • As mentioned above, I started working on that when the PR was already opened so I could get early feedback, as of now this has been added to the PR
  • It shouldn't take that long. It can be just another json file which can be cleaned with the accept/reject notifications
  • If the previous request is fullfilled, we can use the pending request to store our needed data for this history

But I think it's better for the last two to be converted into seperate issues

Copy link
Collaborator

@woutslabbinck woutslabbinck left a comment

Choose a reason for hiding this comment

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

I focused on the interfaces and noticed that not all methods have documentation. It seems that IStore, IController and IAccessRequest are crucial, yet only some methods in these interfaces are detailed. As a result, I am unsure what these methods specifically do. I've added remarks for the ones that are somewhat confusing to me. Could you add documentation describing their functions?

Another high-level aspect I do not understand is why the implementation of AccessRequest requires an IController and why the IController interface itself has a method AccessRequest(): IAccessRequest;.
Could you elaborate how that works?

Finally, there is the granting control on access requests.
We've added a safeguard (a warning clickbox) when you try to give access to other users when granting control access. However, when doing it via access requests, no message pop ups.
Could you add the same safeguard if the requested access includes control permission?

Other than that, it is ready to be merged.

Furthermore, I agree with your last message to make issues for the requested features and I will make those once this PR is merged.

@@ -1,5 +1,10 @@
import { url } from "loama-common";

export interface Resources {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is a Resource? Can you add some comments such that we know what is meant?

permissionsPerSubject: SubjectPermissions<T>[]
}

export interface ResourceAccessRequestNode {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I presume this is a recursive tree structure for a pod that checks where you can have access. Is that correct?
If so, can you document this at this interface?

I furthermore presume the children Record key is also an url, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added a comment to explain what this data structure should hold.
The first part of your comment is correct. But the children keys are path parts and not full urls to make tree visualizations easier

Comment on lines 45 to 46
allowAccessRequest(resourceUrl: string): Promise<void>
disallowAccessRequest(resourceUrl: string): Promise<void>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do this methods send HTTP requests to the acl files?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it sets if the resource is visible for access request


loadAccessRequests(): Promise<AccessRequestMessage[]>;
loadRequestResponses(): Promise<RequestResponseMessage[]>;
removeRequest(messageUrl: string): Promise<void>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the difference between removeRequest(messageUrl: string): Promise<void> and disallowAccessRequest(resourceUrl: string): Promise<void>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've added explanation for both functions to the interfaces.
The first should remove the message from the inbox, the latter disable the resource from being requestable for another person

@NuttyShrimp
Copy link
Collaborator Author

So, I've added explanation to most of the new functions in the interfaces.
Besides that, I've added a popup when accepting an access request that contains the "control" permission.
The Controller & AccessRequest referencing each other is so we just need to create a controller instance that contains everything.
The "problem" we're having here is that the "shared service" & "controller" logic is combined in 1 file, why the access request instance may look out of place.
The AccessRequest needs access to the controller so we can handle the permissions of access requests in the AccessRequest class

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Access Requests and Grants
3 participants