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

Add cookie session authenticator #211

Merged
merged 1 commit into from
Jun 28, 2019

Conversation

alexdavid
Copy link
Contributor

@alexdavid alexdavid commented Jun 17, 2019

Related issue

@aeneasr as discussed this is an initial implementation of what the cookie session authenticator could look like. If you like the API I can go in and add some documentation around it, otherwise please let me know if you have a better idea of how you would like this to work.

Proposed changes

The authenticator assumes there's a RESTful session service that responds to GET /sesions/{sessionId} with the json structure { subject: string, extra: map[string]interface{} }. This authenticator reads a session cookie from the incoming request. Then if the passed sessionId returns a 200, it populates the subject/extra fields for the pipeline. If the cookie does not exist it returns ErrAuthenticatorNotResponsible, otherwise authentication fails.

Checklist

  • I have read the contributing guidelines
  • I have read the security policy
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got green light (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation within the code base (if appropriate)
  • I have documented my changes in the
    developer guide (if appropriate)

@CLAassistant
Copy link

CLAassistant commented Jun 17, 2019

CLA assistant check
All committers have signed the CLA.

@alexdavid
Copy link
Contributor Author

@kevgo

@aeneasr
Copy link
Member

aeneasr commented Jun 19, 2019

Awesome, thank you! I didn't have time to review it yet but it's on my list for this week.

@aeneasr aeneasr self-assigned this Jun 19, 2019
@aeneasr aeneasr self-requested a review June 19, 2019 11:58
driver/configuration/provider_viper_authn.go Outdated Show resolved Hide resolved
pipeline/authn/authenticator_cookie_session.go Outdated Show resolved Hide resolved
pipeline/authn/authenticator_cookie_session.go Outdated Show resolved Hide resolved
pipeline/authn/authenticator_cookie_session.go Outdated Show resolved Hide resolved
@alexdavid alexdavid force-pushed the ad-add-cookie-session branch 5 times, most recently from e4cca9e to fb27924 Compare June 25, 2019 22:31
@alexdavid
Copy link
Contributor Author

@aeneasr comments addressed, let me know what you think!

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

This looks great! Only a few nitpicks :)

Oh and could you please add a section about this authenticator to the docs? :)

docs/config.yaml Outdated Show resolved Hide resolved
driver/configuration/provider_viper_authn.go Outdated Show resolved Hide resolved
driver/configuration/provider_viper_authn.go Outdated Show resolved Hide resolved
pipeline/authn/authenticator_cookie_session.go Outdated Show resolved Hide resolved
@alexdavid alexdavid force-pushed the ad-add-cookie-session branch from fb27924 to 64cfe1c Compare June 26, 2019 19:04
@alexdavid alexdavid force-pushed the ad-add-cookie-session branch from 64cfe1c to 4bbd164 Compare June 26, 2019 22:15
@aeneasr aeneasr merged commit f8a66b7 into ory:master Jun 28, 2019
@aeneasr
Copy link
Member

aeneasr commented Jun 28, 2019

Awesome, thank you!

@alexdavid alexdavid deleted the ad-add-cookie-session branch June 28, 2019 18:01
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.

3 participants