New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Registration API #77

Merged
merged 8 commits into from Aug 15, 2017

Conversation

Projects
None yet
3 participants
@juliushaertl
Contributor

juliushaertl commented Jun 27, 2017

This PR implements a basic registration API as an OCS endpoint as discussed in #41

I've included the commit from #76 here, since a lot of refactoring has been done based on this and I think it might easier to review the changes together.

ToDo:

  • Write API documentation
  • Unit tests
  • The API should just use client secret as identifier for the registration, because otherwise users could create accounts by verifying with the token without receiving an email

Example usage is documented here for now with some basic curl commands:

https://gist.github.com/juliushaertl/5a1d1132e7370b5ad38fbd6da3cae5b8#example-usage

@pellaeon @rullzer

Maybe @pierreozoux @Gomez want to have a look as well. 馃槈

juliushaertl added some commits Jun 9, 2017

Some refactoring to make code simpler and more readable
- Refactor database classes to use entity/mapper pattern
- Use automatic class loading
- Move logic to RegistrationService class so it is reusable for the api

Signed-off-by: Julius H盲rtl <jus@bitgrid.net>
Add registration API
Signed-off-by: Julius H盲rtl <jus@bitgrid.net>
@rullzer

Took a quick look over it. Looks good in general. Will have a more detailed look and test tomorrow.

Show outdated Hide outdated capabilities.php
use OCP\Capabilities\ICapability;
use OCP\IURLGenerator;
class Capabilities implements ICapability {

This comment has been minimized.

@rullzer

rullzer Jun 29, 2017

Capabilties can only be queried (currently) when authenticated. nextcloud/server#4510

@rullzer

rullzer Jun 29, 2017

Capabilties can only be queried (currently) when authenticated. nextcloud/server#4510

'registration' =>
[
'enabled' => true,
'apiRoot' => $this->urlGenerator->linkTo(

This comment has been minimized.

@rullzer

rullzer Jun 29, 2017

Please use v2.php

@rullzer

rullzer Jun 29, 2017

Please use v2.php

Show outdated Hide outdated controller/apicontroller.php
public function __construct($appName,
IRequest $request,
$corsMethods = 'PUT, POST, GET, DELETE, PATCH',

This comment has been minimized.

@rullzer

rullzer Jun 29, 2017

kill this it is the same as the parent

@rullzer

rullzer Jun 29, 2017

kill this it is the same as the parent

Show outdated Hide outdated controller/apicontroller.php
public function __construct($appName,
IRequest $request,
$corsMethods = 'PUT, POST, GET, DELETE, PATCH',
$corsAllowedHeaders = 'Authorization, Content-Type, Accept',

This comment has been minimized.

@rullzer
@rullzer
Show outdated Hide outdated controller/apicontroller.php
IRequest $request,
$corsMethods = 'PUT, POST, GET, DELETE, PATCH',
$corsAllowedHeaders = 'Authorization, Content-Type, Accept',
$corsMaxAge = 1728000,

This comment has been minimized.

@rullzer
@rullzer
Show outdated Hide outdated controller/apicontroller.php
MailService $mailService,
IL10N $l10n,
Defaults $defaults) {
parent::__construct($appName, $request, $corsMethods, $corsAllowedHeaders, $corsMaxAge);

This comment has been minimized.

@rullzer

rullzer Jun 29, 2017

Remove the args with defaults here if they are the same ;)

@rullzer

rullzer Jun 29, 2017

Remove the args with defaults here if they are the same ;)

Show outdated Hide outdated controller/apicontroller.php
* @PublicPage
* @AnonRateThrottle(limit=5, period=1)
*
* @param $username

This comment has been minimized.

@rullzer

rullzer Jun 29, 2017

use the typehints ;)

@param type $var

So

@param string $username
@rullzer

rullzer Jun 29, 2017

use the typehints ;)

@param type $var

So

@param string $username
Show outdated Hide outdated service/registrationexception.php
$this->setHint($hint);
}
public function setHint($hint) {

This comment has been minimized.

@rullzer

rullzer Jun 29, 2017

Setter for exception seems weird....

@rullzer

rullzer Jun 29, 2017

Setter for exception seems weird....

juliushaertl added some commits Jun 30, 2017

Fix copyright, comments, phpdoc and OCS status codes
Signed-off-by: Julius H盲rtl <jus@bitgrid.net>
Add basic unit testing and tests for API controller
Signed-off-by: Julius H盲rtl <jus@bitgrid.net>
Use client secret as identifier
Signed-off-by: Julius H盲rtl <jus@bitgrid.net>
@pellaeon

This comment has been minimized.

Show comment
Hide comment
@pellaeon

pellaeon Jul 7, 2017

Owner

Hi @juliushaertl I looked over the changes and most looked good, and with the design of "pending" status it should be feasible if we want to incorporate admina approval feature in the future.

Some notes while I was reading the code:

Client secret

Client secret is held only by the client app, and is used to uniquely identify the client app making the registration request.

Q: Why is there need for a client secret at all? why not just check the registration status by Token?
A: a Token is only received when the email successfully delivers, and it's not received by the app, so there's no way for the client app to check registration status other than introducing client secret

Different behavior of verifyToken when registering with OCS API vs normal web form

When registering through normal web form, Token is verified before username and password are provided by the user (but the email is already provided).
When registering through OCS API, Token is verified after username and password are provided.
So the new verifyToken controller checks if username and password is already provided, if so, a "successfully registered" message is printed, if not, the old form is presented, and the user fills in username and password.

Some of the questions I haven't checked, will do that when I have more time, or you can just answer them ;-)

  • how does the new OCS flow handles resending verification tokens?
  • how does the new OCS flow handles things like "username squatting"? eg. an attacker can repeatedly register with popular usernames while not verifying the email, will new users be able to register if the username is already taken but the email hasn't been verified? or is there some kind of expiration mechanism?

I may think of more questions, I'll post them when I do.

In the mean time, I hope you start writing the documentations :-)

Great work! 馃殌

Owner

pellaeon commented Jul 7, 2017

Hi @juliushaertl I looked over the changes and most looked good, and with the design of "pending" status it should be feasible if we want to incorporate admina approval feature in the future.

Some notes while I was reading the code:

Client secret

Client secret is held only by the client app, and is used to uniquely identify the client app making the registration request.

Q: Why is there need for a client secret at all? why not just check the registration status by Token?
A: a Token is only received when the email successfully delivers, and it's not received by the app, so there's no way for the client app to check registration status other than introducing client secret

Different behavior of verifyToken when registering with OCS API vs normal web form

When registering through normal web form, Token is verified before username and password are provided by the user (but the email is already provided).
When registering through OCS API, Token is verified after username and password are provided.
So the new verifyToken controller checks if username and password is already provided, if so, a "successfully registered" message is printed, if not, the old form is presented, and the user fills in username and password.

Some of the questions I haven't checked, will do that when I have more time, or you can just answer them ;-)

  • how does the new OCS flow handles resending verification tokens?
  • how does the new OCS flow handles things like "username squatting"? eg. an attacker can repeatedly register with popular usernames while not verifying the email, will new users be able to register if the username is already taken but the email hasn't been verified? or is there some kind of expiration mechanism?

I may think of more questions, I'll post them when I do.

In the mean time, I hope you start writing the documentations :-)

Great work! 馃殌

@juliushaertl

This comment has been minimized.

Show comment
Hide comment
@juliushaertl

juliushaertl Jul 7, 2017

Contributor

@pellaeon Thanks for your feedback. I really appreciate it.

Q: Why is there need for a client secret at all? why not just check the registration status by Token?
A: a Token is only received when the email successfully delivers, and it's not received by the app, so there's no way for the client app to check registration status other than introducing client secret

Exactly. The token should not be exposed anywhere else than in the email. Otherwise that would allow users to verify their address without receiving an email.

how does the new OCS flow handles resending verification tokens?

A new token will be generated and sent to the users email. See https://github.com/pellaeon/registration/pull/77/files#diff-b9e15819672f6817a033ecc447a6e2a2R153

how does the new OCS flow handles things like "username squatting"? eg. an attacker can repeatedly register with popular usernames while not verifying the email, will new users be able to register if the username is already taken but the email hasn't been verified? or is there some kind of expiration mechanism?

I have not thought about that kind of attack vector until now. But I guess we could at least add the AnonRateThrottle rate limit annotation that Nextcloud has introduced here. At the moment there is no check if there already is a pending registration for the username, but i'll add that as well. I need to think a bit more about this, maybe we need some kind of expiration, as you said.

I'll try to finish documentation of the API and the unit tests later today.

Contributor

juliushaertl commented Jul 7, 2017

@pellaeon Thanks for your feedback. I really appreciate it.

Q: Why is there need for a client secret at all? why not just check the registration status by Token?
A: a Token is only received when the email successfully delivers, and it's not received by the app, so there's no way for the client app to check registration status other than introducing client secret

Exactly. The token should not be exposed anywhere else than in the email. Otherwise that would allow users to verify their address without receiving an email.

how does the new OCS flow handles resending verification tokens?

A new token will be generated and sent to the users email. See https://github.com/pellaeon/registration/pull/77/files#diff-b9e15819672f6817a033ecc447a6e2a2R153

how does the new OCS flow handles things like "username squatting"? eg. an attacker can repeatedly register with popular usernames while not verifying the email, will new users be able to register if the username is already taken but the email hasn't been verified? or is there some kind of expiration mechanism?

I have not thought about that kind of attack vector until now. But I guess we could at least add the AnonRateThrottle rate limit annotation that Nextcloud has introduced here. At the moment there is no check if there already is a pending registration for the username, but i'll add that as well. I need to think a bit more about this, maybe we need some kind of expiration, as you said.

I'll try to finish documentation of the API and the unit tests later today.

juliushaertl added some commits Jul 8, 2017

Cleanup status codes
Signed-off-by: Julius H盲rtl <jus@bitgrid.net>
Check if username is already used for a pending registration
Signed-off-by: Julius H盲rtl <jus@bitgrid.net>
@juliushaertl

This comment has been minimized.

Show comment
Hide comment
@juliushaertl

juliushaertl Jul 18, 2017

Contributor

Sorry for the delay. I've added unit tests at least for the new code parts. @pellaeon It might make sense to enable travis ci or some similar ci service on the repo, so we can see if some patches break the unit tests in the future.

The API documentation can be found here: https://gist.github.com/juliushaertl/5a1d1132e7370b5ad38fbd6da3cae5b8

Contributor

juliushaertl commented Jul 18, 2017

Sorry for the delay. I've added unit tests at least for the new code parts. @pellaeon It might make sense to enable travis ci or some similar ci service on the repo, so we can see if some patches break the unit tests in the future.

The API documentation can be found here: https://gist.github.com/juliushaertl/5a1d1132e7370b5ad38fbd6da3cae5b8

@rullzer

I'm sure there is stuff we can improve on later. But for now this looks good to me. lets get it in!

@pellaeon pellaeon merged commit 3854317 into pellaeon:master Aug 15, 2017

@pellaeon

This comment has been minimized.

Show comment
Hide comment
@pellaeon

pellaeon Aug 15, 2017

Owner

Hey @juliushaertl , I encountered this error while upgrading the plugin:

Doctrine\DBAL\Exception\DriverException: An exception occurred while executing 'ALTER TABLE oc_registration ADD COLUMN "username" VARCHAR(255) NOT NULL': SQLSTATE[HY000]: General error: 1 Cannot add a NOT NULL column with default value NULL

Might be some problems with my Doctrine or MariaDB, I'm looking into it, please let me know if you have a hint.

Owner

pellaeon commented Aug 15, 2017

Hey @juliushaertl , I encountered this error while upgrading the plugin:

Doctrine\DBAL\Exception\DriverException: An exception occurred while executing 'ALTER TABLE oc_registration ADD COLUMN "username" VARCHAR(255) NOT NULL': SQLSTATE[HY000]: General error: 1 Cannot add a NOT NULL column with default value NULL

Might be some problems with my Doctrine or MariaDB, I'm looking into it, please let me know if you have a hint.

@pellaeon

This comment has been minimized.

Show comment
Hide comment
@pellaeon

pellaeon Aug 15, 2017

Owner

Oops, I forgot I had sqlite3 instead of MariaDB on my test server. So it's probably this problem: https://stackoverflow.com/questions/3170634/how-to-solve-cannot-add-a-not-null-column-with-default-value-null-in-sqlite3

Since sqlite3 is only used for testing purposes, I think the user may just drop the existing table and re-enable the plugin.

Owner

pellaeon commented Aug 15, 2017

Oops, I forgot I had sqlite3 instead of MariaDB on my test server. So it's probably this problem: https://stackoverflow.com/questions/3170634/how-to-solve-cannot-add-a-not-null-column-with-default-value-null-in-sqlite3

Since sqlite3 is only used for testing purposes, I think the user may just drop the existing table and re-enable the plugin.

nikosnikos pushed a commit to nikosnikos/registration that referenced this pull request Apr 12, 2018

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