Skip to content

Migrate to API tokens not tied to user accounts#1277

Merged
lamppu merged 2 commits into
mainfrom
do-1096-change-the-way-dos-api-tokens-are-handled
Jun 17, 2025
Merged

Migrate to API tokens not tied to user accounts#1277
lamppu merged 2 commits into
mainfrom
do-1096-change-the-way-dos-api-tokens-are-handled

Conversation

@lamppu

@lamppu lamppu commented Jun 5, 2025

Copy link
Copy Markdown
Member

In the early development, the DOS API was used with ORT from the CLI, and so the access to the endpoints that ORT uses was authenticated with a personal access token, that each user could get through the UI. Now that ORT Server has changed things quite a bit, there is really only a need for one token that the ORT Server holds, and the only other tokens that are needed are for developers.

There is an upcoming feature, where users will be able to make their own clearance items, and these clearance items will be accessible to the users in the same organization they were created for. This causes a need to change the implementation of the package configurations endpoint, so that it will return clearance items based on an organization ID. It is possible to keep the current authentication method, but a stricter access to the endpoint will make things a bit simpler on the API side.

Another small quite unlikely but also quite an important protection that this change brings, is not allowing users to add jobs straight through the API, which would prevent the edge-case where a malicious user could potentially add some garbage data to DOS.

Also after this change, there won't be a need to have a fake user for the purpose of managing the ORT Server DOS API token.

Please see the individual commit messages for details.

@linear

linear Bot commented Jun 5, 2025

Copy link
Copy Markdown

@lamppu
lamppu temporarily deployed to test-environment June 5, 2025 06:30 — with GitHub Actions Inactive
@lamppu
lamppu temporarily deployed to unittest-environment June 5, 2025 06:30 — with GitHub Actions Inactive
@lamppu
lamppu marked this pull request as ready for review June 5, 2025 06:46
@lamppu
lamppu requested a review from a team as a code owner June 5, 2025 06:46
@lamppu
lamppu force-pushed the do-1096-change-the-way-dos-api-tokens-are-handled branch from 2a92a58 to 31261db Compare June 6, 2025 09:38
@lamppu
lamppu temporarily deployed to unittest-environment June 6, 2025 09:38 — with GitHub Actions Inactive
@lamppu
lamppu temporarily deployed to test-environment June 6, 2025 09:38 — with GitHub Actions Inactive
Comment thread apps/api/src/helpers/auth_helpers.ts Outdated

const cache = new NodeCache({ stdTTL: 5 * 60, checkperiod: 60 });

const tokens = process.env.API_TOKENS?.split(",") || ["token"];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is the default of "token" really meaningful? Couldn't it be more confusing than e.g. using an empty list here as he default?

@lamppu lamppu Jun 16, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah the reason I added it is so that in your local development environment you wouldn't necessarily need to add anything in the .env file. But I'm also fine with doing it the other way.

@sschuberth sschuberth Jun 16, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's good to have it work without anything in .env, but at the same time I think we should not use a fake-token-string, but something that clearly "identifies" as invalid input for a token, like the empty string / array.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So do you mean kind of bypassing this check in local development? So if the list is empty, just allow the request?

@sschuberth sschuberth Jun 16, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure 😄 Is there a chance that the request would succeed in this case? If so, then yes, just allowing the request for local development makes sense. If not, I'd just use empty default values, let the request fail hard, and maybe print / log a hint to the user / developer in this case that a token has to be provided.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, the request would succeed then.

@lamppu
lamppu force-pushed the do-1096-change-the-way-dos-api-tokens-are-handled branch from 31261db to f1af9b7 Compare June 16, 2025 15:32
@lamppu
lamppu had a problem deploying to test-environment June 16, 2025 15:32 — with GitHub Actions Failure
@lamppu
lamppu temporarily deployed to unittest-environment June 16, 2025 15:32 — with GitHub Actions Inactive
lamppu added 2 commits June 16, 2025 19:46
The API tokens that are used to authenticate access to getting scan results,
adding jobs and getting package configurations, used to be personal tokens
for all users. This is changed as there's a need for more restrictions to
the access to these endpoints.

As the number of needed tokens is low and it is expected that they rarely
change, these tokens are added through an environment variable for
simplicity and faster access. However, if the need emerges, `CRUD` support
for token handling for server admins could be added later.

The main motivation for this change comes from developing DOS alongside the
ORT Server, and from an upcoming feature on DOS that will allow users to
create their own clearance items. These clearance items will be linked to
the organization the user belongs in, and for example the package
configurations endpoint will support returning clearance items based on an
organization ID, and so the need to restrict access to this endpoint is
vital, as users shouldn't be allowed to access clearance items on other
organizations than their own.

Signed-off-by: Johanna Lamppu <johanna.lamppu@doubleopen.org>
As the API tokens won't be personal tokens anymore, remove the logic to
add and edit them.

Signed-off-by: Johanna Lamppu <johanna.lamppu@doubleopen.org>
@lamppu
lamppu force-pushed the do-1096-change-the-way-dos-api-tokens-are-handled branch from f1af9b7 to 44ad4cf Compare June 16, 2025 16:47
@lamppu
lamppu temporarily deployed to test-environment June 16, 2025 16:47 — with GitHub Actions Inactive
@lamppu
lamppu temporarily deployed to unittest-environment June 16, 2025 16:47 — with GitHub Actions Inactive
@lamppu
lamppu merged commit a808df6 into main Jun 17, 2025
5 checks passed
@lamppu
lamppu deleted the do-1096-change-the-way-dos-api-tokens-are-handled branch June 17, 2025 06:09
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