Skip to content

Conversation

@berkes
Copy link
Contributor

@berkes berkes commented Dec 23, 2025

This is an attempt to allow for dropping the user into the login flow automatlically.

The strategy taken is to build on top of what exists, changing as little as possible. This has severe downsides, and some upsides.

This PR explores two alternative solutions:

url arguments on login

Up to commit 49d6b56

/login?redirectPath=%2Fdetails%2Fq5Of2C1RSX-s3DBCmvvwGg%3Ffoo%3Dbar&role=student.

We can present this url in Emails and other channels.

  • Con: Already logged in users will have to log in again. This could be solved by adding a check if a user is logged in and then redirecting rather than showing the page. But this will add more logic there as we also need to handle the various force=true and validateName=true scenarios.
  • Con: Technical Debt: The existing method of storing the redirectPath on authn/z errors, remains, and another one is added. We now have two ways to direct user to a path after login. Both with implicit UX differences: one will auto-redirect to auth flow, the other will present the login page first.
  • Pro: we can control the login behaviour from the place where we present the link.

magic determiniation of role based on the path wanted to visit.

Commit a0a485d replaces previous alternative with this alternative.

When visiting a page that we don't have access to, we look up the requested path in a table that maps the path to the required role.
We can now send the user into the login flow for this role with force=true, forcing a re-login.

  • Con: Less control for us, esp in edge-cases where e.g. a user has both staff and student account mixed - most common in test and demo scenarios.
  • Con: Duplicated storage of route->role mapping. In App.svelte implicit in various if/else blocks, and then in a util mapping. If routes are moved, changed, introduced we'll have to keep the places in sync.
  • Pro: Enhances existing method instead of introducing a new one next to it.
  • Pro: UX is cleaner, as the canonical URL can be bookmarked or fetched from browser history and re-used.
  • Pro: Handles already-logged in and not-logged-in users more predictable.

Generic pros and cons over a Component Based (see below) Alternative:

  • Pro: Keeps close to what we already have. Minimizing amount of changes.
  • Pro: Attempts to keep existing implicit business logic in place as much as possible.
  • Con: Adds more technical debt: depending on the alternative, but both add their own debt.
  • Con: More indirection: code in places where it ideally shouldn't be: i.e. technical debt in the form of hard to follow and hard to read logic flows.

Alternative strategy:

I have researched some alternatives to above. These could be considered "The Proper Svelte Way" or such. From this, below strategy is IMO cleanest and simplest.

  • <AuthenticationProvider> component wrapping the <Router> - This would check that there is a) a user and that this user has b) a role - basic authentication.
  • <ProtectedRoutes requiredRole={{role.STUDENT}}> component that mounts underlying routes only if requiredRole == currentRole - basic authorization.
  • Pro: Separation of concerns:
    • Authentication - provider
    • Authorizatio through conditional Route Mounting in ProtectedRoutes.
    • App simply deals with mounting the app, not authn/authz
  • Pro: Allow different messages to user: Not allowed to see because not logged in, vs Not allowed to see because wrong role.
  • Pro: Allows fixing a minor security issue where if now a student changes their "role" to "teacher" in their clients' localstorage, they see the interface and menu-items as if they are a teacher. They cannot interact with anything though, since the server authorizes as well. But being able to access the UI items and pages could be considered a minor issue.
  • Con: Requires a large refactoring of the structure of the app so:
    • Lots of work
    • Requires intensive testing
    • Many opportunities for existing bugs that are currently suppressed to then surface
    • Many oppotunities for business logic mistakes that are now encoded in implicit logic to be missed or re-implemented wrong.

I've weighed pros and cons and decided that such a large change is not a good option to fix a seemingly small feature.

Refactor the redirect handling logic so that we can debug it.
By moving some parts to a central place and re-use some logic,
we can determine that the Login router was re-setting the $redirectPath
to "/login" on hitting the login page.

We've now fixed this. And on return are redirected to the page that we
got the access denied on.

remaining issue: If a user hits a path only accessible to **students**,
logs in with a **staff** role, the redirect will land them on a page
that they have no access to, which then triggers the login logic. Same
with staff path and login as user.

This can, and most likely will, put the user in an endless redirect
loop! This issue existed in the previous logic. But might not have
surfaced because the redirects didn't work properly. The chances of this
triggering is small though: it will only exist with users that both have
a backpack as student and a staff account as staff, and then only when
they mix up logins.
Made url parameter handling consistent. Named methods for easier
reading.
By forcefully setting the role and link
This removes previously introduced url arguments. but introduces a
duplication of what routes are available to current role.
@berkes
Copy link
Contributor Author

berkes commented Dec 23, 2025

@Iso5786 Can you also answer the following?

  • What alternative do you prefer: url arguments or magic determining?
  • Do you agree with my idea that implementing this "correct" is not worth it? Or do you want me to implement a proper authn/authz architecture using Svelte Common Best Practices for this?

@berkes
Copy link
Contributor Author

berkes commented Dec 23, 2025

@Iso5786 Once we have the strategy in place, I'll make this also a PR against develop. I'll have to backport some minor changes, so I'll rather do them once only, here. And then present the result for "develop".

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

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.

2 participants