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.

Sign up for GitHub

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

Jump to bottom

feat!: impersonate users #3042

Merged

sr258 merged 26 commits into master from feat/permission-system

Jul 15, 2023

Member

sr258 commented Jul 9, 2023 •

edited

Loading

This PR implements the functionality to display other users' user states and to set the user state to read only.

The old permission system was replaced to simplify permission handling.: It had 'grown naturally' and needed an overhaul.

Issues with the old system were:

The IUser interface included some permissions (e.g. if users can update libraries). The library expected these permissions to be loaded whenever the user object is passed to the library. This means that all permissions need to be loaded on authentication, even if they are never needed. This approach only allowed user-specific permissions.
The only other place where there was authorization was in the Mongo/S3 storage classes. You were able to pass in a callback function that authorized users. This means authorization was specific to the storage classes. This is not great, as the file-based storage classes didn't have authorization and would lead to redundancy if they included it. Authorization logic should also not be part of a storage driver (separation of concerns, duplication)
The authorization in the Mongo/S3 classes only allowed for lists of permissions to be returned for contentIds and users. This means that you might have to perform complex queries to get the lists, even if you never need them.
The user data system didn't have any proper authorization at all.

Now, it's the job of the manager objects to implement permission checks and not of the storage classes. That means all storage classes are now capable of working in a permission system and not just the S3 classes. There's also a single interface IPermissionSystem that implementations can pass into the H5PEditor or H5PPlayer constructors to have fine-grained control over who can do what.

sr258 added 20 commits

June 18, 2023 17:09


          feat: added contextId to user data classes

cfed169


          feat: contextId in H5PPlayer.play

3477c5f


          feat: contextId in h5p-express and docs

dac89d3


          test: added integration tests for contextId

b755cf2


          test: contextId in rest example

4daae91


          feat(mongo-s3): added contextId to user data storage

a5009b9


          refactor: permission system

52e7dcb


          refactor: permissions system

1c64df4


          test: fix tests

d8c178f


          test: fix more tests

79bd179


          test: fix more tests


          refactor!: moved permissions from IUser to IPermissionSystem

aef3fc1


          refactor: permission system

1e87a63


          refactor: fine grained permission types and REST example

0e159f1


          refactor: prettier

f2a036f


          refactor: more fine grained user data permission handling

8d278e9


          feat: impersonate users and read only states

e0a20fe


          test: fixed test for new content user data interface

b0803d3


          refactor: style improvement and docs

c6283f0


          Merge remote-tracking branch 'origin/head' into feat/permission-system

ce56180

github-advanced-security bot found potential problems

View reviewed changes

packages/h5p-rest-example-server/src/routes.ts Fixed Show fixed Hide fixed

packages/h5p-rest-example-server/src/routes.ts

+                          return res
+                              .status(500)
+                              .send(
+                                  `Error deleting content with id ${req.params.contentId}: ${error.message}`

Check warning

Code scanning / CodeQL

Reflected cross-site scripting

Cross-site scripting vulnerability due to a [user-provided value](1).

packages/h5p-rest-example-server/src/routes.ts

-                      res.send(`Content ${req.params.contentId} successfully deleted.`);
-                      res.status(200).end();
+                      res.status(200).send(
+                          `Content ${req.params.contentId} successfully deleted.`

Check warning

Code scanning / CodeQL

Reflected cross-site scripting

Cross-site scripting vulnerability due to a [user-provided value](1).

packages/h5p-rest-example-server/src/routes.ts Fixed Show fixed Hide fixed

packages/h5p-rest-example-server/src/routes.ts

+                          return res
+                              .status(500)
+                              .send(
+                                  `Error deleting content with id ${req.params.contentId}: ${error.message}`

Check warning

Code scanning / CodeQL

Exception text reinterpreted as HTML

[Exception text](1) is reinterpreted as HTML without escaping meta-characters.

sr258 added 2 commits

July 9, 2023 12:11


          feat: add locales

b2237d8


          test: fix tests

2ede8e6

coveralls commented Jul 9, 2023 •

edited

Loading

coverage: 65.208% (-0.6%) from 65.805% when pulling f88323c on feat/permission-system into 3bb30a3 on master.

sr258 added 4 commits

July 9, 2023 12:39


          docs: extended docs

ec27d77


          test: added test for impersonation


          test: added impersonation and read only state to REST example

7aa1c32


          Merge remote-tracking branch 'origin/master' into feat/permission-system

f88323c

sr258 marked this pull request as ready for review

July 15, 2023 12:29

github-advanced-security bot found potential problems

View reviewed changes

packages/h5p-rest-example-server/src/routes.ts

                                           : undefined
                               }
                           );
-                          res.send(content);
-                          res.status(200).end();
+                          res.status(200).send(content);

Check warning

Code scanning / CodeQL

Reflected cross-site scripting

Cross-site scripting vulnerability due to a [user-provided value](1).

packages/h5p-rest-example-server/src/routes.ts

-                          res.status(500).end(error.message);
+                          console.error(error);
+                          res.status(error.httpStatusCode ? error.httpStatusCode : 500).send(
+                              error.message

Check warning

Code scanning / CodeQL

Exception text reinterpreted as HTML

[Exception text](1) is reinterpreted as HTML without escaping meta-characters.

sr258 merged commit 75d7b68 into master

sr258 deleted the feat/permission-system branch

September 1, 2023 15:09

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

Labels

None yet