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

Implement oauth state attribute #15

Merged
merged 3 commits into from
Jul 8, 2014
Merged

Implement oauth state attribute #15

merged 3 commits into from
Jul 8, 2014

Conversation

doapp-ryanp
Copy link
Contributor

use state attr if defined, otherwise default to current location. QS params were also not encoded.

@doapp-ryanp doapp-ryanp mentioned this pull request Jul 3, 2014
@jaspernbrouwer
Copy link

This would indeed cover various cases of using the state property. +1

andreareginato added a commit that referenced this pull request Jul 8, 2014
Implement oauth state attribute
@andreareginato andreareginato merged commit 0baa247 into angularjs-oauth:master Jul 8, 2014
@andreareginato
Copy link
Collaborator

Hi @doapp-ryanp, thanks a lot for your contribute. Much more appreciated. I would love to ask two more thing:

  • README section where you can see how to use state to solve security problems.
  • Tests for the new additions.

@doapp-ryanp
Copy link
Contributor Author

No - thank you for the work on this wonderful plugin.

Yea man np on the other stuff. Will get to it in the next day or so.

Question regarding this pull. I left the $location.url() in var state = scope.state || $location.url();. Did you really intend to use $location.absUrl()? My guess is most people, by default, would want the full URL (to account for different staging environments). I didn't want to touch this because I was not sure if it was intentional.

@andreareginato
Copy link
Collaborator

I should absolutly think more about this. Actually I'm not sure about keeping $location.url() as a default value for state. I would appreciate to start a discussion about the possibility of keeping it (and making it useful) or removing it.

@doapp-ryanp
Copy link
Contributor Author

My opinion would be to remove the default all together. Most oAuth2 providers (all that I have seen) have it as an optional param. It should be used, but oauth-ng defaulting it may give developers the idea that the CRSF issue has been handled for them, which is a false (and potentially dangerous) assumption.

@andreareginato
Copy link
Collaborator

I agree. If you have any time to take it off, would be great.

One question, to eventually move in one different issue. How are you handling the expired token? In a new app I'm creating these days, I move the user to a specific page where I place a Refresh link that will redirect the user to the oauth server. I'm thinking. Would make sense to create a kind of flow to automatically handle the refresh token?

@doapp-ryanp
Copy link
Contributor Author

Yea I'll remove when I doc and testcase.

Re your other question - its a good one - so I started a new issue here: #17

@doapp-ryanp
Copy link
Contributor Author

Pull request here #18

I can't figure out how to run your test cases - please checkout my comment in the pull request

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