Closed Bug 851679 Opened 11 years ago Closed 11 years ago

SecReview: forked/updated version of the browserid-wordpress plugin for integrating Persona.org login/authentication (will be used on Quality.Mozilla.Org/QMO)

Categories

(mozilla.org :: Security Assurance: Review Request, task)

Product:
mozilla.org
Component:
Security Assurance: Review Request
Type:
task
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: stephend, Assigned: freddy)

Details

(Whiteboard: [completed secreview][Web]) 

1. Who is/are the point of contact(s) for this review?

* For QMO integration, Craig Cook, Raymond, myself, with Shane (sorry for the lack of notice, Shane!) Tomlinson helping, hopefully, on plugin-specific issues/questions

2. Please provide a short description of the feature / application (e.g. problem solved, use cases, etc.):

* We'd like to remove our installation of and dependency on, Buddy Press (http://buddypress.org/) for QMO (https://quality.mozilla.org/), and, at the same time, integrate Persona.org authentication, using https://github.com/shane-tomlinson/browserid-wordpress/

3. Please provide links to additional information (e.g. feature page, wiki) if available and not yet included in feature description:

* See above (note: Shane's plugin is a fork/upgrade from http://wordpress.org/extend/plugins/browserid/, which claims it's "no longer supported." 

4. Does this request block another bug? If so, please indicate the bug number

* No

5. This review will be scheduled amongst other requested reviews. What is the urgency or needed completion date of this review?

* We'd really, really like to move off Buddy Press, streamline updates to QMO (security/functionality), as well as bring QMO up to parity with most other Mozilla websites - ASAP (hopefully it'll be quick)

6. To help prioritize this work request, does this project support a goal specifically listed on this quarter's goal list? If so, which goal?

* No, no direct, expressed goal

7. Please answer the following few questions: (Note: If you are asked to describe anything, 1-2 sentences shall suffice.)

Does this feature or code change affect Firefox, Thunderbird or any product or service the Mozilla ships to end users?

* No

8. Are there any portions of the project that interact with 3rd party services?

Shane can detail more, but it does verify authentication via https://login.persona.org/verify 

9. Will your application/service collect user data? If so, please describe

* No, I don't believe so

If you feel something is missing here or you would like to provide other kind of feedback, feel free to do so here (no limits on size):
Desired Date of review (if known from https://mail.mozilla.com/home/ckoenig@mozilla.com/Security%20Review.html) and whom to invite.
Summary: SecReview: → SecReview: forked/updated version of the browserid-wordpress plugin for integrating Persona.org login/authentication (will be used on Quality.Mozilla.Org/QMO)
this should be triaged on 2013-03-20
Whiteboard: [pending secreview] → [pending secreview][triage needed]
Assignee: nobody → fbraun
Whiteboard: [pending secreview][triage needed] → [pending secreview]
I am reviewing your code from github, but it came to my notice that the plugin on wordpress.com has been updated during the last days.

Please ping me, if I should change my target.
I had a bit of trouble using this plugin, and this is probably a bug and not a security issue: Logging in didn't work in my main profile, but it did with a fresh new browser profile. I think this comes down to third-party cookie support.
Against which component/product should I file bugs for this plugin?
Flags: needinfo?(stephen.donner)
Shane, are you able to help us with comment 3/comment 4?  Thanks!
Flags: needinfo?(stephen.donner) → needinfo?(stomlinson)
Freddy - is it OK to install this on a password-protected, non-LDAP instance (https://quality-dev.allizom.org/) to try to work out the issues you've noted, while we work together on the potential security issues?
Flags: needinfo?(fbraun)
(In reply to Frederik Braun [:freddyb] from comment #4)
> Against which component/product should I file bugs for this plugin?

I'd suggest https://github.com/shane-tomlinson/browserid-wordpress/issues, for now; thanks!
Pardon the delay in my response, :freddyb, and :stephend. 

The goals for my branch are to work on experimental features until they are ready to be merged into the mainline branch. The two most notable changes are using navigator.id.watch/request instead of navigator.id.get and "Only allow Persona logins"

"Only allow Persona logins" makes me uncomfortable and is not ready for wide adoption. This feature requires write access to .htaccess so that a new, Persona only, login page can be served. Instead of using a distinct page, the same effect can be accomplished using Javascript and CSS to hide the authentication form.

The sign in problem that :freddyb has seen I have seen as well, but I have not had time to diagnose. This also manifests in the original plugin.
Flags: needinfo?(stomlinson)
Some modifications to :stephend's responses.

3. Please provide links to additional information (e.g. feature page, wiki) if available and not yet included in feature description:

* See above (note: Shane's plugin is a fork/upgrade from http://wordpress.org/extend/plugins/browserid/, which claims it's "no longer supported." 

I wrote to Marcel, the plugin author, before starting my branch. He is no longer actively developing the plugin but is willing to review and accept patches.

8. Are there any portions of the project that interact with 3rd party services?

Shane can detail more, but it does verify authentication via https://login.persona.org/verify 

This is the only 3rd party service used aside from the core Persona dialog.

9. Will your application/service collect user data? If so, please describe

* No, I don't believe so

No
Flags: needinfo?(fbraun)
I have updated my repo to clean up the "Only allow Persona logins" feature. This is in much better shape. All recent changes from the 0.35 and 0.36 have been merged in. I have sent a patch with my changes to Marcel as well.

I feel more comfortable with this moving forward now, how can I help?
(In reply to Shane Tomlinson [:stomlinson] from comment #10)
> I have updated my repo to clean up the "Only allow Persona logins" feature.
> This is in much better shape. All recent changes from the 0.35 and 0.36 have
> been merged in. I have sent a patch with my changes to Marcel as well.
> 
> I feel more comfortable with this moving forward now, how can I help?

We want to get it installed on quality-dev.allizom.org ASAP so we can play with it -- Craig might be able to give you access, if you'd like to help with that, even post-install.
(In reply to Stephen Donner [:stephend] from comment #11)

> We want to get it installed on quality-dev.allizom.org ASAP so we can play
> with it -- Craig might be able to give you access, if you'd like to help
> with that, even post-install.

The previous version is on dev right now but I'll pull in the updates later today when I get a spare moment. I'll also make an admin account for Shane so he can use dev for testing.
:stephend I would be happy to help out. I have had an email exchange with the original author, Marcel. I am taking over as maintainer of the plugin.
Craig, can you provide access for me as well? This will help speed up the security review.
Flags: needinfo?(craigcook.bugz)
:craigcook, :stephend - I updated the plugin on Friday to make commenting with new users significantly smoother. It still isn't perfect, but it is much better.
:craigcook When I try to visit the test site, I am asked for credentials using HTTP auth. My LDAP creds are no good here, do I need a different set?
(In reply to Shane Tomlinson [:stomlinson] from comment #16)
> :craigcook When I try to visit the test site, I am asked for credentials
> using HTTP auth. My LDAP creds are no good here, do I need a different set?

Emailed them to you privately :-)
Ping? :)
:freddyb - :craigcook and I have been going back and forth testing and updating, but I am happy to have a review start.
It looks like I still need someone to grant me access for testing against quality-dev which will conclude my review.

The needinfo from :craigcook is still unresolved.
(In reply to Frederik Braun [:freddyb] from comment #21)
> It looks like I still need someone to grant me access for testing against
> quality-dev which will conclude my review.
> 
> The needinfo from :craigcook is still unresolved.

I just created your account on quality-dev and made you an administrator (so you can activate/deactivate plugins). Sorry for the delay.
Flags: needinfo?(craigcook.bugz)
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Freddyb are we all clear?
Flags: needinfo?(fbraun)
There are some outstanding blockers, which I would like to have fixed before this gets deployed.
I just noticed that only Shane was CCed on them, I then added a bunch of you guys on these bugs.
Flags: needinfo?(fbraun)
Whiteboard: [pending secreview] → [completed secreview]
:freddyb I have taken care of the issues you commented on, would you like to take a final pass?
Flags: needinfo?(fbraun)
Good to go
Flags: needinfo?(fbraun) → sec-review+
Whiteboard: [completed secreview] → [completed secreview][Web]
You need to log in before you can comment on or make changes to this bug.