Fix basic auth not generating a random password

[Tieske]

When no password is provided, one will be generated before inserting the record in the datastore.

The generated password will be returned to the requester in a new field plain_password. This field is only returned when no password was provided.

Please review. The point of inserting is in the DAO, though I'd rather fixed it in the api.lua file there was no way without duplicating the crud.post method.

TODO:

• add testcase to prove no plaintext password is returned when a password is given
• add testcase to check the plaintext password is returned

Please review, I'll fix the todo's after agreement on the solution (point of insertion and adding plain_password).

[thibaultcha]

The idea is to rather implement this at the schema level, to avoid having to do the same operations for all future DAO implementations. Kong's "Model" layer is the schema definition of an entity. It is where most of the logic should be implemented so that it is abstracted as much as possible from DAOs (and the Admin API).

Granted, the schema and the DAO are in the same file as of now, but ideally such logic should be implemented above (see the encrypt_password method), and leverage the schema default and func, or again on_insert functions to generate a random password. Things might be a bit tricky because the schema validation will not like an unknown field (plain_password), but I believe this is where the changes must be done and improvements to the schema_validation must be done too.

[Tieske]

I'm not following you. In this case we need code executing before the update (generating password), and after (adding it in plaintext). func only runs the 'before' code.

Moving the code out of insert in a function above, would mean splitting it up in before and after functions. Calling both, and pass the password around in the arguments. The code would become worse imo. Is this what you meant or didn't I understand it correctly?

Looking at the schema definition I think func could be renamed to before_update and an after_update could be added. That would make it more clear imo, and easier to implement this type of updates.
Performance wise you might want to drop them and add the same two on the schema-level instead of the field-level (fewer check and function calls). Maybe could replace the marshall functions on the DAO also, though I haven't really looked in to those.

[thibaultcha]

Okay, I'll explain in more details, as I don't know how much of this you already know, hang on. Sorry if I am repeating things you already know. Also, all of the following points are is still open to discussion, they are just the current implementation following the needs we faced with Kong (and the little time we had).

One of Kong's requirements was to be database agnostic. In order to do that, we initially used to have a Model, represented with "Lua classes" for each entity, and a DAO, which was the actual implementation of a given database's operations. The application code would use the Model (with insert/update... methods), which in turn would call the DAO. This lead to a lot of duplicated code, and the addition of new entities was much more verbose, which we did not want because we wanted anybody to simply be able to define a new entity with as little effort as possible.

To do so, we decided to ditch out the Model layer. But since a common (and valid) design pattern wants one to implement validation logic at the Model layer, we came up with those stateless schemas, which define rules on an entity. Having the validation being performed on the Model allows it to be run when the entity is manipulated form anywhere (Admin API, CLI, Proxy logic...). The schema_validations and schemas work together to apply various rules on entities and properties of those entities. The schema validation is then called by the DAO before any DB operations.

In the long term, we would like several things:

• Kong to really be DB agnostic. That means an entity can be defined once, and any DAO implementation will follow the same validation rules. For now, the schema validation is called by the DAO itself, which is not optimal yet. When we implement the PostgreSQL DAO, it would have to also call the schema validation for example. This subject is still open to discussion.
• A developer could create an entity just with a schema definition. Currently this is not the case, a developer has to define the schema, and implement a DAO that simply inherits from the base_dao (since it implements basic CRUD operations and schema validations). implementing the DAO is most of the time just a formality, and the resulting DAO is simply inherited from base_dao without anything added. Like the keyauth-credentials DAO. But sometimes, one can add new methods (for example the APIs's DAO's find_all(), which are implemented differently depending on the database. PostgreSQL and Cassandra won't be executing the same code to retrieve all APIs, as you might have already guessed.

As you can see, a Kong entity is currently made of a schema, and a DAO. This design pattern is simpler than an ORM one (like we previously has with the Model classes) and, as we learned over time, more adapted for Lua and ngx_lua. Entities are simple tables, but rules apply on this table's values.

Hence, the real "Model" layer of Kong are the schemas. Now, I did not say the API to define and validate schema was complete. The schema_validation is where all the logic happens (it suffers from its own complexity, for example I prefer the validation I implemented for the config validation in next, which is more modular).

Back to the point of my initial comment: if you implement such logic in the DAO, this logic only applies for the Cassandra DAO, and not for any future DAO. It also prevents one from running, say, a dry-run of the entity validation. Which, we never know, might be useful for some validations in the Admin API or CLI. My idea was to follow the current design pattern of Kong: implement this logic in the key-credential's schema. Yes, that would probably mean improvements are to be made on the schema API (like you said, before_* and after_* methods) (iirc, a on_insert method exists). The day we abstract DAO creation for entities, nobody will have to worry about re-implementing that logic in all existing DAO implementations (for each datastore I mean). It would also probably improve the schema validation API for other contributors and users (for example, #771 mentioned a way to "hide" fields, that has also been suggested in #385).

---

I hope I was clear enough. Maybe I will think of other things to add. The whole point is: schemas are the current Model layer of Kong. Validation should happen in the Model (so, schema), and that the actual schema definition API is not yet finished and can certainly be cleaned-up/improved.

[Tieske]

Thanks for the detailed explanation! enlightening.

Back to the issue at hand; current schema does not allow for the implementation of this PR in the desired manner.

Proposal; merge this, open a new Issue to describe the refactorings for the schema.

[thibaultcha]

No I am not in favor of merging this. That would be doing one more quick hack instead of implementing the correct fix, which leads to technical debt. Sadly this is becoming the norm those days so I think we should avoid it as much as possible to happen again.