anvilresearch / connect

A modern authorization server built to authenticate your users and protect your APIs
http://anvil.io
MIT License
361 stars 86 forks source link

Remove format validation on sub #313

Open simonrenoult opened 8 years ago

simonrenoult commented 8 years ago

AnvilConnect tries to validate the format of the _id field which stores the sub claim. Thus, specifying its value with a non-uuid value (format chosen by AnvilConnect, see here) throws an error:

{ [ValidationError: "12345678" is not a valid uuid.]
  valid: false,
  errors: 
   { _id: 
      { attribute: 'format',
        property: '_id',
        expected: 'uuid',
        actual: '12345678',
        message: 'is not a valid uuid' } },
  name: 'ValidationError',
  message: '"12345678" is not a valid uuid.',
  statusCode: 400 }

Nowhere in the spec is mentioned the sub format. On the contrary, it uses at several locations a simple integer (see paragraphs 2, 5.5.1, 8, A.2).

As discussed, a temporary workaround is to remove the format property on the _id field (in server.js for instance)

var User = require('node_modules/anvil-connect/models/User')
delete User.schema._id.format

I guess that's a bug.

christiansmith commented 8 years ago

I wouldn't call it a bug. The UUID actually conforms to the spec, which only requires:

A locally unique and never reassigned identifier within the Issuer for the End-User, which is intended to be consumed by the Client, e.g., 24400320 or AItOawmwtWwcT0k51BayewNvutrJUqsvl6qs7A4. It MUST NOT exceed 255 ASCII characters in length. The sub value is a case sensitive string.

The suggested workaround isn't ideal from the perspective of organizing your changes to the server, but it does work for now.

We have a dramatically better solution to the problem of customizing Connect in the works. It will take a while to fully integrate, but the end result should enable a number of these open issues to be solved in a way that doesn't need mountains of new conditional logic in the server to handle every possible variation on requirements.

simonrenoult commented 8 years ago

Well, that's at least an oversight on AnvilConnect part. But as you say, since there's a workaround and a fix on its way, that's good enough.