This repository has been archived by the owner. It is now read-only.

PHP example needs lots of improvements (meta) #13

Open
itpastorn opened this Issue Nov 9, 2012 · 6 comments

Comments

Projects
None yet
3 participants
@itpastorn
Contributor

itpastorn commented Nov 9, 2012

GitHub does not support bug Bugzilla-style dependencies, but I'll make one anyway.

The PHP example can be improved in many ways, some of which are small and easy, some that may require more thought.

Small improvements:

  • Filter input, stop using the superglobal arrays in lieu of the filter extension (PHP 5.2)
  • Stop using urlencode for the assertion, according to discussions on IRC and the mailing list
  • Add PHPDoc comments
  • Session management

Medium improvements

  • Make the HTML code better, stop mixing HTML and JavaScript (Make JS unobtrusive)
  • CSRF protection
  • "Back to login page" as a link is confusing, when it's really the same page in another state
  • Stop mimiciking Ajax using a hidden field. That solution won't scale.
  • Allow for self-hosting
  • Add note why cabundle.crt is needed and how the backup file works
  • Add debug options to cURL

Big improvements

  • Encapsulate the code in a class, perhaps also in a namespace
  • Allow for templating, stop using echo

itpastorn added a commit to itpastorn/browserid-cookbook that referenced this issue Nov 9, 2012

Added comments to the PHP code
This commit only adds comments but is not complete in that regard.
More comments are needed.

See also bug #13

@itpastorn itpastorn referenced this issue Nov 9, 2012

Merged

PHP code fixes #16

@fmarier

This comment has been minimized.

Show comment Hide comment
@fmarier

fmarier Nov 10, 2012

Member

Thanks for your help Lars!

One thing I'd like to note is that the goal behind this repo is not to have perfect code, but rather to show how easy it is to use Persona in PHP.

We should make sure that we're not recommending unsafe or otherwise bad practices (like the input filtering stuff), however we don't have to have the best possible architecture for this quick sample code.

I got lots of good feedback from people who said that it was really handy to have the whole thing in a single file so that you can get a complete picture of how everything fits together. At the same time, what you're suggesting, a proper "full" application, sounds quite useful too.

So maybe what we need is two things:

  1. the simplest possible safe PHP code (in the cookbook)
  2. a sample full-size application which includes templates, sessions, CSRF protection, phpdocs etc.

What do you think? That second program, could live in a separate repo just like this node.js sample application: https://github.com/tmarble/nongrata

Member

fmarier commented Nov 10, 2012

Thanks for your help Lars!

One thing I'd like to note is that the goal behind this repo is not to have perfect code, but rather to show how easy it is to use Persona in PHP.

We should make sure that we're not recommending unsafe or otherwise bad practices (like the input filtering stuff), however we don't have to have the best possible architecture for this quick sample code.

I got lots of good feedback from people who said that it was really handy to have the whole thing in a single file so that you can get a complete picture of how everything fits together. At the same time, what you're suggesting, a proper "full" application, sounds quite useful too.

So maybe what we need is two things:

  1. the simplest possible safe PHP code (in the cookbook)
  2. a sample full-size application which includes templates, sessions, CSRF protection, phpdocs etc.

What do you think? That second program, could live in a separate repo just like this node.js sample application: https://github.com/tmarble/nongrata

@pscheit

This comment has been minimized.

Show comment Hide comment
@pscheit

pscheit Feb 17, 2013

Contributor

i would contribute the php classes with namespace and correctly written in PSR-2 with composer and everything else for the full-size application.

I'll would develop it anyway

Contributor

pscheit commented Feb 17, 2013

i would contribute the php classes with namespace and correctly written in PSR-2 with composer and everything else for the full-size application.

I'll would develop it anyway

pscheit added a commit to pscheit/browserid-cookbook that referenced this issue Feb 17, 2013

improvement suggestions for #13
refactor readability
refactor function into a Persona class
use a php template to display the output, not functions
fix issues when E_NOTICE is enabled
inject audience if $_SERVER is not avaible
indent html
@pscheit

This comment has been minimized.

Show comment Hide comment
@pscheit

pscheit Feb 17, 2013

Contributor

why not:

  • provide a really small class instead of a function
  • use a php template instead of duplicate function calls

things i don't understand: what is the "assertion" hidden field here? how can we filter it better?

i fixed some issues when E_NOTICE is in error reporting
unfortunately i was in such a flow, that i forgot to split it up in some more commits, but this is a suggestion anyway

Contributor

pscheit commented Feb 17, 2013

why not:

  • provide a really small class instead of a function
  • use a php template instead of duplicate function calls

things i don't understand: what is the "assertion" hidden field here? how can we filter it better?

i fixed some issues when E_NOTICE is in error reporting
unfortunately i was in such a flow, that i forgot to split it up in some more commits, but this is a suggestion anyway

fmarier added a commit that referenced this issue Feb 18, 2013

improvement suggestions for #13
refactor readability
refactor function into a Persona class
use a php template to display the output, not functions
fix issues when E_NOTICE is enabled
inject audience if $_SERVER is not avaible
indent html
@fmarier

This comment has been minimized.

Show comment Hide comment
@fmarier

fmarier Feb 18, 2013

Member

Thanks for the patch @pscheit. I've cleaned up a few things in follow-up commits and was able to bring it down to less than 100 lines of code :)

I'm not sure I understand your question about the assertion in the hidden form though. The assertion needs to be posted to the backend for verification. We can either do that via an AJAX call or by submitting a form. The latter seems simpler for the purpose of a quick example. In larger applications, I'm sure many people will choose to use something like jQuery to do it.

Member

fmarier commented Feb 18, 2013

Thanks for the patch @pscheit. I've cleaned up a few things in follow-up commits and was able to bring it down to less than 100 lines of code :)

I'm not sure I understand your question about the assertion in the hidden form though. The assertion needs to be posted to the backend for verification. We can either do that via an AJAX call or by submitting a form. The latter seems simpler for the purpose of a quick example. In larger applications, I'm sure many people will choose to use something like jQuery to do it.

@pscheit

This comment has been minimized.

Show comment Hide comment
@pscheit

pscheit Feb 18, 2013

Contributor

okay, cool. I like it. good job

i was just wondering if the assertion is a string, a json plain, an elefant, something that flies, etc. But I understand now, that the formular is just a hack around ajax.

Contributor

pscheit commented Feb 18, 2013

okay, cool. I like it. good job

i was just wondering if the assertion is a string, a json plain, an elefant, something that flies, etc. But I understand now, that the formular is just a hack around ajax.

@fmarier

This comment has been minimized.

Show comment Hide comment
@fmarier

fmarier Feb 18, 2013

Member

@pscheit The assertion is a string that's base64-encoded. If you decode it, you get a JSON object. (If you're curious, you can use these tools to take a peak into one: https://github.com/fmarier/browserid-tools)

Member

fmarier commented Feb 18, 2013

@pscheit The assertion is a string that's base64-encoded. If you decode it, you get a JSON object. (If you're curious, you can use these tools to take a peak into one: https://github.com/fmarier/browserid-tools)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.