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

Unauthorized access pages #108

Open
Allcharles opened this issue Feb 19, 2020 · 7 comments
Open

Unauthorized access pages #108

Allcharles opened this issue Feb 19, 2020 · 7 comments
Assignees
Labels
bug Something isn't working triage:medium Medium priority issue or pull request ui feature A new feature/page to add to the project

Comments

@Allcharles
Copy link
Contributor

Currently pages do not verify if the user is able to access the page. They should instead, check the predicate for the page, and determine users authorization status. Some pages (specifically forms) will allow an unauthorized user access to the form without displaying an error.

@Allcharles Allcharles added the bug Something isn't working label Feb 19, 2020
@Allcharles Allcharles self-assigned this Feb 19, 2020
@atruskie
Copy link
Member

The predicate should not be used for determining auth states. They're really only there to hide menu items.

All pages require resolving data. If the server returns no authorised then we act on that. For any services not currently plumbed into a real API they may show an empty page atm, but that won't happen in the future.

@Allcharles
Copy link
Contributor Author

So for the form pages which currently only talk to workbench-client we just leave the auth handling until the forms are moved to the server?

@atruskie
Copy link
Member

I don't understand what you mean?

The server will be the authoritative source on... well... authorised access

@Allcharles
Copy link
Contributor Author

Example page: /projects/new

On this page, there are no requests to the baw server before displaying the form. Because of this the workbench-client doesn't really know if you are allowed to create projects until you try make a project.

@atruskie
Copy link
Member

Ok: let's break this down.

Scenarios for landing on that page:

  1. clicking on link within app (solved by route guard)
  2. direct link to page (page loads)

Ok so page loads and is unauthorised, big woop. They submit form, still unauthorised, mechanism kicks in.

I think we can ignore direct links.

I also think the pages can guard against unauthorised access, only if they need to (i.e. a resource isn't loaded to trigger 403/401). That should be applied sparingly, and only because we want a better user experience, i.e. to redirect them before they bother to fill out a form.

We'll have capabilities metadata available that can be used by menu items to disable themselves, or iff needed to hide themselves (this is the only case where the predicate is used, and should be done so rarely). By default though, disallowed actions shouldn't be hidden.

The capabilities can also be used by new pages to redirect disallowed access.

Importantly: I want most of the auth flow to be driven from server 401/403s and capability responses from API. This way we keep auth logic in one place and do not replicate it.

After re-reading your issue I think I misunderstood you. Currently, only menu items have predicates.

We're you instead suggesting pages should have an authorization predicate?

@Allcharles Allcharles added the ui feature A new feature/page to add to the project label Feb 26, 2020
@Allcharles Allcharles added the triage:medium Medium priority issue or pull request label Mar 9, 2020
@Allcharles
Copy link
Contributor Author

I am thinking that each page has the ability to query the server whether the current user should be able to view the current page by querying a list of actions which may be taken. This would link into the capabilities route most likely, however the actual implementation is up for discussion.

I would image this would come in the form of the page requesting /site/:id/edit/capabilities which would return whether the current user can edit the current site. This way the page can load a 401 error before the user attempts to create a site.

@atruskie
Copy link
Member

atruskie commented Jul 16, 2020

so I envisaged adding a capabilities section to the meta section for any single item response. It'd basically have the format:

{
    "meta": {
        "capabilities": ["update", "delete"]
    }
}

I also thought this could be exposed via a request to /new which could return the json schema in the body, as well as any relevant capabilities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working triage:medium Medium priority issue or pull request ui feature A new feature/page to add to the project
Projects
None yet
Development

No branches or pull requests

2 participants