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

Routes auth config not honoured #7863

Closed
diocas opened this issue Oct 25, 2022 · 9 comments · Fixed by #7875
Closed

Routes auth config not honoured #7863

diocas opened this issue Oct 25, 2022 · 9 comments · Fixed by #7875
Assignees
Labels
Early-Adopter:CERN Type:Bug Something isn't working

Comments

@diocas
Copy link
Contributor

diocas commented Oct 25, 2022

I might want to create an extension that exposes a route publicly (no user context).
Before I could set meta.auth: false and everything would work, but this configuration is no longer honoured.

The auth logic (auth guard) eventually enters in the user context 'if' (isUserContext === true because contextRoute === null), where it checks that isUserContextReady === false, thus redirecting the user back to login.

@diocas diocas added Type:Bug Something isn't working Early-Adopter:CERN labels Oct 25, 2022
@diocas
Copy link
Contributor Author

diocas commented Oct 25, 2022

I tried replacing (from router/helpers.ts > isUserContext)

  const contextRoute = getContextRoute(router, to)
  return !contextRoute || contextRoute.meta?.auth !== false

with

  const contextRoute = getContextRoute(router, to)
  if (contextRoute) {
    return contextRoute.meta?.auth !== false
  }
  return false

Which prevents the login (does next()), but then something gets stuck somewhere else...

@diocas
Copy link
Contributor Author

diocas commented Oct 25, 2022

Think I found it.. I can make it simpler.. Will run the tests and do a PR.

@kulmann
Copy link
Member

kulmann commented Oct 25, 2022

There was this PR #7437 approx. 2.5 months ago, where we changed the behaviour of the authGuard to interpret the absence of a contextRoute (and not having meta.auth on the target route) as requiring a login. This still makes sense to me. Defining meta.auth: false on the route itself always takes precedence over context route handling. That being said I don't understand the exact details of how you reach the faulty state. Can you disclose more details about your use case?

@diocas
Copy link
Contributor Author

diocas commented Oct 25, 2022

The problem is that the target route auth is not absent and I'm still redirected to login if I'm not logged in.
I would expect non oC extensions (especially if not opened via files app), to not have the contextRoute and still be able to expose public routes.

I just created an extension that sets auth: false and observe this. This extension, in particular, is to redirect old CERNBox URLs to the new ones, including the public links. This could be handled on our nginx or on the gateway (like upstream oCIS does), but this way I kept all logic in a single place. Users should be able to reach [appId]/s/:token (with appId = index.php), which was supposed to be available worldwide but is not.
When we pushed this to all CERN on Monday without realising the issue, we had to create a workaround.

You can see here the definition:
https://github.com/cernbox/web-extensions/blob/main/old-web-redirector/src/index.js#L14

@kulmann
Copy link
Member

kulmann commented Oct 26, 2022

The problem is that the target route auth is not absent and I'm still redirected to login if I'm not logged in. I would expect non oC extensions (especially if not opened via files app), to not have the contextRoute and still be able to expose public routes.

I just created an extension that sets auth: false and observe this. This extension, in particular, is to redirect old CERNBox URLs to the new ones, including the public links. This could be handled on our nginx or on the gateway (like upstream oCIS does), but this way I kept all logic in a single place. Users should be able to reach [appId]/s/:token (with appId = index.php), which was supposed to be available worldwide but is not. When we pushed this to all CERN on Monday without realising the issue, we had to create a workaround.

You can see here the definition: https://github.com/cernbox/web-extensions/blob/main/old-web-redirector/src/index.js#L14

Ah, thank you, now I understand the issue.

So with the authentication changes some months ago we now define three different contexts:

  • public (really, really public, no context required whatsoever)
  • public link (requires a round trip to the resolvePublicLink page to resolve the public link context, i.e. load public link data, show password prompt if needed, etc)
  • user (requires a round trip to the login page to resolve the user context, i.e. login the user at the IdP, load user info like id, name, quota, etc)

What you try to achieve is public, but that's not configurable at the moment. Because until now no one needed it and we struggled with proper naming. Workaround for you would be to add your route name here:

const publicRouteNames = [

What we could easily do, is to agree on a new route meta param like anonymous to indicate that the route doesn't need a context, and then evaluate route.meta.anonymous in the isAuthenticationRequired helper that I linked above. I can make if pull request if you want to use that.

For the meta.auth flag we honestly would like to deprecate that (and then remove it when releasing a 7.0.0 some time in the future), because the naming is confusing. See this ticket: #7234 The flag is not about whether or not an authentication is required, but it only indicates that a user context is required. Since the public context is defined via the hardcoded array of route names linked above, the only other option is public link context, so that explains why you didn't reach what you wanted to achieve by setting auth: false.

@kulmann
Copy link
Member

kulmann commented Oct 26, 2022

@diocas could you have a look at #7875 ? setting meta.authContext to anonymous should solve your issue.

@diocas
Copy link
Contributor Author

diocas commented Oct 27, 2022

What you did is interesting, and it solves in theory our issue...

But, this looks very inconsistent and would not be ok if I was developing a proper app (assuming we want a single route for both with/without auth context, which maybe you think this should always be separate...).
If I set it to anonymous, I see the following:

  1. If I have no auth context, I see a dark grey screen with my app view (so, no header, including buttons to switch to light view, no branding, etc)
  2. If I have context, mounted() is called, but I'm in the generic blue screen (so, my view is not displayed, no header, branding, etc).

If I set it to hybrid, I would expect it to show me the user context if logged in, or the normal app frame if not. But I suppose that is not what hybrid means, with no context I'm sent to login, with context everything shows well.

What was there before (and what my PR did), what to show the view of my application, always within the runtime frame (meaning, the header, branding, etc). And this is already prepared to show me the username on the top right corner if there's a user context or hide it otherwise.

So, look, I think what I just described would be a better way of presenting (we should show the app frame). But I do not consider this important for now, so I would leave it into your consideration. We can expose our route logic without user context, so this would already unblock us.

@kulmann
Copy link
Member

kulmann commented Oct 28, 2022

Thank you for looking into it!

But, this looks very inconsistent and would not be ok if I was developing a proper app (assuming we want a single route for both with/without auth context, which maybe you think this should always be separate...).

A single route with / without auth context is not supported with this. That's a different topic - in the future we want to query the IdP to check if there is an active user session, and if yes restore it. That will open up the opportunity of defining anonymous routes that can make use of the "maybe-existing-user".

As of now, you can only ever have a user with an authContext choice of user or hybrid (or if you don't set one, which defaults to user). Or kind of "accidentally", if you open a new URL in the current browser tab and are already logged in with a user.

If I set it to anonymous, I see the following:

  1. If I have no auth context, I see a dark grey screen with my app view (so, no header, including buttons to switch to light view, no branding, etc)
  2. If I have context, mounted() is called, but I'm in the generic blue screen (so, my view is not displayed, no header, branding, etc).

In my opinion that's something else: the page layout. The ownCloud Web runtime as of now supports three different page layouts: plain, loading and application. We hardcoded the chosen page layouts, depending on the required authContext and its resolved state. Examples:

  • an anonymous route like oidc-callback or access-denied would choose the plain layout, resulting in a, well, plain page that would just render whatever you have defined in the component for the route.
  • a user route like e.g. shares/with-me of the files app would render the loading layout as long as the required user context was not resolved, and would switch over to the application layout as soon as the user context was resolved. The application layout includes the left sidebar and the topbar and shows the route component in the remaining space, with rounded corners.

Please note that the plain layout doesn't give you some nice content area, you're responsible for that yourself. We often do this with the oc-card or oc-login-card classes from the design system. You can have a look at the (updated, in this PR) pages in packages/web-runtime/src/pages for examples. Most of them are rendered in a plain layout.

If I set it to hybrid, I would expect it to show me the user context if logged in, or the normal app frame if not. But I suppose that is not what hybrid means, with no context I'm sent to login, with context everything shows well.

hybrid can be either publicLink or user, so it always requires a resolved context. Please note that the context can always be the target route itself or the route that is set in the query param contextRouteName. We make use of that when navigating from the files app into any viewer app. The one existing route of the preview app has authContext: "hybrid", because we derive the required context from the meta.authContext of the route specified by contextRouteName.

What was there before (and what my PR did), what to show the view of my application, always within the runtime frame (meaning, the header, branding, etc). And this is already prepared to show me the username on the top right corner if there's a user context or hide it otherwise.

So, look, I think what I just described would be a better way of presenting (we should show the app frame). But I do not consider this important for now, so I would leave it into your consideration. We can expose our route logic without user context, so this would already unblock us.

Could you please try if you can achieve what you want by setting authContext: "anonymous" and pageLayout: "application" with the most recent state of the linked pull request?

@diocas
Copy link
Contributor Author

diocas commented Nov 1, 2022

Seems great! Thanks for the effort, I was thinking this would be a very small thing but it ended up not the case.
As I said, for our use case, the first iteration was already good, but I think this new option makes things even better for devs. Thanks for that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Early-Adopter:CERN Type:Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants