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

Support setting cookie domain for auth #827

Merged
merged 3 commits into from Sep 29, 2017

Conversation

Projects
None yet
3 participants
@almightyju
Contributor

almightyju commented Sep 19, 2017

Implementation for https://issues.apache.org/jira/browse/COUCHDB-1959

It's a very simple use where you can set cookie_domain in the config which gets put on the set-cookie header.

I couldn't work out where the documentation is to update that and I'm not familiar enough with Erlang to write a test for it but a manual test with and without the config value works fine. If anyone wants to point out how to make a test for it I'm happy enough to put it in if desired.

@wohali

This comment has been minimized.

Show comment
Hide comment
@wohali

wohali Sep 20, 2017

Member

Before merging this we'll definitely want to see a test for this.

Can you confirm that this does the right thing on the clustered port (5984)?

Member

wohali commented Sep 20, 2017

Before merging this we'll definitely want to see a test for this.

Can you confirm that this does the right thing on the clustered port (5984)?

@almightyju

This comment has been minimized.

Show comment
Hide comment
@almightyju

almightyju Sep 20, 2017

Contributor

I've been using this build myself for dev since making the pull request and it all works as expected on port 5984.

With the tests I can see how the login is handled easy enough but I can't work out how the server is started since I guess it would have to be restarted with different a config to test the domain gets set on the cookie correctly.

Contributor

almightyju commented Sep 20, 2017

I've been using this build myself for dev since making the pull request and it all works as expected on port 5984.

With the tests I can see how the login is handled easy enough but I can't work out how the server is started since I guess it would have to be restarted with different a config to test the domain gets set on the cookie correctly.

@wohali

This comment has been minimized.

Show comment
Hide comment
@wohali

wohali Sep 28, 2017

Member

Hi @almightyju , I'd like to push forward with this. I also want you to get the credit for the change!

You don't have to restart the server with a different config, just change it as part of your setup() function, and set it back to the default value in your teardown() function. Here's an example:

https://github.com/apache/couchdb/blob/master/src/couch/test/couchdb_mrview_cors_tests.erl#L32-L57

If you need more guidance, I'm happy to work together with you on this, just let me know.

Member

wohali commented Sep 28, 2017

Hi @almightyju , I'd like to push forward with this. I also want you to get the credit for the change!

You don't have to restart the server with a different config, just change it as part of your setup() function, and set it back to the default value in your teardown() function. Here's an example:

https://github.com/apache/couchdb/blob/master/src/couch/test/couchdb_mrview_cors_tests.erl#L32-L57

If you need more guidance, I'm happy to work together with you on this, just let me know.

@janl

janl approved these changes Sep 29, 2017

+1

@almightyju

This comment has been minimized.

Show comment
Hide comment
@almightyju

almightyju Sep 29, 2017

Contributor

@wohali, finally got the tests done, I really do dislike erlang :/

Anyway, new tests are passing for me, not confident about the need for clustered and backdoor for this so I've only tested against the clustered port.

Contributor

almightyju commented Sep 29, 2017

@wohali, finally got the tests done, I really do dislike erlang :/

Anyway, new tests are passing for me, not confident about the need for clustered and backdoor for this so I've only tested against the clustered port.

@wohali

This comment has been minimized.

Show comment
Hide comment
@wohali

wohali Sep 29, 2017

Member

Only clustered matters here, the node-local port is deprecated for all communications other than essential system admin tasks.

Looks good! Assuming the Travis changes pass, we can merge this PR.

Member

wohali commented Sep 29, 2017

Only clustered matters here, the node-local port is deprecated for all communications other than essential system admin tasks.

Looks good! Assuming the Travis changes pass, we can merge this PR.

@wohali wohali merged commit 0eb7677 into apache:master Sep 29, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@wohali

This comment has been minimized.

Show comment
Hide comment
@wohali

wohali Sep 29, 2017

Member

Thanks again @almightyju - great job, and congrats on your first-time contribution to Apache CouchDB!

Member

wohali commented Sep 29, 2017

Thanks again @almightyju - great job, and congrats on your first-time contribution to Apache CouchDB!

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