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

Feature/package provider #207

Merged
merged 4 commits into from
Apr 11, 2015
Merged

Conversation

CrispyConductor
Copy link
Contributor

This pull request adds the ability to create authorization (ie, package provider) plugins (to control access to packages and publishing) in addition to the existing authentication plugins. This is useful if an organization already has an access control infrastructure and doesn't want to duplicate roles across systems.

A plugin using this structure that connects to gitlab is here (it's not yet published to npm, pending this pull request). It will automatically pull package data and access rights from corresponding repositories in gitlab.

This pull request contains the following changes to Sinopia, necessary to add this capability:

  • Move plugin loading code to a separate file to avoid duplicating common code for loading authentication and package provider plugins
  • Add additional code in the config parser to validate the package provider plugin specification and configuration; this is based on the config structure used for the authentication plugins
  • Create a file (packages.js) to handle providing package metadata and access control; this file mirrors the corresponding functions in config.js, except that the calls are asynchronous to allow for connecting to external services (such as in the above gitlab plugin)
  • Change code calling allow_access() and allow_publish() to be asynchronous

The changes are backwards compatible and do not break any existing configurations.

These plugins behave as follows:

If a package matches the "packages" section in the config file, and that section contains an object called "plugin", package information for that package is taken from that plugin. Configuration for the plugin is taken from the object underneath the plugin name, see the gitlab plugin for an example. If a plugin requires no configuration, it may be specified as a string instead of an object.

Multiple plugins can be specified. In this case, the first one that responds to a function with a non-undefined response is used. If no plugins are specified, or all respond with 'undefined', the values specified in the configuration file are used.

Plugins are constructed and instantiated in the same way, and with the same parameters, as auth plugins.

@crowelch
Copy link

This looks cool. Our organization uses gitlab and it would be nice to have sinopia integrate permissions with it so we don't have to maintain duplicate sets of access controls.

@drudge
Copy link

drudge commented Mar 11, 2015

+1

1 similar comment
@Flickdm
Copy link

Flickdm commented Mar 17, 2015

+1

@CrispyConductor
Copy link
Contributor Author

Bump. Any word on this?

@rlidwka
Copy link
Owner

rlidwka commented Mar 28, 2015

Sorry for the delay, I've been extremely busy this month.

Can the existing auth plugins be changed in a way to support this feature? Like add allow_access and allow_publish there? Current auth plugins manage both authentication and authorization (because of usergroups). So having two authorization mechanisms isn't a good idea.

Also, we might have an issue with search, because sinopia will get the list of all the packages and try to check if user has an access to each and every one of them. Any ideas how to solve that?

@CrispyConductor
Copy link
Contributor Author

I considered adding the functionality to auth plugins, but this could be messy and require significant changes to how auth plugins are loaded and configured, or would limit flexibility. Auth takes places when the connection is established, so it doesn't make sense to have multiple auth plugins dependent on package name. Authorization/package-provider features, however, can apply on a per-package basis. In my own use case (and probably others), I'm using different authorization settings for different package name prefixes.

I don't see the current search behavior being an issue. It's up to the package provider plugin to ensure that authorization is sufficiently fast or cached.

Conflicts:
	lib/auth.js
	lib/index.js
	lib/middleware.js
@CrispyConductor
Copy link
Contributor Author

Note: Merged in upstream latest and resolved conflicts.

@rlidwka
Copy link
Owner

rlidwka commented Apr 8, 2015

I'm trying to pull this in with a few fixes, check this branch:
https://github.com/rlidwka/sinopia/tree/package-provider

Basically, only major change is: plugin is now called for every package, so it's its responsibility to know if it should handle this particular package or not.

For this purpose, second argument is not a package name now, but a package object that contains name and all the config stuff.

Necessary changes in gitlab plugin (didn't test it):
rlidwka/sinopia-gitlab@caee052

A bit of a demo how to write those (using it for testing now):
https://github.com/rlidwka/sinopia-playground

So if everything looks good to you, I'll be glad to finally merge and release it.

@CrispyConductor
Copy link
Contributor Author

The changes look good. If you'd like, I'll test your branch together with the modified gitlab plugin tomorrow, and let you know how it goes. Thanks!

@CrispyConductor
Copy link
Contributor Author

Tested (both sinopia, and your changes to sinopia-gitlab), and verified working. If you don't mind, please submit a pull request to sinopia-gitlab after this is merged into sinopia; I'll update the readme and push it to npm. Thanks!

@rlidwka rlidwka merged commit 6954898 into rlidwka:master Apr 11, 2015
rlidwka added a commit to rlidwka/sinopia-gitlab that referenced this pull request Apr 11, 2015
rlidwka added a commit to rlidwka/sinopia-gitlab that referenced this pull request Apr 11, 2015
@rlidwka
Copy link
Owner

rlidwka commented Apr 11, 2015

Merged and published as sinopia@1.2.0. I sent those changes in a pull request as you requested.

Thanks!

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

Successfully merging this pull request may close these issues.

5 participants