Factorized the code for allow_access and allow_publish #101

Telokis · 2019-11-19T11:45:10Z

Closes #100

This PR aims to fix the security issues the current allow_access implementation has.
Anybody can access any package as long as he is authenticated.
Instead of just fixing this issue, the new branch completely merges the behavior of allow_access and allow_publish by introducing a generic allow_action function that is used by both of them.
The new behavior allows users to specify $authenticated and $all/$anonymous for both publish and access.
It also introduces a new meta-group: $owned-group (This name could change before the release). This meta-group represents a permission where the user can only perform the action on a package where he has publishLevel permission in Gitlab. This is the curent behavior for publish.

- allow_access - allow_publish

Telokis · 2019-12-03T13:37:42Z

I updated the tests and the README (I took the liberty to incorporate and adapt the changes suggested by @slhck in #104 ).

bufferoverflow

Sorry for the delay, I left a few comments.

bufferoverflow · 2019-12-08T11:18:11Z

README.md

@@ -52,13 +52,13 @@ packages:
  '@*/*':
    # scoped packages
    access: $all
-    publish: $authenticated
+    publish: $owned-group


This should e.g. be $maintainer as described here https://github.com/bufferoverflow/verdaccio-gitlab#configuration-options.

I thought about doing it this way but the issue is that I would need to retrieve all user's projets/groups. The current way is to retrieve only the groups/projets where the user is at least auth.gitlab.publish meaning that I would have to change that as well.

Originally, when working on this PR, I decided to use $owned-group as a way to represent the current behavior that the plugin is using for publish authorization.
The naming may not be ideal (though I have nothing better in mind, yet) but it's there for the user to have a way to say "Only do this action if you have at least auth.gitlab.publish permission on the associated group/project".

I think adding support for $maintainer, $owner, ... beyond the auth.gitlab.publish should be another PR with a discussion regarding the design itself.

I'm not sure you remember my other issue but I made this PR because I plan to do #99 right after it's merged.

@bufferoverflow What should I do regarding this comment?

$maintainer is the current default as figured out here, so we do not need $owned-group

https://github.com/bufferoverflow/verdaccio-gitlab/blob/master/src/authcache.ts#L50 does not have the access level, so if we add functionality for different access levels, people should be able to define the level of their choice for access and publish.

Yes, for now they will both use the publishLevel value without distinction.
The proper way would be to be able to specify $maintainer, $owner and so on for both publish and access but this requires much more work. (as mentioned here: #101 (comment))

Do you have any other feedback or is this PR ok as-is?

as said, if we add functionality for different access levels, people should be able to define the level of their choice for access and publish.

Yes but this is not the goal of this PR. Its only goal is to increase security.

bufferoverflow · 2019-12-08T11:19:15Z

README.md

+condition at all.
+- `$authenticated`: Any successfully authenticated user can perform the action
+on the packages.
+- `$owned-group`: A user can perform the action on a package if


We need to take care on the access level and I think there is no need for $owned-group as we have access level for this.

See my answer above, I would need to alter the way the extension retrieve the user's data.

bufferoverflow · 2019-12-08T11:20:42Z

package.json

@@ -6,7 +6,7 @@
  },
  "scripts": {
    "type-check": "tsc --noEmit",
-    "commitmsg": "commitlint -e $GIT_PARAMS",
+    "commitmsg": "commitlint -q -e $GIT_PARAMS",


Why is this needed?

I kept getting a formatted.join when executing commitlint commands. Absolutely no idea why and I didn't feel like investigating the library.

But I agree that I shouldn't quiet it. It totally disables the error output which makes fixinx lint errors annoying.

Regarding this, I also noticed a deprecation warning:

$ yarn run commitmsg yarn run v1.19.0 $ commitlint -e $GIT_PARAMS Using environment variable syntax ($GIT_PARAMS) in -e |--edit is deprecated. Use '{-E|--env} HUSKY_GIT_PARAMS instead' C:\Users\Telokis\Documents\git\verdaccio-gitlab\node_modules\@commitlint\cli\lib\cli.js:119 throw err; ^ Error: Received $GIT_PARAMS as value for -e | --edit, but GIT_PARAMS or HUSKY_GIT_PARAMS are not available globally. at getEditValue (C:\Users\Telokis\Documents\git\verdaccio-gitlab\node_modules\@commitlint\cli\lib\cli.js:307:9) at normalizeFlags (C:\Users\Telokis\Documents\git\verdaccio-gitlab\node_modules\@commitlint\cli\lib\cli.js:268:15) at C:\Users\Telokis\Documents\git\verdaccio-gitlab\node_modules\@commitlint\cli\lib\cli.js:126:11 at new Promise (<anonymous>) at main (C:\Users\Telokis\Documents\git\verdaccio-gitlab\node_modules\@commitlint\cli\lib\cli.js:123:9) at Object.<anonymous> (C:\Users\Telokis\Documents\git\verdaccio-gitlab\node_modules\@commitlint\cli\lib\cli.js:115:1) at Module._compile (internal/modules/cjs/loader.js:759:30) at Object.Module._extensions..js (internal/modules/cjs/loader.js:770:10) at Module.load (internal/modules/cjs/loader.js:628:32) at Function.Module._load (internal/modules/cjs/loader.js:555:12) error Command failed with exit code 1. info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

I can easily fix it by removing $GIT_PARAMS from the arguments.

I pushed this fix and reverted the -q

bufferoverflow · 2019-12-08T11:21:07Z

src/gitlab.ts

@@ -37,6 +37,7 @@ const BUILTIN_ACCESS_LEVEL_ANONYMOUS = ['$anonymous', '$all'];

 // Level to apply on 'allow_access' calls when a package definition does not define one
 const DEFAULT_ALLOW_ACCESS_LEVEL = ['$all'];
+const DEFAULT_ALLOW_PUBLISH_LEVEL = ['$owned-group'];


should be $owner

If I were to do it the way you suggest, I think the default value should be $maintainer since it currently is the default value in the code.

verdaccio-gitlab/src/gitlab.ts

Line 68 in 2fa6944

this.publishLevel = '$maintainer';

yep, this makes sense

Telokis · 2019-12-16T09:29:52Z

By the way, I didn't mention it but I think this should be a major release, it will break all existing configurations relying on $authenticated.

bufferoverflow · 2020-03-10T21:24:15Z

as said, if we add functionality for different access levels, people should be able to define the level of their choice for access and publish.

#101 (comment)

Telokis force-pushed the security-patch branch from 5b817bf to df18247 Compare December 3, 2019 10:12

feat: factorized the code for

4e77835

- allow_access - allow_publish

Telokis force-pushed the security-patch branch from df18247 to 4e77835 Compare December 3, 2019 10:35

Telokis added 5 commits December 3, 2019 11:36

fix: commitlint not working on fresh install

b9b972e

test: updated tests for new system

be0a733

fix: bad replace when copy/pasting

8f2bdcb

fix: fixed lint error with refacto

b280bd3

docs: improved documentation to describe new permissions

59c8c2a

Telokis and others added 2 commits December 3, 2019 14:38

Merge branch 'master' into security-patch

5247bc6

docs: fix lint errors

52db88f

bufferoverflow requested changes Dec 8, 2019

View reviewed changes

fix: removed deprecated GIT_PARAMS

9dbd505

Telokis requested a review from bufferoverflow December 12, 2019 09:43

bufferoverflow closed this Mar 10, 2020

bufferoverflow mentioned this pull request Mar 10, 2020

Security concerns #100

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Factorized the code for allow_access and allow_publish #101

Factorized the code for allow_access and allow_publish #101

Telokis commented Nov 19, 2019

Telokis commented Dec 3, 2019

bufferoverflow left a comment

bufferoverflow Dec 8, 2019

Telokis Dec 8, 2019

Telokis Dec 10, 2019

Telokis Dec 26, 2019

bufferoverflow Dec 26, 2019

bufferoverflow Jan 8, 2020 •

edited

Loading

Telokis Jan 8, 2020

Telokis Jan 9, 2020

bufferoverflow Jan 13, 2020

Telokis Jan 13, 2020

bufferoverflow Dec 8, 2019

Telokis Dec 8, 2019

bufferoverflow Dec 8, 2019

Telokis Dec 8, 2019

Telokis Dec 9, 2019

Telokis Dec 10, 2019

Telokis Dec 10, 2019

bufferoverflow Dec 8, 2019

Telokis Dec 8, 2019 •

edited

Loading

bufferoverflow Dec 18, 2019

Telokis commented Dec 16, 2019

bufferoverflow commented Mar 10, 2020

Factorized the code for allow_access and allow_publish #101

Factorized the code for allow_access and allow_publish #101

Conversation

Telokis commented Nov 19, 2019

Telokis commented Dec 3, 2019

bufferoverflow left a 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

bufferoverflow Jan 8, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Telokis Dec 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Telokis commented Dec 16, 2019

bufferoverflow commented Mar 10, 2020

bufferoverflow Jan 8, 2020 •

edited

Loading

Telokis Dec 8, 2019 •

edited

Loading