refactor(backend): context hook

[roschaefer]

Motivation

This fixes #1503.

This is necessary before I can properly implement role based authorization (for the admins).

How to test

1. CI should be green

[roschaefer]

This is something I don't understand: Why was the updatedUser not returned?

[Mogge]

When I wrote this code I was confused as well. Check if the false positive is workuing

[Mogge]

Nice refactor. Much better structured, and I like the context function. Please check for the false positive.
How do you want to realize the admin authorization? So far the single source of truth is authentik and therefore the token. When you touch the authChecker function, the token is already `out of scope.

[Mogge]

This is a false positive as introduction and availability are already null.

[roschaefer]

I will have a look into this, but is this a blocker for this PR?

[roschaefer]

@Mogge it's addressed and certainly not a false negative anymore - I consider a failing test as positive (like a HIV-test) and a false positive a false alert. A false negative for me would be a test that does not fail but a bug is present.

[Mogge]

Did you change something? There is only one force pushed commit and I cannot see the changes done by the last commit. Was I wrong? Now the code looks fine

[Mogge]

When I wrote this code I was confused as well. Check if the false positive is workuing

[Mogge]

It would be nice to avoid the

    const { user } = context
    if (!user) throw new Error('User not found!')

construction in all resolvers that require a user. Can we somehow force the type UserWithProfile for calls that require authorization?

[roschaefer]

Yes, I will also have a look but this is out of scope of this PR, isn't it?

Maybe there is a typescript solution for this. Ideally Authorized() would mark the context.user as not null.

[roschaefer]

@Mogge I think we're out of luck here: microsoft/TypeScript#49229

I would suggest we use a simple helper method:

const { user } = requireUser(context)

with:

const requireUser = (context: Context) => {
  const { user } = context
  if (!user) throw new Error('context.user must be present after authorization!')
  return context
}

[Mogge]

This would not raise the coverage of our branches

[Mogge]

Could we have a dummy unauthenticated user that is set if there is no authentication?

[roschaefer]

Nice refactor. Much better structured, and I like the context function. Please check for the false positive. How do you want to realize the admin authorization? So far the single source of truth is authentik and therefore the token. When you touch the authChecker function, the token is already `out of scope.

Role based-authorization can be done with this: https://github.com/dreammall-earth/dreammall.earth/pull/1686/files#diff-ab0ffeecd111988f9570b4645447f2cf50e1aecc99eef4cc4d45fbfbe3314dcbR65 and we would create an admin group with { roles: ['admin'] } attributes. Every member of this group will have the deep-merged roles in the JWT payload. Authentik has a concept of "roles" as well, but I could not find a documented way to encode that in the payload. I guess, group attributes are the way to go.

Also, I vote to decode the JWT in the backend and return the currentUser.roles to the frontend without a database call if not necessary. We can store the roles in the database, too, why not. But I would like to avoid a findOrCreateBy database call in the context hook unless we need to update/save the currentUser in our database.

[Mogge]

We can merge this for now.

[ulfgebhardt]

@roschaefer pls do your thing