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

Include auth_url in AUTH_API_KEY ? #162

Closed
nelsonic opened this issue Nov 11, 2021 · 11 comments
Closed

Include auth_url in AUTH_API_KEY ? #162

nelsonic opened this issue Nov 11, 2021 · 11 comments
Assignees
Labels
discuss Share your constructive thoughts on how to make progress with this issue enhancement New feature or enhancement of existing functionality priority-2 Second highest priority, should be worked on as soon as the Priority-1 issues are finished question A question needs to be answered before progress can be made on this issue T4h Time Estimate 4 Hours technical A technical issue that requires understanding of the code, infrastructure or dependencies

Comments

@nelsonic
Copy link
Member

nelsonic commented Nov 11, 2021

While going through the steps of setting up a "dev" version of this app on Heroku so that we can have Review Apps #161,
it occurred to me that Google includes a URL in their GOOGLE_CLIENT_ID ... 🤯

export GOOGLE_CLIENT_ID=YourAppsClientId.apps.googleusercontent.com

e.g:

88803175225-4hkccb841dh9ffvd2c0.apps.googleusercontent.com

Note: Just an example for illustration. Not a real/valid key. 🔑

We could make the setup of auth_plug even easier because the AUTH_API_KEY could contain the auth_url
and thus the single environment variable becomes all that is needed for configuration!

This would require a fair amount of refactoring (2-3h with tests), but I think it's probably worth it! 💭

@SimonLab Thoughts? 💭

@nelsonic nelsonic added enhancement New feature or enhancement of existing functionality question A question needs to be answered before progress can be made on this issue priority-3 Third priority. Considered "Nice to Have". Not urgent. T4h Time Estimate 4 Hours discuss Share your constructive thoughts on how to make progress with this issue technical A technical issue that requires understanding of the code, infrastructure or dependencies labels Nov 11, 2021
@nelsonic
Copy link
Member Author

Basically we can avoid hard-coding the auth_url in the router.ex of the "consumer" app:

plug(AuthPlug, %{auth_url: "https://dwylauth.herokuapp.com"})

Because the URL would be in the AUTH_API_KEY environment variable. 💭

@nelsonic nelsonic added priority-2 Second highest priority, should be worked on as soon as the Priority-1 issues are finished and removed priority-3 Third priority. Considered "Nice to Have". Not urgent. labels Nov 11, 2021
@SimonLab
Copy link
Member

Adding the redirect url in the environment variable should work. I don't find it to bothering to define the url in the plug config, I was using it to test redirection to other auth applications while developing/testing but I don't mind to switch to a full environment variable configuration.
Like you said there will be a bit of refactoring to do, not so much on auth_plug where we just need to extract the url once I think but mostly on auth where we need to make sure we don't break the way we get the client id and client secret.

@nelsonic
Copy link
Member Author

OK. let's roll this Developer Experience improvement into the "V2" update. 💭

@nelsonic
Copy link
Member Author

To give you an idea: the project I've been working on these last few months has 27 environment variables.
(and even more secrets stored in AWS Secrets Manager ... 🙄)
It's an outlier because there are many 3rd party services . But I think it could easily have been reduced to 2 environment variables with a little bit of forethought which would have significantly reduced Engineer onboarding time.

As for the environment variables required to get the auth_plug working ... My goal is to get it down to one.

image

That way when people run the dwyl/app on their localhost they can get setup in less than a minute and only have 1 thing they need to configure/set.
Thus minimising the friction/barrier to adoption & contribution.
I know some people will consider this "premature optimisation" to be thinking about the onboarding experience for new developers when our App isn't even built yet! But if the whole point of our App is to never waste people's time we need to be conscious of these tiny details at every step.

@SimonLab SimonLab added the in-progress An issue or pull request that is being worked on by the assigned person label Nov 15, 2021
@SimonLab
Copy link
Member

I'm adding the auth_url on the app show page:

<%= #{apikey.client_id}/#{apikey.client_secret}/#{auth_url} %> 

However using the "/" character to seperate the different parts of the key might not work nicely with the url (ie https://...). I'm thinking of using "-" instead

<%= #{apikey.client_id}/#{apikey.client_secret}-#{auth_url} %> 

@nelsonic
Copy link
Member Author

@SimonLab yeah, was considering using the | (vertical bar or "pipe" character https://en.wikipedia.org/wiki/Vertical_bar ) because it is seldom used in URLs. Also the auth_url is just the base URL so it should never contain a | (or at least we won't ever use it). 💭

@SimonLab
Copy link
Member

SimonLab commented Nov 15, 2021

sounds good, I don't mind to change "-" to "|". Or we could create an environment variable to configure this character in the app 😉

There is however an issue in the .env file when we source them:
sh bash: https://dwylauth.herokuapp.com: No such file or directory
So it might not be the best character to use

@nelsonic
Copy link
Member Author

@SimonLab i wasn’t going to use a fully qualified url (with the https:// ) as the protocol is just clutter. We will always connect over HTTPS so it will be omitted from the environment variable.

@SimonLab
Copy link
Member

"https://" removed with 01b19c2

@SimonLab SimonLab removed the in-progress An issue or pull request that is being worked on by the assigned person label Nov 18, 2021
@nelsonic
Copy link
Member Author

Thanks Simon! 🎉

@SimonLab
Copy link
Member

SimonLab commented Dec 1, 2021

The auth_url has been added to the api key with #163

@SimonLab SimonLab closed this as completed Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Share your constructive thoughts on how to make progress with this issue enhancement New feature or enhancement of existing functionality priority-2 Second highest priority, should be worked on as soon as the Priority-1 issues are finished question A question needs to be answered before progress can be made on this issue T4h Time Estimate 4 Hours technical A technical issue that requires understanding of the code, infrastructure or dependencies
Projects
None yet
Development

No branches or pull requests

2 participants