Skip to content
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’ll 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
Merged

Conversation

juliushaertl
Copy link
Member

@juliushaertl 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. 😉

- 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>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

capabilities.php Outdated
use OCP\Capabilities\ICapability;
use OCP\IURLGenerator;

class Capabilities implements ICapability {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use v2.php


public function __construct($appName,
IRequest $request,
$corsMethods = 'PUT, POST, GET, DELETE, PATCH',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kill this it is the same as the parent

public function __construct($appName,
IRequest $request,
$corsMethods = 'PUT, POST, GET, DELETE, PATCH',
$corsAllowedHeaders = 'Authorization, Content-Type, Accept',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

IRequest $request,
$corsMethods = 'PUT, POST, GET, DELETE, PATCH',
$corsAllowedHeaders = 'Authorization, Content-Type, Accept',
$corsMaxAge = 1728000,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

MailService $mailService,
IL10N $l10n,
Defaults $defaults) {
parent::__construct($appName, $request, $corsMethods, $corsAllowedHeaders, $corsMaxAge);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* @PublicPage
* @AnonRateThrottle(limit=5, period=1)
*
* @param $username
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use the typehints ;)

@param type $var

So

@param string $username

$this->setHint($hint);
}

public function setHint($hint) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setter for exception seems weird....

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@pellaeon
Copy link
Collaborator

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
Copy link
Member Author

@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.

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliushaertl
Copy link
Member Author

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

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 nextcloud:master Aug 15, 2017
@pellaeon
Copy link
Collaborator

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
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants