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

Option to exclude strategy.token from some ajax requests. #97

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bencolon
Copy link

I make some cross domain ajax requests from my Ember app using ember-auth.
But I don't want the tokenKey parameter to be automatically added to these requests.
This parameter could overwrite a mandatory parameter of the API requested.

So I added an option to exclude strategy.token from some ajax requests based on the domain name with this option:
tokenDomainsBlacklist: ['api1.com:3000', 'api2.com:4000']

Hope it makes sense for you too ;)

@heartsentwined
Copy link
Owner

I like the idea, although forgive me for nitpicking issues.

I think that this concern should belong to Ember.Auth.Strategy itself. Would you mind moving this upwards to the master serialize() method?

In addition, whitelisting should be a better approach than blacklisting in this case. Two reasons: first, your own api server(s) is likely to be the only consumer of auth information; while you could theoretically link to a whole array of external APIs that you don't want to pass auth info to. Next, blacklisting is prone to errors from "forgetting one"; while whitelisting would alert the dev to errors because it "just won't work" if the dev forgot an item.

Finally, the domainUrl method is not tolerant enough to different use cases / methods of specifying the domain. Should we distinguish between http / https? Or other protocols? How about the implicit 80 / 8080 port? Subdomains? Paths? All these should be left to each individual use case, and I'd like to propose replacing string arguments with regexes, and do a simple match on the full URL.

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.

2 participants