Restrict project creation to users that can use version control

That's because your account doesn't have the "Git user" role since you haven't checked the "I agree" checkbox. It's a permission matrix bug and/or a bug in how we're migrating existing vetted users. They should probably all have the Git user role automatically or something. Anyway, this is not a bug in Project* and the sandbox code itself, it's in how git-dev is being built/configured.

tagging for retest.

Still true, and this is a policy decision. It does pose a huge support problem, and since the user can create full projects it truly makes no sense.

Either: 'vetted Git user' should have neither ability or they should have both.

I favor bringing 'vetted Git user' over with the ability to create projects if the agreement is the same with the exception of Git vocabulary.

Removing the retest tag until I have reason to believe this will be different after next build.

Developer issue at #1032172: Issues to address before Jan 19 Build

Yes, we wouldn't have to touch any code if we just made all existing CVS users members of both "Git users" and "Git vetted users". Then everything works as expected.

Another option that wouldn't require any changes to code would be to just give "Git vetted users" the 'create sandbox projects' permission, too. The "I agree" checkbox wouldn't automatically be checked (and there'd be nothing forcing them to check it, really), but at least the UI WTF wouldn't be there.

If you want otherwise, it's not so simple as a hook_menu_alter(). It's only a single 'project' node type, that just has a sandbox bit. See #986718: Add support for sandbox projects for the patches and technical details.

OH, unless you mean you want to prevent vetted users from creating any projects at all until they check the "I agree" checkbox which gives them the 'use git' permission as part of the "Git user". Yeah, I guess that'd work. Although the logic would need to be something like:

user_access('use git') && (user_access('create full project') || user_access('create sandbox project'))

That's probably do-able, if we want to force everyone to re-agree to the terms of service. Although I suspect we'll get a boatload of support requests about this unless we make it PAINFULLY obvious that everyone has to do this, ideally via the UI itself (not exactly sure how that'd look), not just announcements and docs.

Tagging with Legal because Crell asked me to when I had previous questions for legal.

/me has been gotten a hold of, in a thoroughly inappropriate way because it was by sdboyer... :-)

I would actually prefer that we *do* force everyone to re-check the checkbox. The things you have to agree to post-git-migration are slightly different than they are now, and different than they were 5 years ago when I first got a CVS account. They have evolved over time. By rights any time we change anything we should be requiring re-affirmation so that people cannot say "hey, you changed the policy after I signed it, that's no fair, screw you!" (Technically it's easy to add a "we reserve the right to change this policy without telling you" clause, and most website licensing terms do so, but the legal term for that is a "dick move", and I don't think we want to be dicks.)

That makes this a good opportunity to make sure people know about those changes; I'm thinking of "GPLv2-and-later" rather than just "the GPL", the explicit statement that "yes, I am legally allowed to distribute this in the first place", the new code of conduct, etc.

So, if possible, I would recommend:

- You need to check a box to get "Git user". At the start, no one has this.
- "Git user" can create sandbox projects and push to them.
- "Git cool user" can promote sandbox projects to real projects. At the start, anyone who had a CVS account has this.

Which means that current CVS users will need to check a box before they do anything, but after that they can do both things (create and promote).

OK, with these discussions, and understanding that the goal is to only allow project creation AT ALL if the 'Git user' role has been allocated (as that's our primary signal that someone has agreed to the condition by checking the checkbox), then this is a slightly modified version of the code from #7 which makes for looser coupling in two ways:

• It uses the generic 'use version control systems' permission instead of a git-specific one, so it'll work with any properly set up vcapi backend.
• If vc_project is not present, then skip the check, and only care about the 'create full projects' or 'create sandbox projects' permission. That way this all works gracefully. Note that the check is for vc_project, so you can have vcapi installed, but it only intercedes in the access process if vc_project is installed (as that's the indicator that you want the two to work together).

- I slightly chafe at seeing this as a "critical bug" in project. Project's not doing anything here. ;) Re-titling and reclassifying to reflect the current proposal.

- I slightly chafe at project having to know about this at all. I was hoping to use this migration as a good excuse to clean up special-case logic like this (you know, like the lines of code immediately following the ones you're adding). ;) What's wrong with vc_project doing the hook_menu_alter()?

- Thinking off d.o (which is what I have to do if this issue is in the project.module queue) the logic of this patch sort of contradicts #1025916: Add a setting for enabling version control for a project. Why should I need 'use version control systems' to create an issue-only project that doesn't touch version control? The answer is obvious in the d.o case, but it's less obvious for anyone else.

It's too late here and I need sleep, so I can't really propose a concrete alternative. But, I also don't think (at least not yet) that this should be committed as-is. So, I'm calling this "needs work". Either we need a new patch (perhaps for a different module) or we need a comment here to address my concerns and convince me I should commit this. ;) Hope that's received in the light-hearted manner I'm writing this. I'm not really challenging anyone to a duel or anything. ;) Just trying to clearly communicate my concerns and what I think needs to happen to move this forward. Cool?

Thanks,
-Derek

Sure, I was going to write it as a patch that would work using hook_menu_alter(), but then it seemed to me that it's not just the menu entry, but really the whole node access system that needs to be aware of this restriction. Maybe not, or maybe there's a slick way to do alters into the node access system if so, but I wasn't sure - so I went with the way that I knew would work and could do in 5 minutes. I quite agree that it's hacky and awful to do checks like this, and have generally been in support of using the migration as an excuse to clean up our code and implement better separation. So if a menu alter can do everything we need, by all means, throw out the patch, move this to vc_project and make the change there, instead. I'd be 150% for that.

And really, I should have added the tags I'm adding here when I originally moved queues, sorry - the rule has been that the priority setting for issues with the 'git phase 2' refers to their importance to the migration process, not necessarily the overall priority to the project itself in its own context.

Some movement on this, please?

I'm with dww, this should be in drupalorg_git_gateway, or (maybe) drupalorg_project.

Thing is, D6 sucks and there's no way to alter the answer from hook_access() other than to do a full-blown node_access module with access grants and all that crap. :( So either we have to design our own alter hook here, or just add the hack from #11. At this point, given time crunch, I'm leaning towards committing #11 for now, and mark this issue postponed until after the D7 port and clean it up then.

Works for me. Like I said, it makes my skin crawl too.

Heh, so we basically did what jpetso originally proposed at #186856: Cleanup node access hacks from versioncontrol_project. ;) Committed #11 to HEAD: http://drupal.org/cvs?commit=489600

Since this particular issue is tagged for git sprints and such, I'm just going to call it fixed (instead of duplicate). Instead, I turned #186856 into the cleanup task for after #834252: [meta] Port Project to Drupal 7.

Just a note - if you're one of the few people with administer nodes (d.o administrators), this permission check gets bypassed. At least I think it's administer nodes.

Noting this because I've been bypassing it over and over again during testing and wondering why the fix didn't seem to take when the logic was correct :)

Right, core never even calls hook_access() if a user has 'administer nodes'.