-
Notifications
You must be signed in to change notification settings - Fork 49
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
feat(backend): Allow project auto-claim with pre-defined token #763
Conversation
By declaring the environment variable(s) `DOCAT_GLOBAL_CLAIM_TOKEN` (and optionally `DOCAT_GLOBAL_CLAIM_SALT`), all projects can be automatically claimed with a previously defined token (and salt). This resolves docat-org#618.
Pretty cool idea @g3n35i5 ! While I still think this is a very good change to have, I don't think it fully resolves #618 . However for automated use cases like CI environments I still think this makes sense. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
docat/docat/utils.py
Outdated
@@ -162,3 +166,32 @@ def should_include(name: str) -> bool: | |||
reverse=True, | |||
), | |||
) | |||
|
|||
|
|||
def claim_project(project: str, db: TinyDB) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as an optional remark, I think this function could also take an api toke as parameter.
If this token is defined it would salt the token and then use this token as the claim token.
This would then also resolve the #618 issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you want, I can implement it in a second commit. Or in another PR.
Give me a few minutes, I'll implement it 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done 👍
The change for providing the token on initial push should be minimal with this change. But you're right, it doesn't fully resolve the issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 🎉
@@ -21,6 +21,8 @@ poetry install | |||
* **DOCAT_SERVE_FILES**: Serve static documentation instead of a nginx (for testing) | |||
* **DOCAT_STORAGE_PATH**: Upload directory for static files (needs to match nginx config) | |||
* **FLASK_DEBUG**: Start flask in debug mode | |||
* **DOCAT_GLOBAL_CLAIM_TOKEN**: If set, all uploaded projects are being claimed automatically with that token. | |||
* **DOCAT_GLOBAL_CLAIM_SALT**: If set, all claims will be made with this salt. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this exposed? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I assumed that you only get the token back if you are authorized, but that is not implemented at all. I'll change the PR again tomorrow so that the token is only returned if it's not the globally defined one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before you implement too much please take a look at my other comment, i do my best to keep docat as simple and slim as possible with the least amount of features and im not convinced yet that this is in the current form is a essential feature so i would like to understand the use-case better to decide if we need this feature in the current form or something else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue that at least the use case where one can send a token on first time upload and directly claim the project makes sense both from a security perspective and automation perspective (like your CI/CD usecase @g3n35i5 ) -> #618.
With this implemented is the global env token still necessary or could your usecase also be covered by the first time submitted token feature you implemented yesterday?
@g3n35i5 im struggling a bit to understand what the use-case is here, can you describe what the goal is you want to achieve after this change? |
After sleeping on it for a night, I would now want to implement it differently. I would still leave the global token implemented, but to prevent the upload without a token. This would cover the following use cases:
What do you think of this approach @fliiiix @randombenj? |
Sorry that it took me some time to respond. Would it be good enough to disable tokens completely? Eg. instead of setting a global token one could disable tokens for all projects. Downside is that then everyone could remove and override versions Also its still unclear to me what the use-case is because a CI/CD system can upload new docs without a token and if you want to override ci versions or something you can claim a token once and use that in your ci/cd script. |
The matrix I set up in my comment above could be used as a great documentation (with some text), don't you think?
Unfortunately, your suggestion is not at all the goal I am pursuing with this PR 😆 I would like to use In addition, I don't want to claim the project (with a random token) every time I upload from Jenkins, but rather store the credentials for the upload beforehand. |
How about we do a global token behaving the same as 'normal' tokens and then introduce some kind of lock down config where you need a token to do everything and in normal mode you need to claim the project first before you can upload the first version? Thoughts @randombenj @g3n35i5 ? |
To be honest, I think that's more complicated. As far as I know other (comparable) tools, the approach that you can upload something with a secret API token is actually an established approach, isn't it? |
Yes but one of the goals is to make the usage as simple as possible and not requiring a token to upload things is exactly that. An other option I see it to do a flag to disable tokens completely and then you can patch the nginx config to require http basic auth for all post routes something like this -> https://serverfault.com/a/246583 |
As this is an optional feature, I wouldn't say that it makes it any more difficult to use. To be honest, I'm not sure in what environment you want a documentation server where everyone can upload and delete by default. Then I will probably go with the reverse proxy variant... |
By declaring the environment variable(s)
DOCAT_GLOBAL_CLAIM_TOKEN
(and optionallyDOCAT_GLOBAL_CLAIM_SALT
), all projects can be automatically claimed with a previously defined token (and salt).This resolves #618.