So, we have the following issues:
- Not all admins will want or require to manage their (CVS) account passwords with Drupal, as it needs to be hooked up with the VCS - that assumes that you have the permission to manage those accounts at all! Furthermore, it won't be necessary if you just want to use the "viewer" functionality of a backend.
- Subversion, Git and Mercurial use the same kind of user authentication systems - HTTP(s), or SSH (probably with an .ssh_keys based solution). Subversion also has an additional "svnserve" possibility, so backend specific and backend agnostic authentication methods need to co-exist side by side.
So let's make those authentications pluggable. What constitues an authentication method plugin?
- Storing the password that the user has entered in the registration form.
- And based on that, import/export functionality.
The idea is to let authentication method plugins register themselves with Version Control API (much like hook_versioncontrol_backends()) and let backend modules specify which authentication methods they support, as additional info that we get from hook_versioncontrol_backends(). The Version Control API will determine if those plugins exist, and let the admin choose which authentication methods should be enabled for each repository. On the register page, the user can then select which one of the predefined authentication methods he wants to use (HTTPs or SSH, for example), and if no method has been enabled by the admin then the user doesn't get to enter a password at all.
Authentication methods will be specified with unique string identifiers that they pass with the registration hook. More details (or a finished implementation in the first place) to follow soon.
| Comment | File | Size | Author |
|---|---|---|---|
| #36 | new-0001-minor-things-v2.patch | 1.32 KB | marvil07 |
| #36 | 223891-28-pluggable-auth-v9.patch | 41.81 KB | marvil07 |
| #35 | 223891-pluggable-auth-v8.patch | 41.77 KB | sdboyer |
| #35 | 223891-incremental-vs-34.patch | 19.77 KB | sdboyer |
| #34 | new-0001-several-fixes-v2.patch | 13.05 KB | marvil07 |
Comments
Comment #1
jpetso commentedOops, still not implemented in 5.x-2.x? Well, seems I was a little too confident. Still important, because a) the VCS account "subsystem" needs to be improved anyways, and b) account synchronization for anything else but CVS is pretty much impossible with the current system. Guess I should have outlined my design with a bit more detail (of course I forgot nearly everything since I wrote the original issue), but then again it should still not be all too difficult to just separate authentication from Drupal/VCS account associations. Input, help & everything appreciated.
Comment #2
jpetso commentedOh hey, and I totally forgot to link to my analysis of this issue (and the subsequent findings in the comments section): http://groups.drupal.org/node/8527
Comment #3
marvil07 commentedJust to point dhax review: http://groups.drupal.org/node/23464 really related and where it _is_ proposed a solution :-D
Comment #4
marvil07 commentedComment #5
marvil07 commentedrelated issue: #965890: [meta] Revisit VersioncontrolAccount and bring it up to speed
Comment #6
sdboyer commentedHa, look at this, this is pretty much exactly one of the issues I need to open in order to split up #965890: [meta] Revisit VersioncontrolAccount and bring it up to speed.
However, Jacob's original description of what we need is still focused on the old central command-and-control vcapi paradigm, which we've thoroughly established to be the wrong route.
So to be clear, we like this part:
But the rest is kinda stuck in the old, centralized command-and-control thinking than we like:
...maybe. I need to ponder on this a bit. In any case, though, this is one of the biggies we're tackling in sprint 5.
Comment #7
marvil07 commentedsdboyer also point me to User authentication in non-CVS repositories as a good reading, linking here.
Comment #8
sdboyer commentedQuick thing that lent me some clarity. Whatever we do, it should keep in mind that there are two discrete steps with auth:
The implementation details of these ought not be the concern of VCAPI (as much as possible) - we just care about checking the outcomes.
Comment #9
sdboyer commentedThis is on me to work on until tomorrow, after which it'll be marvil07's.
Comment #10
marvil07 commentedtagging
Comment #11
greg.1.anderson commentedI have had really good luck with organic groups, og subgroups, and particularly og user roles to control access to external Apache resources by way of the mod_auth_mysql module. Using these same techniques, I could easily imagine using information from the og tables to write out an authz_svn access files; this could also be relevant to other VCSs that support Apache authentication.
There are a lot of modules in the og suite; building an access control model with these would be preferable to sticking with the existing VCS API "motivation texts" mechanism, as og provides a lot of flexibility in configuration.
Comment #12
sdboyer commented@greg.1.anderson - thanks for chiming in here. If we retain the mapping table at all, it'll be as part of a plugin - in no way a required part of the system. And re: your comment #965890-13: [meta] Revisit VersioncontrolAccount and bring it up to speed, we're currently planning on getting rid of the
VersioncontrolAccountclass entirely.Comment #13
sdboyer commentedOK, this patch is VERY much in-progress, but I want to get it out there pre-review so we at least have something.
Basically, we introduce a method,
VersioncontrolRepository::getAuthHandler()that gets an auth plugin. These plugins must implement an interface which answers all possible auth-type questions. I've got one fallback, "free-for-all" plugin (which just says "yes!" to every auth check), and I'm partway through a much more complicated plugin that handles granular, per-repo/per-user auth. I'm intending that plugin to sit at the head of a family of "account" auth plugins, as there are a lot of use cases where you might want to extend and slightly tweak its logic (vc_project probably being one). There are also a bunch of schema changes pursuant to the account plugin - and if you look, you'll notice it's gonna be pretty easy for us to do per-branch ACLs! the much more challenging part will be the UI for it :)Before this goes in, I want to at LEAST write another plugin that does auth based on user roles. Chances are that for d.o, we're going to want a combo of the accounts-based and role-based approaches, where we let certain global role settings short-circuit all the db-based auth. That'll allow us to easily flip a switch that disables someone's git access, or (much less likely, if we even want it at all) gives someone access to all repos.
my local changelog for the attached patch:
Also, it's important to note that in my original version, I was expecting to pop off lots of these auth plugins, so
VersioncontrolRepository::getAuthHandler()doesn't cache them. With the architecture of the account auth plugin family the way it is, though, it's really much better if we do cache them. That's one of the first things on my list to follow up and change.Comment #14
sdboyer commentedComment #15
sdboyer commentedPS - there's clearly UI and CRUD considerations that haven't been touched on at all thus far in the account auth plugin family. CRUD is pretty easy, UI's a little more tricky. Probably going to use either some additional interfaces, or some properties on the plugin definition.
Comment #16
greg.1.anderson commentedI like the direction this is taking. It looks like it is easy to provide a simple "pass-through" VersioncontrolAuthHandlerInterface that just assumes that the drupal name == the vcs name.
Regarding the UI for per-branch access control, I recommend setting up versioncontrol_project to allow a single repository + branch to be associated with each project, just like it is today in the vc_1.x API. Each project needs a "Role to read" and a "Role to write". Really, you can use just one role for each, "vcs_read" and "vcs_write", and have all the control you need. Let me explain how; this is an expansion of what I was saying in #11.
1. If a (drupal) user has the "vcs_read" role, then they can read any branch of any available repository.
2. If a suer has the "vcs_write" role, then they can also globally write.
3. In order to provide per-branch access control, you must create a project and set the repository and branch of that project appropriately. Access control is based on projects. I would also recommend adding controls in versioncontrol_project to specify which specific role should be used for "vcs_read" and "vcs_write" for better control in the non-fine-grained, non-og case.
4. For a given project, either the project_project type must be a group node, or if it is not, the project must have its audience set to some group. Otherwise, only people with global access (those who have the "vcs_read" / "vcs_write" role).
5. The module og_user_roles can then be used to add vcs_read and vcs_write as roles that can be granted on a per-group basis. If you add these roles to all groups, then the group admin can decide who has the vcs_read and vcs_write role for their group. If a user already has the vcs_read / vcs_write role, that fact will take precedence over what group admins do.
I have already successfully done 1, 2, 4 and 5, and I'm working on temporarily adapting vc_project in the 1.x api to provide #3 without having to create vcs users for everyone. It would be possible and more Drupal-like to use a permission rather than a role for vcs_read / vcs_write, but I haven't tried that yet. (It should be pretty much the same, just somewhat more complicated SQL.) I'd like to help out here so this same model can be used in the 2.x branch, which I'd like to migrate to when it's feasible for me to do so.
I think that 1-3 is essentially exactly what you were already planning on implementing in #13 (paragraph 2), minus the code to provide a UI for setting access control for individual accounts.
I would like to make the pitch that versioncontrol_api should require og for branch-level access. Without og, you can provide global read only or global read/write access to all repositories. Fine-grain access is as easy to add as turning on og and og_user_roles.
As I mentioned before, og already has a great API for creating groups, assigning administrators to the group, letting the administrator have control over their group (without giving them control over other people's groups), and there are also a lot of og-related modules to allow you to fine-tune the way your groups behave. It would just confuse the versioncontrol_api interfaces to add a separate UI for this; plus, that's a lot of extra code you'd have to write just to replicate og functionality. I don't see any benefit to trying to make a "simpler" "og-free" access control UI.
Here's my proposal / offer to go with my pitch: if you implement role-based global access control as you describe in #13, then I'll submit a patch + documentation that converts it into fine-grain access with og and og_user_roles. I'm presuming you're going to hook up all of the VersioncontrolBranch stuff as you describe above, so that each branch is already doing role-based access checks.
Comment #17
greg.1.anderson commentedOh, one thing that #16 leaves open is, who has the right to add a repository + branch to a project? In the short term, I would do this with a Drupal permission. Anyone with this permission could potentially compromise security by making a new project whose branch is a subbranch of an existing project; this user would then gain control of all of the vcs paths below that point.
It is an interesting question to define what branches a partially-privileged user should be allowed to attach to a project. Maybe the branch should be required to be a subbranch of an existing project that the user is an og administrator of. Then you could make meta-projects that define where your section admins are allowed to create subprojects. That would work reasonably well, and wouldn't require a new complicated UI form.
Comment #18
marvil07 commentedJust to be consistent with user_mapping_methods it would be better to use user_auth_methods.
IMO this is not the right place to remove {versioncontrol_accounts} table, since it completely breaks versioncontrol_account_status module.
Maybe is better to deal with #983926: Remove account class before follow working here since that seems to me like the right place to remove it.
Well, I need to take a more closely look to this.
Powered by Dreditor.
Comment #19
sdboyer commentedI don't actually like user_auth_methods, but I wasn't going to nitpick :P More importantly, though, I don't think it's quite as accurate to call it an "auth method" - it's an entire handler that does potentially quite a lot, including furnishing some UI, etc.
Comment #20
sdboyer commented@greg.1.anderson #16
re: requiring og - so, my first reaction is a kneejerk hell no :) As I read the rest, though, you make some interesting points - certainly something to consider. I would have to pretty much abandon the approach I've started to take here to introduce an actual og dependency, though, so I don't think that will happen. But I'd be perfectly happy with another 'family' of auth plugins that was og-centric. And I'm definitely open to being convinced of more.
Oh, one thing that #16 leaves open is, who has the right to add a repository + branch to a project? In the short term, I would do this with a Drupal permission.
I think you might need to look at the patch again. But there are several answers to that - first, versioncontrol itself has nothing to do with deciding what repos can go on projects. That's all versioncontrol_project. And this patch is really focused on the question of authorizing manipulations of repo data once it's already been created, but punting the responsibility for just what auth logic gets used to whoever (e.g., versioncontrol_project) 'owns' the repository, via these auth plugins.
I don't follow this at all, primarily because I don't understand what a 'subbranch' is, or how that would cause any sort of collision. I think you're thinking the datastructures work/interact in some way that they don't :) Sounds maybe like you're still thinking in the old, super-size all-inclusive CVS repository mode? Not how things work going forward - _every_ project has its own repository, and in the new project creation workflow, new repo creation is triggered, there's no path selection. See esp. #961144-73: Determine/finalize technical requirements for post-Git migration project approval process.
In any case, we should talk more. I actually kinda have a hankering to get you on the phone/skype to chat about this :)
Comment #21
greg.1.anderson commentedYou're right, I did drift a little bit off-topic in #16; sorry. I do understand that it is plugable, I just wandered down into implementation-specific details (e.g. versioncontrol_project).
The same is true about my og comments. I did not mean to imply that versioncontrol_api should depend on og; it shouldn't. What I meant to say was that I don't see any advantage to implementing a non-og UI for branch-level access control. If the primary branch-level access control project is not og, then I could certainly pull together a 'family' of auth plugins that was og-centric as you suggest, so it's all good.
You are correct in your final points. I cannot use git. I must use subversion. Some of my projects will have their own repository, some must be a branch of an existing repository. In this respect, some of my comments (esp. #17) are only my problem, and do not apply to the standard git implementation.
Comment #22
sdboyer commentedOK, that clarifies a lot. And is actually great - we are trying very hard to take care that vcapi stays a real API, but having somebody actually interested in a backend other than git is an excellent counterpoint to where the migration team's focus is. Now, as to:
Maybe it's just that I'm not familiar with the UI you're talking about, but I don't see how there could be such a categorical win here. The basic 'account' family of plugins I'm working on is entirely structured around interacting with rights granted to individual users, not group-based rights. Which means the UI we need to present is one where, on a per-user basis, admin users can set those users' branch- and tag-level rights on an individual basis. The og use case is, for what I think are obvious reasons, interesting and compelling. But I don't see how its UI could be of benefit to the individual accounts case - it is, after all, based on group-related datastructures which aren't present or relevant in the basic single account case. I'm sure we could get a lot of ideas for how to put together that UI for the individual accounts case by looking at what's been done with og, sure, but I don't understand how we'd actually _use_ it.
If you did want to embark on writing a groups-based auth plugin for vcapi, I'd love to be kept in the loop and help with it as I can. And as for the subdirectories ACL issue, I don't think that needs necessarily be a concern, either. In the past, the svn backend has overlaid the concept of branches onto the directory structure in an svn repository, and could probably do the same thing with projects. Should be able to build proper access control enforcement around that without too much difficulty.
Comment #23
greg.1.anderson commentedCool, I got it -- you have more permissions to manage than I do. For now I'm just worried about "in" and "not in", so og makes a lot of sense.
Comment #24
sdboyer commentedA beautiful example of why this system ought to be pluggable :) Let's make sure to connect next week, though - I'd like to become as familiar as reasonably possible with your use case. There's that great rule about an API not _really_ being an API until it works well for at least two, preferably three, differing implementations...
Comment #25
sdboyer commentedAnother round on this patch; I believe this one should complete the lower-level mechanics. The permissions for interacting with repositories are potentially very verbose, though, and could use some sanity checking to ensure a) they're not too verbose (I think there's valid use cases for everything I have in there) and b) the logic is actually correct. I spent a couple hours tearing at my hair, trying to figure out the most efficient way to write the checking algorithms, and I may well have made mistakes.
Which leaves...the UI questions. Ugh. I'm still undecided as to how much really ought to live in this plugin, and how much ought to be other systems decorating it.
Also, re: #18, I reverted the removal of {versioncontrol_accounts}.
Comment #26
sdboyer commentedWhoops, empty patch file.
Comment #27
sdboyer commentedtagging for sprint 6
Comment #28
sdboyer commentedOK, posting an incremental patch here that does introduces SOME of the tests we need to verify the plugin actually works like we expect. That means testing all the plugin's methods - basically, CRUD and the
VersioncontrolAuthHandlerInterface. marvil07 is gonna pick those up, see if he can't finish it, while I ponder and think if there are any other parts of this that need to go in before being committed.Related to that, we're going to punt on the UI. See #992074: Decide if & what UI to provide for authentication plugins.
Comment #29
marvil07 commentedI solved some little problems:
- add second parameter to db_select() calls at VersioncontrolAuthHandlerMappedAccounts class
- save auth account data to DB and compare it to the inserted dummy data with two asserts.
but I end up on a question that is going to be quicker to answer for you: versioncontrol_auth_account schema information is inconsistent(see
hook_schema(), hook_update_6309() and the test array$super_user_dataatVersioncontrolAccountAuthPlugin::testAuthPluginCrud()), so what is the expected right data for that table?Comment #30
sdboyer commentedYeah, I caught that second parameter to the db_select() later last night, too. I'm not sure what you mean by schema inconsistencies, but the question about what the data is SUPPOSED to look like is legit - I haven't written that down anywhere yet :) Here's a pastebin of a quick test that I wrote which verifies that all the logic actually works correctly. Which is one of the big blockers.
Output from that looks like this:
Comment #31
marvil07 commentedOk, still needs to finish the test, but this:
Comment #32
sdboyer commentedAh, and I forgot - I have a skitch image describing the way I'm imagining the auth logic flow works internally for the 'account' plugin:
Comment #33
sdboyer commentedOK, another incremental patch. I think we're actually pretty much there, since we're punting on UI. What needs to be added now is documentation. Here's the commit log:
Comment #34
marvil07 commentedI end up with a weird error at tests, so posting an update based onthe last patch that:
- Re-insert lost changes in the patch at #223891-33
- minor: account auth plugin test: add uid for the comparison(db info autoreturns it, but we set it at setUserData() not in the array passed)
- Add methods to make it easier to create branches and tags on tests.
- Convert sdboyer #223891-30 test into repeatable test.
Comment #35
sdboyer commentedNew patch, sorted out the db issue (reserved field names were causing errors, evidently). All tests but one are now passing, leaving that one to marvil07 to sort out. New patch also adds a metric ton of documentation to the auth plugin.
Once we get that last test fixed, I think we're RTBC.
Comment #36
marvil07 commentedLast test not passing fixed!
Comment #37
sdboyer commentedCommitted. http://drupal.org/cvs?commit=461590
We've still got to think about the UI here, as this is TOTALLY not configurable from the UI at the moment - not even in the way that mapping plugins can be set on the repo edit page. Right now, systems (like, say, project*) that want to use this just have to write their own UI entirely, and we could certainly _help_ with that, at least a bit. But we didn't want that to delay getting the fundamental functionality in, and I didn't want to try to make too many guesses without a specific use case in mind. Which is why we've opened #992074: Decide if & what UI to provide for authentication plugins.
To be honest, I feel less confident about this architectural move than any of the other ones we've made since I started working on the migration. I do expect this to grow and change a fair bit as we learn more. But that's why we've opted towards a more minimalist approach with this - hopefully it shouldn't be too tough to rework/expand later.