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

Get rid of basicauth #1736

Merged
merged 7 commits into from Sep 27, 2018
Merged

Get rid of basicauth #1736

merged 7 commits into from Sep 27, 2018

Conversation

leplatrem
Copy link
Contributor

@leplatrem leplatrem commented Aug 14, 2018

Alright, let's do this.

Ref #1733 #1494

The basicauth authentication policy is one the most confusing feature of Kinto. And it's used by default.
Let's switch everything to accounts by default.

  • Remove default value for policies
  • Rewrite tutorials
  • Remove basicauth for tests (use specific always true policy)
  • Rewrite functional tests
  • Get green tests

@n1k0
Copy link
Contributor

n1k0 commented Aug 16, 2018

Thank you for tackling this, I think it will benefit everyone eventually 👍

@leplatrem leplatrem force-pushed the 1733-some-default-config-improvement branch from 29dd324 to 2f3fc22 Compare September 24, 2018 17:42
@leplatrem
Copy link
Contributor Author

Some progress made here. I'm glad I found the motivation and time to tackle this 😄

@leplatrem leplatrem changed the title [WIP] Get rid of basicauth Get rid of basicauth Sep 26, 2018
@leplatrem
Copy link
Contributor Author

Tests are green \o/

r+ anyone ?

@@ -19,7 +19,7 @@ RUN \
apt-get install -y nodejs; \
cd kinto/plugins/admin; npm install; npm run build; \
pip3 install -e /app[postgresql,memcached,monitoring] -c /app/requirements.txt; \
pip3 install kinto-pusher kinto-attachment ; \
pip3 install kinto-attachment kinto-emailer kinto-elasticsearch kinto-portier kinto-redis; \
Copy link
Member

Choose a reason for hiding this comment

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

Are those official plugins that we want to ship with Kinto? Why would we want to install them all by default? Should we plan to move them to kinto.plugins?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took the ones that were not coupled to any brand or commercial service

# kinto.portier.webapp.authorized_domains = *.github.io
# kinto.portier.cache_ttl_seconds = 300
# kinto.portier.state.ttl_seconds = 3600
kinto.account_write_principals = account:admin
Copy link
Member

Choose a reason for hiding this comment

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

Should we enforce admin here as the default root user? I believe it would help people but it seems a bit brittle regarding security.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean by default root user?

Copy link
Member

@Natim Natim Sep 26, 2018

Choose a reason for hiding this comment

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

account:admin suggests that the admin account should exists. Also I would rather encourage people to use an email address as username because it simplifies the use of the sharing capabilities (although for the later we might need to verify the address)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

account:admin suggests that the admin account should exists.

Yes maybe there are some place in the getting started docs that don't mention creating the admin. I added mentions in the install docs

Also I would rather encourage people to use an email address as username because it simplifies the use of the sharing capabilities (although for the later we might need to verify the address)

We could have a regex for account ids, whose default would be an email adress for example. But please open another issue, I don't want to let this rot ;)

Copy link
Member

@Natim Natim Sep 26, 2018

Choose a reason for hiding this comment

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

But please open another issue, I don't want to let this rot

I wasn't suggesting that we should make any changes or enforce the email address there, I was just suggesting to use account:your.email@host.tld rather than account:admin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the sake of simplicity I think I prefer account:admin :/

Copy link
Member

@Natim Natim left a comment

Choose a reason for hiding this comment

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

Thank you for tackling this, seems pretty nice.

Copy link
Contributor

@glasserc glasserc left a comment

Choose a reason for hiding this comment

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

I don't really love the current accounts system, but I guess it's better than the old basicauth system, so sure.


Kinto uses the request headers to authenticate the current user.
If the authentication fails, the server will return a |status-401| error response. It can also return a |status-403| error response, which would mean that the operation performed on the resource is not allowed to unauthenticated users :)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused because here it seems like failed authentication should be obvious (401/403) but elsewhere the documentation says to request /v1/ and try to infer that auth has failed because of no principals. Is that specific to /v1/? Does it depend on the auth mechanism?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right I'll rephrase this!

@leplatrem leplatrem merged commit 2b147d0 into master Sep 27, 2018
@Natim Natim deleted the 1733-some-default-config-improvement branch November 8, 2018 16:46
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

4 participants