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

feat(backend): Allow project auto-claim with pre-defined token #763

Closed
wants to merge 2 commits into from

Conversation

g3n35i5
Copy link

@g3n35i5 g3n35i5 commented Jan 16, 2024

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 #618.

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.
@randombenj
Copy link
Member

Pretty cool idea @g3n35i5 !

While I still think this is a very good change to have, I don't think it fully resolves #618 .
The idea there was to provide an api token on the first push of a project and then always use this token.

However for automated use cases like CI environments I still think this makes sense.

Copy link
Member

@randombenj randombenj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@@ -162,3 +166,32 @@ def should_include(name: str) -> bool:
reverse=True,
),
)


def claim_project(project: str, db: TinyDB) -> str:
Copy link
Member

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

Copy link
Author

@g3n35i5 g3n35i5 Jan 16, 2024

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 😄

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done 👍

@g3n35i5
Copy link
Author

g3n35i5 commented Jan 16, 2024

Pretty cool idea @g3n35i5 !

While I still think this is a very good change to have, I don't think it fully resolves #618 . The idea there was to provide an api token on the first push of a project and then always use this token.

However for automated use cases like CI environments I still think this makes sense.

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.

Copy link
Member

@randombenj randombenj left a 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this exposed? 😅

Copy link
Author

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.

Copy link
Member

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

Copy link
Member

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?

@fliiiix
Copy link
Member

fliiiix commented Jan 16, 2024

@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?

@g3n35i5
Copy link
Author

g3n35i5 commented Jan 17, 2024

@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:

Global Token Defined Token In Header Upload Possible Project Claim Token
✔️ ❌ (Unauthorized) N/A
✔️ ✔️ If tokens are matching Global Token
✔️ ✔️ Header Token
✔️ None (Unclaimed)

What do you think of this approach @fliiiix @randombenj?

@g3n35i5 g3n35i5 marked this pull request as draft January 17, 2024 08:01
@g3n35i5 g3n35i5 closed this Jan 17, 2024
@g3n35i5 g3n35i5 reopened this Jan 17, 2024
@fliiiix
Copy link
Member

fliiiix commented Jan 20, 2024

Sorry that it took me some time to respond.
I guess the main thing that would be confusing to me is that for normal token a different rule set applies which is hard to explain. The other concern i have is that this might be not granular enough and we soon need to add something to add tokens for each project.

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.

@g3n35i5
Copy link
Author

g3n35i5 commented Jan 20, 2024

I guess the main thing that would be confusing to me is that for normal token a different rule set applies which is hard to explain. The other concern i have is that this might be not granular enough and we soon need to add something to add tokens for each project.

The matrix I set up in my comment above could be used as a great documentation (with some text), don't you think?

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.

Unfortunately, your suggestion is not at all the goal I am pursuing with this PR 😆 I would like to use docat to automatically upload all documentation for all projects from my Jenkins. Uploads, overwrites or deletes of documentation for "normal" users should not be possible.

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.

@fliiiix
Copy link
Member

fliiiix commented Jan 20, 2024

The matrix I set up in my comment above could be used as a great documentation (with some text), don't you think?
Don't get me wrong the matrix is great but nobody likes to read documentation :D so if we get a away with building software where similar things behave the same it's better.

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 ?

@g3n35i5
Copy link
Author

g3n35i5 commented Jan 20, 2024

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?

@fliiiix
Copy link
Member

fliiiix commented Jan 20, 2024

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

@g3n35i5
Copy link
Author

g3n35i5 commented Jan 20, 2024

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...

@g3n35i5 g3n35i5 closed this Jan 20, 2024
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.

CI Token management
3 participants