Skip to content

Conversation

@abuecker
Copy link
Contributor

No description provided.

andreareginato added a commit that referenced this pull request Oct 29, 2014
fix(AccessToken): Added token query based on $location.path()
@andreareginato andreareginato merged commit 3c38c14 into angularjs-oauth:master Oct 29, 2014
@andreareginato
Copy link
Collaborator

Thanks a lot!

@ahmet
Copy link

ahmet commented Oct 30, 2014

👍 👏

@pavelhoral
Copy link

This broke the plugin for me. Access token is in the hash / fragment part, not in the path, right? Also why is this changed only in dist folder? Am I missing something?

@andreareginato
Copy link
Collaborator

Yes. It broke also the code I've tested this morning. I have no idea on why
this is only in dist. If someone could fix this problem, would be great.
Il 30/ott/2014 12:44 "Pavel Horal" notifications@github.com ha scritto:

This broke the plugin for me. Access token is in the hash / fragment
part, not in the path, right? Also why is this changed only in dist
folder? Am I missing something?


Reply to this email directly or view it on GitHub
#48 (comment)
.

@pavelhoral
Copy link

Ok, so it is not just me :). It is a pitty that this got merged and released. I am missing any kind of description for the "fix" (what use-case is this supposed to fix).
Maybe remove 0.2.9 tag completly (it contains only this commit) because if anyone uses 0.2.x dependency, they will get the broken version.

@andreareginato
Copy link
Collaborator

v0.2.9 removed as suggested. I'm sorry for this, but all tests were passing. I would love to investigate, but no time right now. @pavelhoral, can you help me in this. It would be really nice.

@pavelhoral
Copy link

Tests were not failing because the change was only in the dist. Next release (or maybe sooner to remove it from current master) you should regenerate the dist files, which will drop the change.
As for this change I think that @abuecker needs to explain what was the purpose of the commit. Otherwise there is nothing to do.

@abuecker
Copy link
Contributor Author

Sorry y'all, moving quick to squash production issues on my end, and completely side stepped protocol.

$location.hash() was returning nothing. Using $location.path().substr(1) in theory is the same thing and this would give me the hash value/ access token.

@pavelhoral
Copy link

$location.hash() was returning nothing. Using $location.path().substr(1) in theory is the same thing and this would give me the hash value/ access token.

No, it does not return the same thing - $location behaves differently depending on HTML5 mode (i.e. whether the path is based on the hash or not). Please check the documentation. For non-HTML5 mode you need to register special route.

@abuecker
Copy link
Contributor Author

Yep, I did check the docs. Correct me if I'm wrong, the docs say to use AccessToken.setTokenFromString() which is no longer a public function:

https://github.com/andreareginato/oauth-ng/blob/master/app/scripts/services/access-token.js#L65

@abuecker
Copy link
Contributor Author

Looks like there is a PR to make AccessToken.setTokenFromString() public again:

#49

@pavelhoral
Copy link

You are right, seems that 4ab133f broke non-HTML5 mode. #49 should be merged to make it work again.

@andreareginato
Copy link
Collaborator

Guys. I merged #49. Still not compiled in dist, but I'm gonna do it pretty soon hoping this solves everything. I'm wondering if we should make a better test suite to avoid this same problem next time.

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.

4 participants