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

[9.0][FIX][IMP] Backport of auth_totp bug fixes and improvements from v10 PR #898

Merged
merged 4 commits into from
Jul 20, 2017

Conversation

obulkin
Copy link
Contributor

@obulkin obulkin commented Jul 12, 2017

This PR brings several bug fixes and an admin improvement from the v10 auth_totp PR to v9. Please see the v10 PR for more info: #703.

@obulkin obulkin force-pushed the auth_totp-fixes-improvements-from-v10-PR branch from e9ebf45 to bd4a35f Compare July 13, 2017 01:24
@obulkin
Copy link
Contributor Author

obulkin commented Jul 13, 2017

@moylop260 - Could you take a look at what's going on here? Locally, all the auth_totp tests are passing, and I can't recreate the error showing up in the Travis logs. Runbot doesn't seem to be running any tests at all, but when I log in the functionality associated with the failing test case is present, so I would expect the tests to pass there as well.

@yajo yajo added this to the 9.0 milestone Jul 13, 2017
@moylop260
Copy link
Contributor

moylop260 commented Jul 13, 2017

You can reproduce the error using a ssh connection for runbot build:

ssh -L 18080:localhost:18069 -p 8779 odoo@runbot2.odoo-community.org
cp -r ~/data_dir/filestore/openerp_template ~/data_dir/filestore/obulkin
createdb -T openerp_template obulkin
~/odoo-9.0/odoo.py -d obulkin --db-filter=obulkin --xmlrpc-port=18069 --stop-after-init -i auth_totp --test-enable

screen shot 2017-07-13 at 10 20 27

Now you can debbug it:

NOTE: Run dropdb obulkin;createdb -T openerp_template obulkin in order to reproduce the error from scratch again.
NOTE2: You can connect to this second instance opening your browse: http://localhost:18080

@lasley
Copy link
Contributor

lasley commented Jul 13, 2017

@obulkin - once you solve this one, please also make a contribution to runbot_travis2docker ReadMe explaining how to SSH to runbot so that we can help minimize pings to Moises in the future. Looks like theres some info in the travis2docker ReadMe

@obulkin
Copy link
Contributor Author

obulkin commented Jul 13, 2017

Thanks, @moylop260! Unfortunately, I'm getting a Permission denied (publickey). error when I try to SSH in.

@moylop260
Copy link
Contributor

Little detail runbot take your ssh key from your github user: https://github.com/obulkin.keys
But you don't have registered one.

Send me email with your public key, please
cat ~/.ssh/id_rsa.pub

@obulkin obulkin force-pushed the auth_totp-fixes-improvements-from-v10-PR branch from bd4a35f to ca43743 Compare July 13, 2017 20:49
* Slightly reword README
* Replace LasLabs logo with OCA one
* Overload _build_model in res.users model to add two MFA fields to the model
class's list of self-writeable fields, allowing these fields to be edited by
users without admin permissions for their own record
* Update view_users_form_simple_modif and the unit tests in the module based
on the self-writeable field change
* Add MFA fields to normal res.users form view for admin access
* Update record rules to give admins read/unlink access to MFA authenticators
* Add ondelete='cascade' to the res.users.authenticator.create wizard model to
properly support deletion of users who have just created an MFA authenticator
* Add website compatibility by modifying the decorator on one of the routes and
updating the login_success request parameter as needed
@obulkin obulkin force-pushed the auth_totp-fixes-improvements-from-v10-PR branch from ca43743 to 22c64f0 Compare July 14, 2017 01:48
@obulkin
Copy link
Contributor Author

obulkin commented Jul 14, 2017

This PR should be ready for review now. There was an actual bug here, caused by the overloaded _build_model method in res.users, which is designed differently in v9 than in v10.

@moylop260
Copy link
Contributor

Do you know why locally you can't reproduce it?

@obulkin
Copy link
Contributor Author

obulkin commented Jul 14, 2017

@moylop260 - The short answer is no.

As you can see here, the v10 code overloads _build_model in res.users to add a couple MFA fields to the SELF_WRITEABLE_FIELDS attribute on that class. The problem is that in v9, _build_model returns an instance of the model class rather than the class itself, so when I first brought the code over, it was making these changes at the instance level. With an attribute pointing to a basic type, this probably would have thrown an error right away, but the attribute is a list, so in some cases this was still working through mutation by reference. However, in others it clearly wasn't. I don't know what differentiates the two scenarios as I stopped investigating once I was able to figure this out and could confirm that it was fixing the Travis test failure.

@obulkin
Copy link
Contributor Author

obulkin commented Jul 14, 2017

@lasley - I've created a PR that adds this SSH info to the OCA guidelines: OCA/maintainer-tools#282. This seemed like a better place to put it given that the instructions are specific to the OCA Runbot setup and are more likely to be found by contributors in this spot.

@lasley lasley merged commit 0b6208a into OCA:9.0 Jul 20, 2017
@lasley lasley deleted the auth_totp-fixes-improvements-from-v10-PR branch July 20, 2017 02:55
mikevhe18 pushed a commit to mikevhe18/server-tools that referenced this pull request Jul 29, 2017
* [FIX] auth_totp: Permissions fix and other tweaks
* Slightly reword README
* Replace LasLabs logo with OCA one
* Overload _build_model in res.users model to add two MFA fields to the model
class's list of self-writeable fields, allowing these fields to be edited by
users without admin permissions for their own record
* Update view_users_form_simple_modif and the unit tests in the module based
on the self-writeable field change

* [IMP] auth_totp: Admin support
* Add MFA fields to normal res.users form view for admin access
* Update record rules to give admins read/unlink access to MFA authenticators

* [FIX] auth_totp: User deletion
* Add ondelete='cascade' to the res.users.authenticator.create wizard model to
properly support deletion of users who have just created an MFA authenticator

* [FIX] auth_totp: Website compatibility
* Add website compatibility by modifying the decorator on one of the routes and
updating the login_success request parameter as needed
andhit-r pushed a commit to open-synergy/server-tools that referenced this pull request Jul 29, 2017
* [FIX] auth_totp: Permissions fix and other tweaks
* Slightly reword README
* Replace LasLabs logo with OCA one
* Overload _build_model in res.users model to add two MFA fields to the model
class's list of self-writeable fields, allowing these fields to be edited by
users without admin permissions for their own record
* Update view_users_form_simple_modif and the unit tests in the module based
on the self-writeable field change

* [IMP] auth_totp: Admin support
* Add MFA fields to normal res.users form view for admin access
* Update record rules to give admins read/unlink access to MFA authenticators

* [FIX] auth_totp: User deletion
* Add ondelete='cascade' to the res.users.authenticator.create wizard model to
properly support deletion of users who have just created an MFA authenticator

* [FIX] auth_totp: Website compatibility
* Add website compatibility by modifying the decorator on one of the routes and
updating the login_success request parameter as needed
andhit-r pushed a commit to open-synergy/server-tools that referenced this pull request May 18, 2018
* [FIX] auth_totp: Permissions fix and other tweaks
* Slightly reword README
* Replace LasLabs logo with OCA one
* Overload _build_model in res.users model to add two MFA fields to the model
class's list of self-writeable fields, allowing these fields to be edited by
users without admin permissions for their own record
* Update view_users_form_simple_modif and the unit tests in the module based
on the self-writeable field change

* [IMP] auth_totp: Admin support
* Add MFA fields to normal res.users form view for admin access
* Update record rules to give admins read/unlink access to MFA authenticators

* [FIX] auth_totp: User deletion
* Add ondelete='cascade' to the res.users.authenticator.create wizard model to
properly support deletion of users who have just created an MFA authenticator

* [FIX] auth_totp: Website compatibility
* Add website compatibility by modifying the decorator on one of the routes and
updating the login_success request parameter as needed
andhit-r pushed a commit to open-synergy/server-tools that referenced this pull request May 20, 2018
* [FIX] auth_totp: Permissions fix and other tweaks
* Slightly reword README
* Replace LasLabs logo with OCA one
* Overload _build_model in res.users model to add two MFA fields to the model
class's list of self-writeable fields, allowing these fields to be edited by
users without admin permissions for their own record
* Update view_users_form_simple_modif and the unit tests in the module based
on the self-writeable field change

* [IMP] auth_totp: Admin support
* Add MFA fields to normal res.users form view for admin access
* Update record rules to give admins read/unlink access to MFA authenticators

* [FIX] auth_totp: User deletion
* Add ondelete='cascade' to the res.users.authenticator.create wizard model to
properly support deletion of users who have just created an MFA authenticator

* [FIX] auth_totp: Website compatibility
* Add website compatibility by modifying the decorator on one of the routes and
updating the login_success request parameter as needed
andhit-r pushed a commit to open-synergy/server-tools that referenced this pull request Sep 6, 2018
* [FIX] auth_totp: Permissions fix and other tweaks
* Slightly reword README
* Replace LasLabs logo with OCA one
* Overload _build_model in res.users model to add two MFA fields to the model
class's list of self-writeable fields, allowing these fields to be edited by
users without admin permissions for their own record
* Update view_users_form_simple_modif and the unit tests in the module based
on the self-writeable field change

* [IMP] auth_totp: Admin support
* Add MFA fields to normal res.users form view for admin access
* Update record rules to give admins read/unlink access to MFA authenticators

* [FIX] auth_totp: User deletion
* Add ondelete='cascade' to the res.users.authenticator.create wizard model to
properly support deletion of users who have just created an MFA authenticator

* [FIX] auth_totp: Website compatibility
* Add website compatibility by modifying the decorator on one of the routes and
updating the login_success request parameter as needed
andhit-r pushed a commit to open-synergy/server-tools that referenced this pull request Sep 6, 2018
* [FIX] auth_totp: Permissions fix and other tweaks
* Slightly reword README
* Replace LasLabs logo with OCA one
* Overload _build_model in res.users model to add two MFA fields to the model
class's list of self-writeable fields, allowing these fields to be edited by
users without admin permissions for their own record
* Update view_users_form_simple_modif and the unit tests in the module based
on the self-writeable field change

* [IMP] auth_totp: Admin support
* Add MFA fields to normal res.users form view for admin access
* Update record rules to give admins read/unlink access to MFA authenticators

* [FIX] auth_totp: User deletion
* Add ondelete='cascade' to the res.users.authenticator.create wizard model to
properly support deletion of users who have just created an MFA authenticator

* [FIX] auth_totp: Website compatibility
* Add website compatibility by modifying the decorator on one of the routes and
updating the login_success request parameter as needed
andhit-r pushed a commit to open-synergy/server-tools that referenced this pull request Sep 21, 2018
* [FIX] auth_totp: Permissions fix and other tweaks
* Slightly reword README
* Replace LasLabs logo with OCA one
* Overload _build_model in res.users model to add two MFA fields to the model
class's list of self-writeable fields, allowing these fields to be edited by
users without admin permissions for their own record
* Update view_users_form_simple_modif and the unit tests in the module based
on the self-writeable field change

* [IMP] auth_totp: Admin support
* Add MFA fields to normal res.users form view for admin access
* Update record rules to give admins read/unlink access to MFA authenticators

* [FIX] auth_totp: User deletion
* Add ondelete='cascade' to the res.users.authenticator.create wizard model to
properly support deletion of users who have just created an MFA authenticator

* [FIX] auth_totp: Website compatibility
* Add website compatibility by modifying the decorator on one of the routes and
updating the login_success request parameter as needed
andhit-r pushed a commit to open-synergy/server-tools that referenced this pull request Sep 21, 2018
* [FIX] auth_totp: Permissions fix and other tweaks
* Slightly reword README
* Replace LasLabs logo with OCA one
* Overload _build_model in res.users model to add two MFA fields to the model
class's list of self-writeable fields, allowing these fields to be edited by
users without admin permissions for their own record
* Update view_users_form_simple_modif and the unit tests in the module based
on the self-writeable field change

* [IMP] auth_totp: Admin support
* Add MFA fields to normal res.users form view for admin access
* Update record rules to give admins read/unlink access to MFA authenticators

* [FIX] auth_totp: User deletion
* Add ondelete='cascade' to the res.users.authenticator.create wizard model to
properly support deletion of users who have just created an MFA authenticator

* [FIX] auth_totp: Website compatibility
* Add website compatibility by modifying the decorator on one of the routes and
updating the login_success request parameter as needed
andhit-r pushed a commit to open-synergy/server-tools that referenced this pull request Oct 4, 2019
* [FIX] auth_totp: Permissions fix and other tweaks
* Slightly reword README
* Replace LasLabs logo with OCA one
* Overload _build_model in res.users model to add two MFA fields to the model
class's list of self-writeable fields, allowing these fields to be edited by
users without admin permissions for their own record
* Update view_users_form_simple_modif and the unit tests in the module based
on the self-writeable field change

* [IMP] auth_totp: Admin support
* Add MFA fields to normal res.users form view for admin access
* Update record rules to give admins read/unlink access to MFA authenticators

* [FIX] auth_totp: User deletion
* Add ondelete='cascade' to the res.users.authenticator.create wizard model to
properly support deletion of users who have just created an MFA authenticator

* [FIX] auth_totp: Website compatibility
* Add website compatibility by modifying the decorator on one of the routes and
updating the login_success request parameter as needed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants