sdboyer and I have been coordinating about removing VersioncontrolAccount class, since it is not really adding value to the API.

That class is the only class not really following the refactor long ago.

That class only have(grep function includes/VersioncontrolAccount.php) these methods:

  public function usernameSuggestion($user) {
  public function isUsernameValid(&$username) {
  public function update($options = array()) {
  public function insert($options = array()) {
  public function delete($options = array()) {

CRUD methods can be safety removed, and the othre two methods really belong to the backend class, so we just need to move them.

About its table, it now has 'uid', 'repo_id' and 'vcs_username' fields, with uid and repo_id as PK, so a user can only have one vcs_username per repository, that's not so flexible, and now the mapping is going to be exclusively at operations table as #979040: Make pluggable the process of mapping of raw vcs data to Drupal users.

What is going to be long here will be replace all over the versioncontrol modules* the use of that class, hopefully not so much places :-)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marvil07’s picture

Lot of code to see :-/

For now:

  • move VersioncontrolAccount::isUsernameValid() to VersioncontrolBackend class
  • move VersioncontrolAccount::usernameSuggestion() to VersioncontrolBackend class

One more thing I realized re-reading some related code is that we will be also removing import/export accounts feature in this issue. Actually, the only backend implementing it is CVS backend, and it was done with the idea to move from cvslog to versioncontrol + versioncotrol_cvs, but now we are in the process to migrate at the same time from CVS to Git and for cvslog to versioncontrol* modules. So, the import feature conceived to migrate the cvs authentication file will not be really required. In the other side, since different VCSs(and systems on top of VCSs) have different ways to authenticate and we are going to be depending on other modules(through plugins) to do the authentication(#223891: API change: make authentication management pluggable), so there will not be a place __at versioncontrol__ where we can see authentication rules in detail, nor versioncontrol api will see a list of accounts(unless we force plugins to give such a list).

marvil07’s picture

Status: Active » Needs work
FileSize
19.09 KB

A good hint to see what is left to change/remove: git grep -in account(819 lines).

What extra this patch do:

  • Remove import/export accounts feature(VersioncontrolRepositoryImportExport)
marvil07’s picture

Well, now(with a local un-finished 76K patch), I am not sure about removing it, since, anyway the "account entity" is represented on versioncontrol_account_status module, and in the process of removing the class, I end up replicating it inside one of that module tables.

The other thing is that we loose account listings, since we are not really storing vcs_account vs uid.

Not really sure, feedback would be great :-)

BTW dww really wants to stay with that feature at VCS API #781344-15: Extend project maintainer UI for manipulating git project ACLs

sdboyer’s picture

I think dww doesn't really understand the proposal - if his concern were describing our actual proposal, then it'd be valid, but I don't think it is.

sdboyer’s picture

Could you post that local patch? I'd like to work on this.

marvil07’s picture

Sorry for the delay, I'm working on rebasing to the current code.

marvil07’s picture

Status: Needs work » Active

I am splitting the patches I think are independent:
- #989174: Move account class non-object-dependent non-crud methods to backend class
- #989210: Remove export-import accounts feature

I'm going to provide the rest of patch here, but now I'm discussing with sdboyer some architectural stuff that might end up invalidating my unstable patch :-p

PS: moving status as patches are in other issues.

marvil07’s picture

Assigned: marvil07 » Unassigned
Status: Active » Needs work
FileSize
53.29 KB

After committing the other changes and rebasing the rest on top of current code, I end up with this patch.

It:

- remove VersioncontrolRepository::loadAccounts(). account class is going to be removed
- Remove account class and its controller.
- Add a serial field to versioncontrol_account_status_users table.
- Move account delete to versioncontrol_account_status module.
- Move 'versioncontrol/register' menu item to versioncontrol_account_status module.
- Move menu items and functions that deal with accounts to versioncontrol_account_status module.

It end up in unstable code, and the main idea I was trying to do is move "account entity" to the versioncontrol_account_status module, where the it is necessary. Anyway, this still need major refactor on versioncontrol_account_status module.

BTW I end up writing a comment #879704-3: Kill the account status module completely on a related issue, that I think can influence what we do here.

marvil07’s picture

Issue tags: +git sprint 6
marvil07’s picture

Assigned: Unassigned » marvil07

self-assigning, will be working on this now that #223891: API change: make authentication management pluggable finally got in!

marvil07’s picture

FileSize
121.17 KB

Finally back!

Here it is a patch that do not really remove account class :-/, the main reason behind it is that versioncontrol_account_status module assumes many things, so I just tried to get to a good point to review to post this. I am also adding account_id PK and introducing views implementation for versioncontrol_account_status module in preparation to remove some old code.

What this patch does(reorder and rework many aspect in the last patch, so re-listing):

- No more multi-filed PKs
  - Add a serial field to versioncontrol_account_status_users table.
  - minor: add some more info about account entity.
  - add account_id to {versioncontrol_accounts}
  - Stop usign a 2-level array for account controller output.
- Move a lot of code from versioncontrol module to versioncontrol_account_status module(mainly menu items, its callbacks and its dependencies).
- Views implementation for account status accounts listing.
- other changes
  - temporary avoid operations statistics that depend on the old VersioncontrolOperationCache class.
  - minor: Fix calls to versioncontrol_user_accounts_load_multiple().
  - Fix access versioncontrol_private_account_access() callback.
  - minor: avoid using not anymore avalaible commitlog_get_account_url().
  - minor: add FIXME: remove hacky VERSIONCONTROL_REGISTER_DEMO logic.
  - minor: remove VersioncontrolRepository::loadAccounts().
  - Remove VersioncontrolRepository::getAccountUidForUsername() and Remove VersioncontrolRepository::getAccountUsernameForUid().
- Move account entity versioncontrol_account_status module.
  - Move entity and controller classes.
  - Remove {versioncontrol_accounts} table.
  - Remove versioncontrol_accounts table from hook_views_data().

This still needs work, what I see now is:

  • Convert VersioncontrolAccount entity into VersioncontrolAccountStatusAccount, change the name of the hooks called on CRUD and stop implementing those hooks in favour of doing it directly(changing all the related forms).
  • Unify old authorization_methods with the current auth plugins(see versioncontrol_account_status_versioncontrol_authorization_methods()).
marvil07’s picture

FileSize
121.08 KB

After #999652: Reorganize VCAPI menu structure this no longer applies, so, thanks git to make a rebase of 25 non-trivial commits a no-so-painful and actually doable experience :-)

eliza411’s picture

Issue tags: +git sprint 7

Tagging for Git Sprint 7.

marvil07’s picture

FileSize
121.26 KB

After #990644: Improve backend interaction with views the last patch is not working again, so rebasing again.

I will be posting a new patch with actually new things inside soon.

marvil07’s picture

FileSize
126.88 KB

Finally the class and its controller removed passing the tests, this still needs a little more work to actually let the code in a acceptable status(see what is missing at the end).

What is new on this patch:

  • Move versioncontrol hook_user and menu loaders for %versioncontrol_user_accounts to account status module.
  • Remove hacky VERSIONCONTROL_REGISTER_DEMO case.
    Tests can serve for its original use.
  • Remove account controller.
  • Remove account class.

What I think I am going to do in the next patch is:

  • Remove declarations of versioncontrol_entity_account_{insert,update} hooks and move account status hook implementations to the right place.
  • Remove the idea of $additional_data from account status module, since it do not apply anymore(account class do not exists), and move its information to real fields at {versioncontrol_account_status_users}(one field per array item or one data field).
  • Unify old authorization_methods with the current auth plugins(see versioncontrol_account_status_versioncontrol_authorization_methods()).
sdboyer’s picture

Shall I review, or wait for the next patch?

marvil07’s picture

@sdboyer: it would be great if you can take a look at it if you have time, I do not really think that the last chnages I want to do(as mentioned in the last comment) would really impact the current ~120KB too much ;-)

marvil07’s picture

Status: Needs work » Needs review
FileSize
138.57 KB
45.9 KB

What is new in this patch:

  • Replace account list page logic with a default view.
    • Remove lot of deprecated code in favour of views.
    • Add a views handler for account status edit link.
  • Remove hook_versioncontrol_account_submit() and move account status implementation to the original form_submit, that now belongs to account status.
  • Remove last versioncontrol_entity_account_* hooks and add a data field to account status.
    • Remove declarations of versioncontrol_entity_account_{insert,update} hooks and move account status hook implementations to versioncontrol_account_status_set().
    • Convert account status $additional_data into a real data field.
      • Add data field to {versioncontrol_account_status_users} table.
  • Some minor fixes:
    • Use versioncontrol_account_status_get() on versioncontrol_account_status hook_versioncontrol_is_account_authorized().
    • Remove doc about hook_account().
    • Rename two hook_update_N implementations at versioncontrol module.

So, I think there will be work still to do in versioncontrol_account_status module, and with this patch I am giving up with cleaning the whole module, IMHO that should be done in future issues/patches, as I am uploading now patches > 130 KB.

The only thing I really want to solve in this patch before commit it is:

  • Unify old authorization_methods with the current auth plugins(see versioncontrol_account_status_versioncontrol_authorization_methods()).

@sdboyer: I think this is re-viewable, as your advice on the last mentioned bullet point would be appreciated.

PS: @sdboyer: I am uploading two patches just in case you had started reviewing the patch at #15.

sdboyer’s picture

Haven't started the review yet, but now that you've got #18 up I'll start poking at it.

sdboyer’s picture

Status: Needs review » Needs work

Yep, that's a pretty hefty patch, all right. I've only done a cursory review of the UIs, but I've been fairly well through the code (mostly focusing on what's been added).

I have to say that the whole accounts thing still makes me kinda twitchy. The approach by which accounts are handled is really a whole subsystem of its own, and solving it in bits and pieces is not optimal. But I said I'd give up on perfection here, and I meant it. So let me just make a few direct comments:

  • VersioncontrolRepository::isAccountAuthorized() is really kind of an uncomfortable method to retain, given that its utility is dependent on the submodule being enabled. I'd rather just get rid of it and have that functionality provided somewhere else. But...it's not a huge deal.
  • None of this is committed, so collapse versioncontrol_account_status_update630{1,2} into a single function
  • really, the whole include_unauthorized notion seemed broken from the start
  • using #-prefixed keys in forms to pass data is a holdover from D5. just store it in $form_state. (e.g., in versioncontrol_account_status_edit_form)

Fix up those things, and I'll give my thumbs-up to putting this in. At the very least, we're getting rid of a LOT of the old cruft, and have basic replacements for the old functionality. That's good enough to get us out the door.

Thanks, marvil07.

sdboyer’s picture

Oh and, to your point about authorization methods/auth plugins - I don't actually see the overlap. They're really about different things, since authorization method was about how an account gets created, but had nothing to do with ACLs once the accounts actually existed. Auth plugins are 100% about ACLs, and don't care about how an account is created (which is why they do well being defined by VCAPI - they define a basic idea without getting bogged down in the UIs to make them work)

sdboyer’s picture

It would be REALLY great to have this in before we wrap up sprint 7.

marvil07’s picture

Status: Needs work » Needs review
FileSize
188.88 KB
51.63 KB

So, I end up, focusing on showing how versioncontrol_account_status can be used as auth plugin.

Actually I am now thinking in auth plugins more like "authorization plugins", because I want _many of them_ to interact for the access, not just one. I was trying to find another alternative without chnaging the API, but I did not get any answer to it in that way.
I needed to do that in order to use account status _and_ commitlog. BTW commitlog is completely broken now, but if we go with the changes I made to the pluggable auth in this patch, I think it would be not so hard to update it(it is now basically 6.x-1.x API implementation :-/).

This patch finally introduce the refactor of VersioncontrolOperation::hasWrite(), that declares the hook that is used by commit_restrictions module, that's why I end up there.

This patch also introduces an UI for auth plugins(choose which plugins to use, no how it behaves), in the same way that we have UI for map users.

Finally, I added a new auth plugin on account status, that shows what I wanted to show about using account status as part of the authentication.

All tests passing ;-)

So, the detailed changes I made in this patch are:

  • Remove the repository->data['versioncontrol']['allow_unauthorized_access'].
  • tests: Let dummy entity creation methods at VersioncontrolTestCase use a flag for insert.
  • pluggable auth: Add the entity object to branch and tag creation auth validation.
  • Remove hook_versioncontrol_write_access declaration.
  • pluggable auth: Let use many auth handlers per repository.
  • Add methods about auth plugins to repository. They will represent the standard way to call plugin methods.
  • Add an UI to choose authentication methods to use at repo edition.
  • A new vcs_auth plugin based on accoust status module.

@sdboyer: I still need to add your suggetions, I'm ok with all, and will be working on that, but not sure if I could get it on time for the demo. In the other side, I really appreciate it if you can comment abobut my last changes, that I think are relevant. I know some can be independent, but, I started them here, so just to make it easy, let's stick them here.

PS: Sorry, now I am not near an internet connection, so sorry for delay.

sdboyer’s picture

I haven't reviewed this yet, and won't have time before the demo today, but

Actually I am now thinking in auth plugins more like "authorization plugins", because I want _many of them_ to interact for the access, not just one. I was trying to find another alternative without chnaging the API, but I did not get any answer to it in that way.

and

BTW commitlog is completely broken now, but if we go with the changes I made to the pluggable auth in this patch, I think it would be not so hard to update it(it is now basically 6.x-1.x API implementation :-/).

is quite disturbing to me. I'm not mentally prepared for an API change at that level, vc_project has already been built on the auth API that was written, and the goal with this sprint was to have a beta release. We also specifically considered and dismissed the use of multiple auth plugins in the past, since you can just write one that combines the functionality much more easily than we can create a system that adequately handles all the possible permutations.

sdboyer’s picture

Status: Needs review » Fixed
FileSize
121.76 KB
174.58 KB

OK, I'm sorry, but a lot of the patch work that's culminated in #23 is not going in. Not now, anyway. I've committed a subset of the patch which pertains much more directly to the changes we've been working on for months, but the vast swathes of old, 1.x code that overengineer the registration process are not something I want vcapi's potential users confusing themselves with. The rest of my patch rips out the account_status module completely. All tests pass. I'll also be making some follow-up commits immediately thereafter that remove old crufty stuff from the repository add/edit form, and rip out the commit restrictions module. We need to make a release, which means we can't let code that we'd like to have work at some point (and with a lot of it, I tend to question how well it really worked in the first place) still hang around.

While I accept that there's value to shipping VCAPI with some sort of system for managing the an account approval workflow, I have little to no faith that the system in vc_account_status will actually do a good job. Hell, versioncontrol/register doesn't even detect properly if the user is logged in, and user/[uid]/edit/versioncontrol 404s. There's no way this it's even close to ready without us investing yet MORE hours into making it passable, or at least consistent with the level of precision and quality we have achieved elsewhere in the 2.x branch. And we've been out of hours for screwing around for the better part of a month now, especially for something that's irrelevant to git phase 2.

Beyond that, the API changes you made make me very twitchy. Allowing multiple authentication plugins is a recipe for combinatorial disaster. There's a reason the menu system allows exactly one access function on a given menu callback - letting them stack together is inviting unexpected combinations that result in security breaches. You wrote it about as safely as possible - all plugins must pass, or no access is granted - but there simply is not time for me to do my due diligence on it and feel confident enough in it to make a stable release & put the code on d.o, even if it's never enabled. That feeling REALLY pertains (mostly, in fact) to the account_status system, too.

So, while I think it'd be worth revisiting account systems at a future time, it's pretty clear to me we're not ready to do it now, and it's not going to make the schedule we have to attend to. And at the end of the day, IMO it's a bonus feature, not part of our core functionality. I understand that it could suck for the (few) people who've run vcapi 1.x and would like to upgrade, but they'll just need to wait until we can do it properly. Most of all, I'm really sorry to be throwing away code that you spent time working on. To try to make it a little better, I've attached a patch vs. master and an incremental vs. your last commit, so as to hopefully make it easier for you to keep a hold on the work you did and maybe revive it later. But this issue needs to be finished with, now.

marvil07’s picture

It feels a little bad to throw away this(and not committing it! :-p), but you are right, it still need improving. And maybe is better to rely on more modules to implement account status functionality itself, so, I understand your point.

API changes on the patch

I still want to make some API changes to auth plugins.

How to provide more checks in the middle of authentication?

Why I wanted that?:

  • versioncontrol_account_status: it wants to allow only approved account users. This module has been removed, so no sense now.
  • commit_restrictions: it wants to prevent "pushing" things like bad tags(prevent common errors) and files like LICENCE.txt and things like that.

So, unless you also want to remove commit_restrictions submodule from versioncontrol project and do not provide a way to implement such a module, I still think it is necessary. Naturally I will open one more issue for it, but first it would be fine to know your opinion.

If we do not let multiple plugins to verify access I do not see a way to implement a module like commit_restrictions, unless you really want to keep that code hardcoded on the VCS hook.

Status: Fixed » Closed (fixed)

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

marvil07’s picture

About comment 26, yes we are relying on VCS hooks calling the right functions, and both versioncontrol_account_status and commit_restrictions were removed.

I've just added another related commit:
4df6600 Issue #983926 follow-up: Removes unused repository authorization_method.

  • Commit 8cec713 on repository-families, drush-vc-sync-unlock by sdboyer:
    Issue #983926 by marvil07, sdboyer: Remove VersioncontrolAccount class,...
  • Commit d4e9811 on 6.x-2.x, repository-families, drush-vc-sync-unlock by marvil07:
    Issue #983926 follow-up: Remove deprecated account authorization related...
  • Commit e676e22 on 6.x-2.x, repository-families, drush-vc-sync-unlock by marvil07:
    Issue #983926 follow-up: Remove account related classes from the base...
  • Commit 4df6600 on 7.x-1.x, drush-vc-sync-unlock by marvil07:
    Issue #983926 follow-up: Removes unused repository authorization_method...

  • Commit 8cec713 on repository-families by sdboyer:
    Issue #983926 by marvil07, sdboyer: Remove VersioncontrolAccount class,...
  • Commit d4e9811 on 6.x-2.x, repository-families by marvil07:
    Issue #983926 follow-up: Remove deprecated account authorization related...
  • Commit e676e22 on 6.x-2.x, repository-families by marvil07:
    Issue #983926 follow-up: Remove account related classes from the base...
  • Commit 4df6600 on 7.x-1.x by marvil07:
    Issue #983926 follow-up: Removes unused repository authorization_method...