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

Auth explanation request #1483

Closed
wants to merge 1 commit into from
Closed

Conversation

Nils-Schiwek
Copy link

@Nils-Schiwek Nils-Schiwek commented Nov 29, 2024

This is unfinished text, so please do not merge.

When migrating to CDSv8 (from 7) we did have a few issues, especially on middleware related to the user.

Please update your documentation, it is missing these points currently afaik.

We cannot (or should not) use req.user any more. Instead we use cds.context.user in the middleware now.
We need some explanation on the different users here. Today I had to debug and identify an anonymous user. This is not documented yet, so please elaborate here or somewhere else.

@sjvans
Copy link
Contributor

sjvans commented Dec 3, 2024

hi @Nils-Schiwek

thanks! some references where really outdated + i tried to clarify in #1493 does that make it clearer?

best,
sebastian

@Nils-Schiwek
Copy link
Author

Thanks @sjvans this looks a lot better! I just miss some pointers on the cds.context.user.id - afaik it can have the static ids 'anonymous' and 'system' (maybe more). In the past our middleware tried to check if the user object (or id) was empty, but I noticed this has changed, as since the update to cds v8 I get cds.context.user.id = 'anonymous' when before I got empty user object. Is this observation correct?
Could you please add some pointers to the documentation describing the described "named" users 'anonymous' and 'system'?

@sjvans
Copy link
Contributor

sjvans commented Dec 5, 2024

hi @Nils-Schiwek

the anonymous and privileged users should be checked via instanceof cds.User.Anonymous/Privileged. the matching strings are not sufficient.

system/ technical users, on the other hand, are identified via a pseudo role and should be checked via cds.context.user.is('system-user').

best,
sebastian

@sjvans
Copy link
Contributor

sjvans commented Dec 5, 2024

for a privileged user, the is(<role>) check always returns true.

btw, why do you need to know if it is the anonymous user? non-anonymous users have pseudo role authenticated-user, so user.is('authenticated-user') should be sufficient.

@Nils-Schiwek
Copy link
Author

I tried to use cds.User.Anonymous in a test today, but I am having trouble.

In the middleware I check if user is anonymous with this code:

    if (!cds.context?.user.is('authenticated-user')) {
      next();
      return;
    }

In the test I would expect to use something like

    it('handles request with anonymous user correctly', async () => {
      cds.context = { user: new cds.User('anonymous') } as EventContext;
      
      const nextMock = jest.fn();

      await initUser({} as XssecExpressRequest, {}, nextMock);

      expect(cds.context.user.id).toBe('anonymous');
      expect(nextMock).toHaveBeenCalledWith();
    });

It would be more convenient to use cds.User.Anonymous directly, but I was not able to.
How do you expect us to mock a cds user? Or do I need to mockuser.is()?

@sjvans
Copy link
Contributor

sjvans commented Dec 9, 2024

I tried to use cds.User.Anonymous in a test today, but I am having trouble.

In the middleware I check if user is anonymous with this code:

    if (!cds.context?.user.is('authenticated-user')) {
      next();
      return;
    }

In the test I would expect to use something like

    it('handles request with anonymous user correctly', async () => {
      cds.context = { user: new cds.User('anonymous') } as EventContext;
      
      const nextMock = jest.fn();

      await initUser({} as XssecExpressRequest, {}, nextMock);

      expect(cds.context.user.id).toBe('anonymous');
      expect(nextMock).toHaveBeenCalledWith();
    });

It would be more convenient to use cds.User.Anonymous directly, but I was not able to. How do you expect us to mock a cds user? Or do I need to mockuser.is()?

new cds.User('anonymous') creates a user that has the id anonymous but is an authenticated user nevertheless. the user's id is an opaque string to us, i.e., it carries no meaning. for a real anonymous user, do user: new cds.User.Anonymous() or user: cds.User.anonymous (the ready-to-use sealed instance).

@Nils-Schiwek
Copy link
Author

new cds.User('anonymous') creates a user that has the id anonymous but is an authenticated user nevertheless. the user's id is an opaque string to us, i.e., it carries no meaning. for a real anonymous user, do user: new cds.User.Anonymous() or user: cds.User.anonymous (the ready-to-use sealed instance).

@sjvans is this a new feature? We are on cds 8.5.0 and I get Property 'Anonymous' does not exist on type 'typeof User'
It does not seem like the two samples you described above are available in typescript.

@sjvans
Copy link
Contributor

sjvans commented Dec 10, 2024

@sjvans is this a new feature? We are on cds 8.5.0 and I get Property 'Anonymous' does not exist on type 'typeof User' It does not seem like the two samples you described above are available in typescript.

no. that's been there a long time. the sealed instances were added in v6.0.0, the classes are even a lot older.

cf. node repl:

Welcome to Node.js v22.8.0.
Type ".help" for more information.
> const cds = require('@sap/cds')
undefined
> cds.User
[Function: exports] {
  default: [Getter/Setter],
  anonymous: Anonymous {},
  Anonymous: [class Anonymous extends User],
  privileged: Privileged {},
  Privileged: [class Privileged extends User],
  _default: Anonymous {}
}
> cds.User.Anonymous
[class Anonymous extends User]
> cds.User.anonymous
Anonymous {}

@daogrady could you please assist re typescript?

@daogrady
Copy link
Contributor

Gladly! Feel free to just send me a meeting request so we can work this out.

@Nils-Schiwek
Copy link
Author

thanks for the help @sjvans and @daogrady - this really helped me out! Closing this PR as everything important is in #1493

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants