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

CRUD for EnvironmentVariables from Project's admin #4899

Merged
merged 17 commits into from Jan 15, 2019

Conversation

@humitos
Copy link
Member

@humitos humitos commented Nov 13, 2018

This PR allows users to add environment variables to be used at build time. This way, they can include secrets (or configs) that should not be in the repository.

Phase II from #3992 | PR branched from #4894

Missing things:

  • django templates
  • crud tests
  • form restriction tests
  • phase I merged (#4894)

I only let the user to LIST, CREATE and DELETE environment variables since I don't think it's a good idea to be able to "Edit" and show a sensible secret on the browser. Also, if that's a secret that only one of the owners knows we should not expose that value to all the owners of the project.

@humitos humitos force-pushed the humitos/admin/crud-env-variables branch from 02b0742 to cdc848d Nov 13, 2018
@codecov
Copy link

@codecov codecov bot commented Nov 13, 2018

Codecov Report

Merging #4899 into master will increase coverage by 0.12%.
The diff coverage is 95.12%.

@@            Coverage Diff             @@
##           master    #4899      +/-   ##
==========================================
+ Coverage   76.74%   76.87%   +0.13%     
==========================================
  Files         158      158              
  Lines        9951     9981      +30     
  Branches     1244     1246       +2     
==========================================
+ Hits         7637     7673      +36     
+ Misses       1980     1976       -4     
+ Partials      334      332       -2
Impacted Files Coverage Δ
readthedocs/projects/models.py 85.56% <100%> (ø) ⬆️
readthedocs/projects/urls/private.py 100% <100%> (ø) ⬆️
readthedocs/projects/forms.py 80.63% <100%> (+0.97%) ⬆️
readthedocs/restapi/serializers.py 96.25% <50%> (-1.19%) ⬇️
readthedocs/projects/views/private.py 80.55% <93.33%> (+0.5%) ⬆️
readthedocs/gold/views.py 66.17% <0%> (ø) ⬆️
readthedocs/doc_builder/environments.py 87.06% <0%> (+0.78%) ⬆️
readthedocs/vcs_support/backends/git.py 82.27% <0%> (+1.14%) ⬆️
readthedocs/core/signals.py 86.66% <0%> (+4.66%) ⬆️

@humitos humitos force-pushed the humitos/admin/crud-env-variables branch from cdc848d to 2657a19 Dec 4, 2018
Copy link
Member Author

@humitos humitos left a comment

Questions about adding a paragraph explaining detail and list pages.

{% endblock %}

{% block project_edit_content %}
<form method="post" action="{% url 'projects_environmentvariables_delete' project_slug=project.slug environmentvariable_pk=environmentvariable.pk %}">
Copy link
Member Author

@humitos humitos Dec 4, 2018

What do we want to communicate on Environment Variable detail page to the user?

Copy link
Contributor

@agjohnson agjohnson Dec 8, 2018

We probably need to explain that "These environment variables are available to all build steps." This is another place to keep UI prose copy short.

<li>
<a class="button"
href="{% url 'projects_environmentvariables_create' project_slug=project.slug %}">
{% trans "Add Environment Variable" %}
Copy link
Member Author

@humitos humitos Dec 4, 2018

What do we want to communicate on Environment Variable list page to the user?

Copy link
Contributor

@agjohnson agjohnson Dec 8, 2018

I like Heroku's example. Just a short blob explaining env vars and point to the docs for anything substantial. I'm not big on prose content in UI.

Copy link
Contributor

@davidfischer davidfischer left a comment

I like the direction this is going. I do think we need a way to reveal and edit the envvar value. Currently once set, it cannot be edited or even viewed! I think there are a couple decent patterns here and I attached some screenshots.

I also think we should have a little bit of verbiage on the envvar list view page along the lines of:

Environment variables are a way to store settings that are used in your documentation builds.
They are a good place to store any private data such as API keys that your build may need.

Some other sites UIs for reference

Azure

screen shot 2018-12-04 at 10 34 21 am
By default, Azure hides the values but you can show them.

Heroku

heroku-configvars
Heroku hides the keys and values until you hit "reveal config vars" and then it shows names and values.

<p>
{% blocktrans trimmed %}
Notice that the values are not escaped when your builds are executed.
Special characters (for bash) should be escaped accordingly.
Copy link
Contributor

@davidfischer davidfischer Dec 4, 2018

I think we should escape these sorts of things. Think a long random secret key that might have some special characters like $ in it. The user should be able to just copy and paste it in.

Copy link
Contributor

@agjohnson agjohnson Dec 8, 2018

Agreed, let's make it easier for the user. This is a technical implementation detail that the user shouldn't have to worry about

Copy link
Member Author

@humitos humitos Dec 10, 2018

Good point.

I don't want to do this by myself. I checked the code and I found that we are already doing something similar at

https://github.com/rtfd/readthedocs.org/blob/089753253397a0399175969e2e028eecc3b74dd2/readthedocs/doc_builder/environments.py#L294-L295

I found that there is shlex.quote in Python3 and pipes.quote in Python2. I will use this accordingly.

NOTE: we may want to replace our bash_escape_re regex also.

Copy link
Member Author

@humitos humitos Dec 10, 2018

I tested this with,

  • enter: {1}\|\1+-(8)$
  • results: "'{1}\\|\\1+-(8)$'"

and,

  • enter: alo'ja
  • results: '\'alo\'"\'"\'ja\''

are they correct?

@humitos
Copy link
Member Author

@humitos humitos commented Dec 4, 2018

@davidfischer I like your feedback. At the moment, I just kept this simple and I based it on what Travis does: do not edit and do not scape (in fact, I copied the message from their settings page)

Currently once set, it cannot be edited or even viewed!

This was on purpose since I was not sure how we want to manage this. I put an example on the PR description about viewing some secrets added by another owner of the project that the user that is seeing them may not be able to see them.

I found that Travis behaves in this way. Also, GitHub for example does not allow you to reveal "Personal Access Tokens" once they are created and copied to the clipboard. It's not exactly the same use case, but similar in the sense they are secrets.

It's pretty easy to show a form to update them. I mean, it's not a technical issue. We only need to make a decision about this.

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Dec 4, 2018

I put an example on the PR description about viewing some secrets added by another owner of the project that the user that is seeing them may not be able to see them.

I had not considered this but that's a pretty good reason to not make them viewable or editable.
I'm fine if we decide to go that route, but I think we should be up front about it. We should say on the list view and the form view that they cannot be seen or edited once set.

@humitos
Copy link
Member Author

@humitos humitos commented Dec 4, 2018

I had not considered this but that's a pretty good reason to not make them viewable or editable.

There are more things that could be considered here. Travis, for example, shows a boolean saying "Display value on build log". We could add a boolean to mark a variable as "Secret" and do not display/allow to edit only these ones.

Although, I'm not sure how if this complexity worth in the end.

@humitos humitos requested a review from Dec 4, 2018
@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Dec 8, 2018

Let's keep this simple to start. I like the plan for making the UI read only as the token is created. If we want to make the UI fancier later, we can revisit this as a new feature.

Copy link
Contributor

@agjohnson agjohnson left a comment

I'll pull this down next week and test, or feel free to attach some screen grabs if you get to this before I can test. I prefer to keep UI copy minimal -- UI with a lot of copy usually indicates your UI is not obivous. We'll link to doc pages with lots of information for users instead.

I like the pattern of keeping this UI as simple as possible for now. Read once on envvar input makes a lot of sense for a few reasons, at least to start.

{% endblock %}

{% block project_edit_content %}
<form method="post" action="{% url 'projects_environmentvariables_delete' project_slug=project.slug environmentvariable_pk=environmentvariable.pk %}">
Copy link
Contributor

@agjohnson agjohnson Dec 8, 2018

We probably need to explain that "These environment variables are available to all build steps." This is another place to keep UI prose copy short.

<p>
{% blocktrans trimmed %}
Notice that the values are not escaped when your builds are executed.
Special characters (for bash) should be escaped accordingly.
Copy link
Contributor

@agjohnson agjohnson Dec 8, 2018

Agreed, let's make it easier for the user. This is a technical implementation detail that the user shouldn't have to worry about

<li>
<a class="button"
href="{% url 'projects_environmentvariables_create' project_slug=project.slug %}">
{% trans "Add Environment Variable" %}
Copy link
Contributor

@agjohnson agjohnson Dec 8, 2018

I like Heroku's example. Just a short blob explaining env vars and point to the docs for anything substantial. I'm not big on prose content in UI.

@humitos humitos force-pushed the humitos/admin/crud-env-variables branch from 0d1c4eb to feaba50 Dec 10, 2018
@humitos
Copy link
Member Author

@humitos humitos commented Dec 10, 2018

Some screenshots,

Empty list view,

captura de pantalla_2018-12-10_11-05-02

Create/Add view,

captura de pantalla_2018-12-10_11-06-07

Non-empty list view,

captura de pantalla_2018-12-10_11-12-20

Validation error,

captura de pantalla_2018-12-10_11-11-48

I think we need more validations on the name, like only allow a limited number of ASCI chars (letters, numbers and underscore).

Delete view,

captura de pantalla_2018-12-10_11-06-41

This one is too poor :/

@humitos humitos requested a review from Dec 10, 2018
@humitos humitos added this to the 2.9 milestone Dec 10, 2018
raise forms.ValidationError(
_("Variable name can't start with READTHEDOCS"),
)
elif self.project.environmentvariable_set.filter(name=name).exists():
Copy link
Member

@stsewd stsewd Dec 13, 2018

We could add this as a constraint in the model

Copy link
Member Author

@humitos humitos Dec 13, 2018

Yes. I suppose that we still need the validation in the form otherwise I think it will explode when trying to save it and the user will have a 500.

@humitos humitos force-pushed the humitos/admin/crud-env-variables branch 2 times, most recently from f0d37c0 to 8a002b9 Dec 17, 2018
@humitos
Copy link
Member Author

@humitos humitos commented Dec 17, 2018

I think we need more validations on the name, like only allow a limited number of ASCI chars (letters, numbers and underscore).

This is already done via a regex.

@humitos humitos requested a review from Dec 17, 2018
@humitos
Copy link
Member Author

@humitos humitos commented Dec 17, 2018

The only missing thing here could be to decide what to show in the detail page of the environment variable, otherwise, we could merge this PR.

I'm not really sure what kind of text to add there since there are not too much to say or to show really because we don't want to show the value.

@stsewd
Copy link
Member

@stsewd stsewd commented Dec 17, 2018

Maybe we can add some user docs? and/or maybe update https://docs.readthedocs.io/en/latest/builds.html

@humitos
Copy link
Member Author

@humitos humitos commented Dec 18, 2018

Maybe we can add some user docs? and/or maybe update docs.readthedocs.io/en/latest/builds.html

Good idea! I added a simple .. tip:: communicating this in the "Build environment" section.

@humitos humitos force-pushed the humitos/admin/crud-env-variables branch 2 times, most recently from 65e3022 to ee5bb12 Dec 26, 2018
humitos added 2 commits Dec 27, 2018
Add the `environment_variables` field to this serializer that will be
returned only when the user is admin.
@humitos humitos force-pushed the humitos/admin/crud-env-variables branch from ee5bb12 to dec39ba Dec 27, 2018
@humitos humitos force-pushed the humitos/admin/crud-env-variables branch from 2beb9e2 to e7a2ea9 Jan 3, 2019
Copy link
Member

@ericholscher ericholscher left a comment

Looks good pending more docs and a bit more copy.

Environment Variable: {{ name }}
{% endblocktrans %}
{% endblock %}

Copy link
Member

@ericholscher ericholscher Jan 10, 2019

I think we need copy here saying "The value here is not shown for security purposes" or similar. Otherwise it's a weird UI.

docs/builds.rst Outdated
.. tip::

In case extra environment variables are needed to the build process (like secrets, tokens, etc),
you can add them going to **Admin > Environment Variables** in your project.
Copy link
Member

@ericholscher ericholscher Jan 10, 2019

I think we need more docs than this. We should have a guide in guides/, something along the lines of "I need secrets or environment variables in my build" that mentions the use case of keeping secrets.

Copy link
Member Author

@humitos humitos Jan 14, 2019

I pushed a guide for this.

It mention only one example for now (using the secret to access to a user/pass protected URL). I'm not sure if we want to mention other cases or something else.

@humitos humitos requested a review from ericholscher Jan 14, 2019
Copy link
Member

@ericholscher ericholscher left a comment

Changes look good. Let's ship it! 👍


Values will never be exposed to users, even to owners of the project. Once you create an environment variable you won't be able to see its value anymore because of security purposes.

After adding an environment variable from your project's admin, you can access it from your build process using Python, for example:
Copy link
Member

@ericholscher ericholscher Jan 15, 2019

💯 addition.

@humitos humitos merged commit 35856b0 into master Jan 15, 2019
2 checks passed
@delete-merged-branch delete-merged-branch bot deleted the humitos/admin/crud-env-variables branch Jan 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants