Input data validation and overall refactoring.

[Zegnat]

Welcome to a slightly-bigger-PR.

Input data validation

Everything is taken from either the query string ($_GET) or post body ($_POST) through PHP’s filter_input. The super-globals are never accessed directly.

me, client_id, and redirect_uri are always validated with FILTER_VALIDATE_URL. Since PHP 5.2.1 this automatically means it expects a valid scheme and host. Others get their own regular expressions.

Currently an error is thrown if response_type is not id, code, or omitted. But non of those values have any effect on the authorisation code that will be generated. This means a scope can be included even on identification requests. In short: Selfauth does not care about the type. if you supply your password for whatever a client asks for, it will sign it.

Some questions that should be addressed before merging:

1. What to do if a scope is defined on an identification (response_type set to id or omitted) request?
2. What to do if the scope is defined as an empty string? Is there such a thing as an empty scope on authorisation requests?

Style guide

I have adapted the code to use shorter line-lengths and have overall the same whitespace rules. Thought this would be the right time to try and introduce a style guide as the reordering of checks touched a lot of lines of the code anyway.

I currently validate using:

phpcs --standard=PEAR,PSR1,PSR2 --exclude="PSR1.Files.SideEffects,PEAR.NamingConventions.ValidFunctionName,PEAR.Commenting.FunctionComment,PEAR.Commenting.FileComment" index.php

Notes:

• I include PEAR for their slightly stricter validation of breaking up IF statements and the like. Personal preference, feel free to be annoyed by it.
• We have to exclude PSR1.Files.SideEffects because we define functions within this file.
• There is still one line that does not validate, line 125, but I don’t feel like breaking up that string gets us anywhere.

Content negotiation

I took this out into its own function, but I am not happy about the name of the function yet. Very little tweaking otherwise, apart from making the regular expression more general. We can now easily start negotiating for other Content-Types if necessary.

The regular expression is also still a bit of a pain. But that can be its own future issue.

[sknebel]

1. What to do if a scope is defined on an identification (response_type set to id or omitted) request?

I'd say that's an error.

2. What to do if the scope is defined as an empty string? Is there such a thing as an empty scope on authorisation requests?

https://tools.ietf.org/html/rfc6749#section-3.3 defines scope as a non-empty value and says

If the client omits the scope parameter when requesting authorization, the authorization server MUST either process the request using a pre-defined default value or fail the request indicating an invalid scope.

I don't see a good default value, since we do not have a specific purpose where we could assume a default, so that'd be an error as well.

[Zegnat]

@sknebel, I think the latest commit – https://github.com/Inklings-io/selfauth/pull/16/commits/08f5f05fd3d8ae85cc621fa476331cc27f335b2a – covers the scope cases.

[sebsel]

What to do if the scope is defined as an empty string? Is there such a thing as an empty scope on authorisation requests?

It turns out the test-form on IndieAuth.com sends an empty &scope, and Selfauth complains about that. I would say if($scope === '') $scope = null;.

[Zegnat]

It turns out the test-form on IndieAuth.com sends an empty &scope, and Selfauth complains about that. I would say if($scope === '') $scope = null;.

But the RFC specifically does not allow an empty scope in the request:

The value of the scope parameter is expressed as a list of space-
delimited, case-sensitive strings.  The strings are defined by the
authorization server.  If the value contains multiple space-delimited
strings, their order does not matter, and each string adds an
additional access range to the requested scope.

  scope       = scope-token *( SP scope-token )
  scope-token = 1*( %x21 / %x23-5B / %x5D-7E )

Having &scope or &scope= as part of the URL means the client is specifically providing an empty string ('') as scope. By my understanding of the spec the current behaviour of treating this as false rather than null is correct.

[sebsel]

Having &scope or &scope= as part of the URL means the client is specifically providing an empty string ('') as scope.

The client is not providing, but requesting a scope. So the server (Selfauth) can say what the default is, right?

Not sure if that's actually what that document says. I'm being pragmatic about it, an empty string is not a useful scope. Treating it as false triggers errors on our side, whereas null just ignores the value, which is what you would expect from the flow presented by IndieAuth.com.

[dissolve]

should have probably done a seperate PR for just adding a coding standard, would have made this all a lot easier

[aaronpk]

My reading of the spec makes it sound like an empty string and a missing parameter should be treated the same, interpreting it as the client not providing a requested scope.

[Zegnat]

As per @aaronpk’s comment, &scope= and &response_type= are now treated as if they weren’t part of the request at all.