The VersioncontrolAccount class has barely been touched since the entities refactor. It'll be getting upgraded as part of the unification of CRUD, but we also need to clean up its remaining methods, fix some property signatures, and decide where we move some of the statistical analysis logic that was previously attached to it.

There's a bit of discovery to do here, which I'll post a follow-up on once I've done. I'll then open additional, more specific issues with that discovery complete, as needed.

Comments

sdboyer’s picture

sdboyer commented
Assigned: Unassigned » sdboyer
sdboyer’s picture

sdboyer commented
Status: Active » Needs review

OK, this has been a pretty revelatory few days wrt the VersioncontrolAccount mapping, and there's a LOT to say about it. I've got a longer comment, but I want to update this now before our scrum, so here's some relevant bullets:

  • We're going to blow away the VersioncontrolAccount class completely. Byebye.
  • There were centralized VCS assumptions made when the account concept was originally constructed which make it really unhelpful to a DVCS. Basically, it assumed that the user behind the network operation == user responsible for a 'commit'. We need to get away from that.
  • VersioncontrolAccount does not represent real, true 'entity' data - the fact that it has a split primary key is a clue to this.
  • The entire way that VCAPI tries to handle doing auth (for network operations) is broken. It should be really, really pluggable, and the only part that's pluggable right now is a) the wrong part and b) broken. VCAPI handling auth internally basically means we'd have to write our own SSH key support, etc...which is obviously wrong. So yeah, big changes.

We need to let the auth happen by hook, decouple the network user from commit user, and if we do end up keeping VersioncontrolAccount around, it'd only be useful as a tool for helping perform the mapping (vcs user info <-> drupal user info) logic. That too, though, needs to be pluggable.

Like I said, I'm gonna make a bigger follow-up that explains some of the theory here, but I wouldn't mind off-the-cuff thoughts, either - marvil07?

marvil07’s picture

marvil07 commented
Status: Needs review » Active

Ok, I am no so in favour to remove account class completely, but I think backends account classes must implement logic about specific behaviour about its vcs.

I am completely in favour of re-visiting auth-methods: #223891: API change: make authentication management pluggable

We need to let the auth happen by hook, decouple the network user from commit user, and if we do end up keeping VersioncontrolAccount around, it'd only be useful as a tool for helping perform the mapping (vcs user info <-> drupal user info) logic. That too, though, needs to be pluggable.

Yep, we need to do that, but I do not see the need to remove the class, maybe is better to keep it and let the backends interact in critical moments like auth, and yes I was also thinking about the VersioncontrolAccount be only the drupal uid mapping(is that now the only thing that do?), and let the rest(needs to specify it) be extendable by backends as much as possible.

PS: status change as needs review means patches ;-)

sdboyer’s picture

sdboyer commented

backends account classes must implement logic about specific behaviour about its vcs.

But what behavior? That's the real question. What behavior really BELONGS on a VersioncontrolAccount class?

I'm not saying there isn't an argument to be made for some - just that I don't feel like it's an unambiguously good idea. So, throw some things out there that ought to be on the class :P

marvil07’s picture

marvil07 commented

Now {versioncontrol_accounts} table have:

      'uid' => array(
        'description' => 'The {users}.uid of the Drupal user associated with the VCS-specific username in {versioncontrol_accounts}.username.',
        'type' => 'int',
        'unsigned' => TRUE,
        'not null' => TRUE,
        'default' => 0,
      ),
      'repo_id' => array(
        'description' => 'Foreign key (referring to {versioncontrol_repositories}.repo_id) for the repository that contains the VCS account.',
        'type' => 'int',
        'unsigned' => TRUE,
        'not null' => TRUE,
        'default' => 0,
      ),
      'vcs_username' => array(
        'description' => 'VCS-specific username of the VCS account associated with the Drupal user in {versioncontrol_accounts}.uid.',
        'type' => 'varchar',
        'length' => 64,
        'not null' => TRUE,
        'default' => '',
      ),
    ),
    'unique keys' => array(
      'repo_id_username' => array('repo_id', 'vcs_username'),
    ),
   'primary key' => array('uid', 'repo_id'),

Account class(and this table) now have one drupal uid per repository. INHO account must have its own id, so we can then reference many versioncontrol usernames pero drupal user/repository.

That way, general mapping should be handled by account class.

Joining this with the activity(now operations again) we could refer an account id in the activity table for any type of activity(commits, network operations, etc).

If I get with more uses I will post again :-)

marvil07’s picture

marvil07 commented

The entire way that VCAPI tries to handle doing auth (for network operations) is broken. It should be really, really pluggable, and the only part that's pluggable right now is a) the wrong part and b) broken. VCAPI handling auth internally basically means we'd have to write our own SSH key support, etc...which is obviously wrong. So yeah, big changes.

IMHO using an account unique id and then calling a hook(used by modules like a-ssh-support-module) for authorization(account would have mapping to drupal users) is fine.

Am I missing something?

sdboyer’s picture

sdboyer commented

dammit, i've rewritten this post three times already. ok, short version - fine, let's introduce an account_id on the versioncontrol_accounts table. maybe we don't end up using it much, but at the very least it'll let us get rid of the contorted multi-PK logic in the controller. let me note, though, that the operations table maps records a direct uid mapping, which very much should stay. there's no need or reason to map it through a secondary table. if we don't know what drupal user is responsible for a commit, we just put in 0.

i could be convinced that we use the VersioncontrolAccount class as the central place for all our logic that helps manage the auth & committer info mappings. nothing's really for sure, though, until i've actually started to hack at the system and figure out what it's architecture ought to look like. TMTOWTDI...

sdboyer’s picture

sdboyer commented
Status: Active » Postponed
Issue tags: +git sprint 5

Marking this postponed - it's gonna be one of the sprint 5 biggies.

eliza411’s picture

eliza411 commented

sdboyer will be breaking this into two sub-issues, pluggable auth and pluggable mapping. When that's done, this issue should be closed.

marvil07’s picture

marvil07 commented

IMHO this issue can stay for the pluggable mapping(not really sure why is mentioned as that, but I mean what I mention on #5) and use #223891: API change: make authentication management pluggable for the pluggable auth.

@sdboyer: is that ok?

sdboyer’s picture

sdboyer commented
Status: Postponed » Active

I'd actually rather keep this as a meta-issue - #223891: API change: make authentication management pluggable is good for the auth, and I opened #979040: Make pluggable the process of mapping of raw vcs data to Drupal users for the mapping. I think there were some other things too, but I can't come up with them now, but will post links to them here as they come up.

I really want this issue to be ultimately closed at the end of this sprint - it'll be closed once we've closed all its sub-issues and are convinced the overall thing is done.

marvil07’s picture

marvil07 commented
Issue tags: +versioncontrol account refactor
greg.1.anderson’s picture

greg.1.anderson commented

It would be very nice if the new VersioncontrolAccount class and related code could be configured to not require the mapping table from Drupal account to VCS account. This would be very useful to anyone using mod_auth_mysql to authenticate their repository via Apache, where the Drupal username == the VCS username (presuming the Drupal usernames were constrained so that none contained characters illigal in the VCS). If an appropriate setting were selected (per VCS backend?), then the VersioncontrolAccount info could just be populated from the Drupal users table.

sdboyer’s picture

sdboyer commented

@greg.1.anderson - check. if VersioncontrolAccount even continues to exist, it will definitely not use that mapping table, except perhaps optionally by a particular plugin.

In my initial work on this on Monday, I explored the possibility of introducing an 'owner' field on VersioncontrolRepository, which would be a string containing the name of an owner plugin, that would then be responsible for doing things like managing auth and mapping. It seemed like a useful approach because it'd be a convenient, centralized way to designate in a single place that a given repository was owned by a particular module. However, that work ended up being a dead-end, because it was just too awkward how it coupled relatively disconnected elements together - such plugins would be sensitive to unrelated API changes.

So the preferred approach, which I have nothing more than conjecture on right now, is something more like this - we put a 'plugins' field on {versioncontrol_repositories}, which contains a serialized array of (ctools) plugin names that sit in particular slots (at least one for 'user auth', and one for 'user mapping' - or something like that). On VersioncontrolRepository::build(), we instantiate those plugins, and then add methods on the class itself which just delegate to the plugin. I'm gonna post to each of the sub-issues talking about what I think those plugins should look like, how they should work.

Note that if we end up deciding that other classes besides VersioncontrolRepository, that's fine - we can follow the same pattern for those classes, and just add something to VersioncontrolEntity::build.

marvil07’s picture

marvil07 commented
Issue tags: +git sprint 6
marvil07’s picture

marvil07 commented
Title: Revisit VersioncontrolAccount and bring it up to speed » [meta] Revisit VersioncontrolAccount and bring it up to speed

Updating title to actually refer its nature.

This is tracking versioncontrol account refactor tag IIRC.

For now the major thing left is #983926: Remove account class, that is too related with #879704: Kill the account status module completely (it is really probable that we stay with it but modified to use auth plugins).

The other point left is #970244: Create a views handler to map operation author/committer to their drupal user.

When that is done this meta issue should be closed.

eliza411’s picture

eliza411 commented
Issue tags: +git sprint 7

Tagging for Git Sprint 7.

sdboyer’s picture

sdboyer commented
Status: Active » Fixed

The issues mentioned in #16 are closed out, and we're officially done with account-related stuff (and this meta-issue) until probably after phase 2 finishes and we have a stable vcapi release.

Status: Fixed » Closed (fixed)
Issue tags: -git phase 2, -git sprint 4, -git sprint 5, -versioncontrol account refactor, -git sprint 6, -git sprint 7

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +git phase 2, +git sprint 4, +git sprint 5, +versioncontrol account refactor, +git sprint 6, +git sprint 7

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.