702498 - Migrate accountportal to pyramid

Status: VERIFIED FIXED

(Cloud Services :: Server: Account Portal, defect)

Description

[Toby Elliott [:telliott]]

Accountportal is the black sheep of the services family, since it has actual html output, etc. Would be nice to use as much of the infra as possible, which means moving it to pyramid is a good step.

Comment 1

[Toby Elliott [:telliott]]

Since it's a completely new port, attaching a diff would be a true horrorshow. Therefore, I'm r? Ben on the following zip:

http://people.mozilla.com/~telliott/accountportal.zip

It's my first pyramid, so I'm happy to hear about area where I did something stupid.

once it's downloaded, make, make test, then 

bin/paster serve etc/dev.ini

Not sure yet how we're deploying pyramid apps, so I don't have the static files serving.

Also ccing rfkelly here for his thoughts on the auth stuff he's working on and how it relates to the credentials.py in the zipfile here.

Comment 2

[Toby Elliott [:telliott]]

Attachment: Placeholder for the review

Comment 3

[Ryan Kelly [:rfkelly]]

telliott, do you have this available in e.g. a mercurial branch somewhere?  I'd like to try tweaking some of the credentials stuff to merge it with my auth work and it'll be easier to manage with proper VCS of some kind.

I've created "accountportal-refactor" on mozilla-services github to use in the meantime, just so I can keep track of things.

Comment 4

[Ben Bangert [:benbangert]]

Comment on attachment 574493 [details]
Placeholder for the review

General thoughts
- Why using includeme per controller module? Scan the whole thing?
- The includeme uses a prefix, Pyramid supports passing a prefix for routes...
- Crumbs.... Pyramid's session has CSRF generation that uses UUID's
- Lots of use of request.registry.settings, why not alleviate some code-smell and make settings off the request directly with the custom request object?
- Lots of request.params, does it really not matter if it came from POST or GET?

controllers/__init__.py
- 45: logger imported but unused
- 73: get_template_lookup not used at all, remove?

controllers/accounts.py
- 110: why is password assigned regardless of if it worked?

controllers/console.py
- 153: The key branch of this, does it only execute during a POST or something? If so, it might make more sense to refactor into a separate view that has a request_method='POST' discriminator. Currently the forgot view seems to be a conglomerate of several views depending on params, these can be checked by pyramid view predicates to split into individual views.
- 187: send_email... maybe try pyramid_mailer and use a transaction? though this would require tying the db access into the transaction with the SQLA transaction extension as well, but provides better linkage

controllers/sync.py
- 79: Looks like two separate methods, view predicate with two methods?
- 83: regex compiled every request, compile once at module scope?
- 86:
	- Why is the file handle not closed?
- 132:
	http://docs.pylonsproject.org/projects/pyramid/en/1.2-branch/narr/viewconfig.html#predicate-arguments
	request_param lets a view match only if a variable is present, so the if can be removed by requiring 'purge'

credentials.py:
- check_login: why does this function handle a POST login? why not a view that logs a user in?

__init__.py:
- 63: How does User() magically find out whats in the request?
- 121-130: scan() can scan all modules in a package... given they have config, why not just add the mapping instead of the import stuff?

Comment 5

[Toby Elliott [:telliott]]

(In reply to Ryan Kelly [:rfkelly] from comment #3)
> telliott, do you have this available in e.g. a mercurial branch somewhere? 
> I'd like to try tweaking some of the credentials stuff to merge it with my
> auth work and it'll be easier to manage with proper VCS of some kind.
> 

Not yet. Because we're in the middle of the hg->git transition, I wanted to avoid having two completely different versions of accountportal floating around.

At this point, I can incorporate Ben's feedback, get everything updated into hg, then copy it over to github under the accountportal moniker. Should happen pretty quickly now.

Comment 6

[Ben Bangert [:benbangert]]

Some additional reference URL's:

CSRF: http://docs.pylonsproject.org/projects/pyramid/en/1.2-branch/api/interfaces.html#pyramid.interfaces.ISession.get_csrf_token

Include (note route_prefix keyword): http://docs.pylonsproject.org/projects/pyramid/en/1.2-branch/api/config.html#pyramid.config.Configurator.include

Splitting views based on request data (GET vs. POST, a request_param, etc.): 
http://docs.pylonsproject.org/projects/pyramid/en/1.2-branch/narr/viewconfig.html#predicate-arguments

Comment 7

[Toby Elliott [:telliott]]

(In reply to Ben Bangert from comment #4)
> Comment on attachment 574493 [details]
> Placeholder for the review


Thanks for the review. Notes inline.

> General thoughts
> - Why using includeme per controller module? Scan the whole thing?

Two reasons. There may be controllers in that directory that you don't want enabled (I might want to turn off the sync functions in my instance if I'm not running sync, for example). As a side benefit, I can add external controllers.


> - The includeme uses a prefix, Pyramid supports passing a prefix for
> routes...

Oh, sweet. Bug 705868

> - Crumbs.... Pyramid's session has CSRF generation that uses UUID's

Bug 705869

> - Lots of use of request.registry.settings, why not alleviate some
> code-smell and make settings off the request directly with the custom
> request object?

Hmm, not sure I follow what you mean here. Are you saying that at the start of each request we add another attribute to RequestWithUserAttribute that holds the settings? I'm fine with that, but I'm trying to use the pyramid conventions, which usually seem to want to use request.registry.settings.



> - Lots of request.params, does it really not matter if it came from POST or
> GET?
> 

Good question. I don't think we care that much - we're crumbed in the places we might worry about a GET, and some of the functions will use GET or POST depending on where in the lifecycle of the request it is (notably forgot-password, which you mention below). Looking into where we could tighten it up a bit would be a fine idea, but I don't think there's any security concerns to make it a priority.


> controllers/__init__.py
> - 45: logger imported but unused
> - 73: get_template_lookup not used at all, remove?
> 

I'll #NOQA logger - it's useful to have around for debugging. get_template_lookup is used in a couple of contexts where we can't use it for pyramid - as the hook for the error page, and in generating the email for the user.


> controllers/accounts.py
> - 110: why is password assigned regardless of if it worked?
> 

Good catch. Fixed.

> controllers/console.py
> - 153: The key branch of this, does it only execute during a POST or
> something? If so, it might make more sense to refactor into a separate view
> that has a request_method='POST' discriminator. Currently the forgot view
> seems to be a conglomerate of several views depending on params, these can
> be checked by pyramid view predicates to split into individual views.

This seems worth looking into, though I'd need to figure out how much code we'd be repeating in a refactor. Bug 705878.

> - 187: send_email... maybe try pyramid_mailer and use a transaction? though
> this would require tying the db access into the transaction with the SQLA
> transaction extension as well, but provides better linkage
> 

We've talked about the correct solution for email in the past and are essentially punting on doing anything more than this until we have a better general plan for email. It sends out sufficiently few emails that it's never been a priority to have work any better.


> controllers/sync.py
> - 79: Looks like two separate methods, view predicate with two methods?

Makes sense, though lower priority than forgot_password. I've filed Bug 705886 to handle this and the ones you note below

> - 83: regex compiled every request, compile once at module scope?

Fixed.

> - 86:
> 	- Why is the file handle not closed?

Because it goes out of scope? ;) Fixed.

> - 132:
> 	http://docs.pylonsproject.org/projects/pyramid/en/1.2-branch/narr/
> viewconfig.html#predicate-arguments
> 	request_param lets a view match only if a variable is present, so the if
> can be removed by requiring 'purge'
> 
> credentials.py:
> - check_login: why does this function handle a POST login? why not a view
> that logs a user in?
> 
> __init__.py:
> - 63: How does User() magically find out whats in the request?

Hmm, not sure I understand this question. The User object is populated by credentials.py, either in 92-95 or 109 (which updates user as a side-effect). It's just a place to store the object that we use.

> - 121-130: scan() can scan all modules in a package... given they have
> config, why not just add the mapping instead of the import stuff?

Unless I'm misreading this, I think the answer here is the same as the answer to your first question.

Comment 8

[Toby Elliott [:telliott]]

Major changes now in http://hg.mozilla.org/services/account-portal/rev/ca3654edd4ac

Onto the ones discussed above, then i18n.

Comment 9

[James Bonacci [:jbonacci]]

Replaced by BID. Nothing to fix.