FLPATH-809: get the user via Identity API #47

mareklibra · 2023-12-18T15:00:15Z

With this patch, the username is not passed from FE to BE anymore but retrieved via Identity API as the logged-in user.

Permissions to BE endpoints are added, policies are left up to configuration of the auth plugin or the rbac in case of the Janus.

TODO:

Add service-to-service support so external systems can create a message
- as a workaround, introduce shared secret (per upstream discussion on Discord)
update README file for the configuration needed

plugins/notifications-backend/README.md

plugins/notifications-backend/src/openapi.yaml

plugins/notifications-backend/src/permissions.ts

plugins/notifications-backend/src/service/router.ts

ydayagi · 2023-12-20T09:03:00Z

plugins/notifications-backend/src/service/types.ts

@@ -54,7 +48,7 @@ export const NotificationsOrderByDirections: string[] = ['asc', 'desc'];
 */
 export const MessageScopes = ['all', 'user', 'system'];

-export const DefaultUser = 'default/guest';
+export const DefaultServiceUser = 'default/externalServiceNotificationsUser';


we create such a user? if yes then why? we can stay with the guest user.

The original use-case for DefaultUser is gone, we do not support it. The identity of the user must be provided from the Backstage API, no matter it internally falls-back to the default/user. We newly throw exception if the user identity is missing.

The idea behind DefaultServiceUser is to differentiate an external caller from a regular user.
We are not there yet, but I am considering cases where

the external caller can retrieve messages of any user (if authorized)

we store info about creator of the particular message

the cases you describe are relevant for internal users as well. E.g. admin

let's not prepare too much for the future. if we'll need to differentiate something then we'll do the coding. so far we have just 'user' and 'default'. service is very vague.

I still do not have a solution for the external caller flow.
I see a lot of people were struggling with this task during the past half a year, a lot of questions/issues raised, no relevant response provided.

Let's wait with this topic till we have complete vision how to identify/authorize the external caller.
Anyway, it would be wrong to reuse the default user for any external caller.

if you can't support external users then remove it.
regarding 'Anyway, it would be wrong to reuse the default user for any external caller.'. it is not reusing. you have just one default

We must support them. IIUC, the Orchestrator will be such an example of external services sending messages to Backstage. A workflow reaches certain step and they will send a notification. This is what I understood.

I have changed the workaround to a simple shared secret. I think it's good enough for now. The JWT token would be clean but I can not make it working. Will keep trying but I want to move forward as well.

I think that it will be ok for now to work without authentication. For Orchestrator and any other client that wants to post notifications.
But whether we support it or not is not really our problem. it is part of the backstage installation.
and we still have just one default user for any source , external or internal or whatever.

I disagree, we must support authentication. The feature recently allows sending messages to any user or their groups by simply reaching a REST endpoint.

Even the Orchestrator has raised this issue before. I do not remember them accepting the missing authentication, they just got muted. And it is a must-have from the upstream perspective.

Can you describe how the "backstage installation" would implement AA without collaboration with the plugin?
IIRC, we have been told to skip it for the reason you have mentioned. But with the experience we gained since those days, I do not see it as a correct solution.

plugins/notifications-backend/src/permissions.ts

plugins/notifications-backend/README.md

plugins/notifications-backend/src/service/router.ts

ydayagi · 2023-12-20T09:20:47Z

plugins/notifications-backend/src/permissions.ts

I don't see a point in these permissions. if user is authenticated (exists in the catalog) that's enough

It does not cost us much, the code on the router.ts side is simple and it gives flexibility.
It is a recommendation by the Backstage upstream. I believe such recommendations are made with the knowledge of higher perspective.

it costs us. code to maintain. code to explain. code to review. more files to maintain/review. I don't need this headache. I am always in favor of doing the required and not 'might be required in the future'.
I don't trust their judgment. the existing consumers/clients needs at least get+count+unread or create. I can understand such a separation. but the rest makes no sense.

besides....do u have a scenario where u think that a client will be prohibited from creating but allowed to list?

Maybe it will be the case of an external caller.
Maybe a company will allow users to see messages of others.
Maybe an admin-plugin will use the NotificationsApi to list messages of other plugins.

In general, this is up to the policies which are not supposed to be implemented by the notifications plugin.

if it is a maybe and also up to someone else then let's not add it.

The policies are up to someone else. The permissions are up to us.
Consider them as "hooks" for the checks - we just mark the place in our code "Here make the check" and delegate the actual implementation of the check elsewhere.

ydayagi · 2023-12-24T11:35:36Z

plugins/notifications-common/src/permissions.ts

this is the only thing we have in the common plugin. that's not worth its own plugin. i do not understand why you are so much against a simple export

Any FE plugin performing a sort of canDo check would need to import from the BE plugin package. That's the reason for separation to a -common by other features and suggested in the docs as well.

Anyway, it will work even without it. To move forward now, the -common can be introduced later, i.e. for the upstream PR.

ydayagi · 2023-12-24T11:41:01Z

plugins/notifications-backend/src/service/router.ts

@@ -114,7 +136,7 @@ export async function createRouter(
      throw validation.errors;
    }

-    api.handleRequest(req as Request, req, res, next);
+    api.handleRequest(req as Request, req, res).catch(next);


did u encounter a scenario where this change was needed? cause it is based on an example from the docs and it worked with all exceptions so far. if there's no real reason then please revert.

With this change, we do not need to handle the catch/next multiple times inside the handler.
In other words, it makes less what-if flows inside of the handler, we can just chain promises there.

Before this patch, there was just a single promise per handler (IIRC) but with the authorization, we have +1.

ydayagi · 2023-12-24T11:43:40Z

plugins/notifications-backend/src/service/constants.ts

it is absurd to have another file just for a few lines. especially when we were ok with it so far. if we get a comment in PR to janus-idp then we'll change. keep to minimum files.

From my perspective, it's more important to match the content with the filename.

These are JavaScript constants, the types are from TypeScript. The former file is called types.ts. Do you have a suggestion how to rename the file?

PS: If a developer uses an IDE like VS Code or IDEA, the count of files does not matter. A normal project has tons of them and there are more benefits than disadvantages.

With this patch, the username is not passed from FE to BE anymore but retrieved from via Identity API as the logged-in user.

mareklibra marked this pull request as draft December 18, 2023 15:00

mareklibra force-pushed the FLPATH-809.Authentication branch 2 times, most recently from 9e773b3 to 5ff2555 Compare December 19, 2023 10:41

mareklibra marked this pull request as ready for review December 19, 2023 10:54

mareklibra force-pushed the FLPATH-809.Authentication branch from 5ff2555 to d7847c9 Compare December 19, 2023 10:56

ydayagi reviewed Dec 20, 2023

View reviewed changes

plugins/notifications-backend/src/permissions.ts Outdated Show resolved Hide resolved

ydayagi reviewed Dec 20, 2023

View reviewed changes

plugins/notifications-backend/README.md Outdated Show resolved Hide resolved

plugins/notifications-backend/src/service/router.ts Outdated Show resolved Hide resolved

ydayagi reviewed Dec 20, 2023

View reviewed changes

mareklibra force-pushed the FLPATH-809.Authentication branch 2 times, most recently from 61c8aa7 to be56d48 Compare December 22, 2023 10:11

ydayagi reviewed Dec 24, 2023

View reviewed changes

mareklibra added 7 commits January 2, 2024 14:20

FLPATH-809: get the user via Identity API

18a5ed5

With this patch, the username is not passed from FE to BE anymore but retrieved from via Identity API as the logged-in user.

chore(notifications-backend): add permissions related deps

63e78d7

chore: update README for authentication

0215862

chore: make service-to-service checks optional

5a25d61

chore: reduce notifications "read" permissions count

cc27893

chore: separate getLoggedInUser and checkPermission funcs

c3fa701

Introduce auth.ts and shared secret for external callers

0a413fd

mareklibra force-pushed the FLPATH-809.Authentication branch from be56d48 to 0a413fd Compare January 3, 2024 09:47

ydayagi approved these changes Jan 3, 2024

View reviewed changes

ydayagi merged commit 0ea9a45 into flpath560 Jan 3, 2024
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FLPATH-809: get the user via Identity API #47

FLPATH-809: get the user via Identity API #47

mareklibra commented Dec 18, 2023 •

edited

Loading

ydayagi Dec 20, 2023

mareklibra Dec 20, 2023

ydayagi Dec 21, 2023

mareklibra Dec 21, 2023

ydayagi Dec 21, 2023

mareklibra Dec 21, 2023

ydayagi Dec 25, 2023

mareklibra Jan 2, 2024

ydayagi Dec 20, 2023

mareklibra Dec 20, 2023

ydayagi Dec 21, 2023 •

edited

Loading

ydayagi Dec 21, 2023

mareklibra Dec 21, 2023

ydayagi Dec 21, 2023

mareklibra Dec 21, 2023 •

edited

Loading

ydayagi Dec 24, 2023

mareklibra Jan 2, 2024

ydayagi Dec 24, 2023

mareklibra Jan 2, 2024

ydayagi Dec 24, 2023

mareklibra Jan 2, 2024

FLPATH-809: get the user via Identity API #47

FLPATH-809: get the user via Identity API #47

Conversation

mareklibra commented Dec 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ydayagi Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mareklibra Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mareklibra commented Dec 18, 2023 •

edited

Loading

ydayagi Dec 21, 2023 •

edited

Loading

mareklibra Dec 21, 2023 •

edited

Loading