Skip to content
This repository has been archived by the owner on Dec 18, 2024. It is now read-only.

createToken.py: Fix typo output_filename => input_filename #661

Merged
merged 1 commit into from
Sep 21, 2023
+17 −8
Merged

createToken.py: Fix typo output_filename => input_filename #661

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions kuksa_certificates/jwt/createToken.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,20 @@
#!/usr/bin/env python3

# All rights reserved. This configuration file is provided to you under the
# terms and conditions of the Eclipse Distribution License v1.0 which
# accompanies this distribution, and is available at
# http://www.eclipse.org/org/documents/edl-v10.php
########################################################################
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#
# SPDX-License-Identifier: Apache-2.0
########################################################################

import argparse
import json
Expand All @@ -19,10 +30,8 @@ def createJWTToken(input_filename, priv_key):

encoded = jwt.encode(payload, priv_key, algorithm="RS256")

if input_filename.endswith(".json"):
output_filename = input_filename[:-5] + ".token"
else:
output_filename = output_filename + ".token"
output_filename = input_filename[:-5] if input_filename.endswith(".json") else input_filename
Copy link
Contributor

Choose a reason for hiding this comment

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

I have no objection to the bug correction itself, the code looks nice and great that you found and addressed it @landgraf.

But both the old and new implementation does not match the example tokens that we actually have in this repository. Like we have single-read.json and single-read.json.token, i.e. it seems that .token has been added and .json has not been removed. I sort of would prefer that this script matches the example tokens we have, i.e. either just add .token regardless of whether input is single-read.json or single-read, or change name of the example token. The latter would how ever require changes in all places currently using the tokens.

So maybe best to just have only

output_filename = input_filename + ".token"

i.e. do not care if input file ends with .json or not. The reason why I prefer this option rather than renaming the tokens is that the tokens in this directory are used by KUKSA Server (C++ only, i.e. not Databroker), and we currently do not want to spend too much effort on KUKSA.val Server

Copy link
Contributor Author

@landgraf landgraf Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@erikbosch
I'm not sure If I got your comment.
Current code with suggested change works exactly like you've mentioned.

>>> input_filename = "single-read.jsob"
>>> input_filename = "single-read.json"
>>> output_filename = input_filename[:-5] if input_filename.endswith(".json") else input_filename
>>> output_filename += ".token"
>>> print(output_filename)
single-read.token

>>> input_filename = "single-read"
>>> output_filename = input_filename[:-5] if input_filename.endswith(".json") else input_filename
>>> output_filename += ".token"
>>> print(output_filename)
single-read.token

.json suffix deleted if exists and .token suffix added for all tokens

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've modified code a bit to make it even simpler and clever. Please re-review

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but if you look at current files we have for example:

i.e. for existing tokens we did not remove the .json suffix when generating tokens, the input is called single-read.json and the token is called single-read.json.token, but that is not consistent with a script that generates a single-read.token if it gets single-read.json as input.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not how current code works.
Prior the change:

    if input_filename.endswith(".json"):
        output_filename = input_filename[:-5] + ".token"

If file ends with ".json" then ".json" suffix is dropped and output_filename will be ".token" (without json).
In other words suggested change doesn't change behavior of the positive branch but fixes negative (else) branch which was broken before..

    else:
        output_filename = output_filename + ".token"

^ This code is broken and fails with:
NameError: name 'output_filename' is not defined
So either your example was not generated with the current version of the script or there's bug somewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I share your conclusion, the current example token pairs in the repository that all have the form <something>.json and <something>.json.token does not match how the script work. If you use <something>.json as input you get <something>.token as output. Which gives us three options:

  • Keep the mismatch. Not optimal as I see it.
  • Adapt the script so it matches the pattern used in the examples, i.e. never remove .json
  • Rename the examples so they match how the scripts works. If this is done we would need to do a search/replace for all places using the current names.

Copy link
Contributor

@argerus argerus Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

the current example token pairs in the repository that all have the form .json and .json.token does not match how the script work.

This is not correct.

All the access tokens in the top level jwt folder are named like this (created using this script). They all use the new authorization scopes, and have been placed there in order to not be confused with the legacy tokens found here.

Would it help (cause less confusion) to move this script as well to that folder?

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, I only thought about the folder https://github.com/eclipse/kuksa.val/tree/master/kuksa_certificates/jwt, not that https://github.com/eclipse/kuksa.val/tree/master/jwt also use this script.

Yes, we could move the script, but if so we need update *.md files referencing the script. But I think that we anyhow would benefit from having a small README.md in this folder that explains how to regenerate the tokens if needed, which will be need if anyone want to use them after 2025-12-31.

As I propose a small README I can create a separate PR myself, as this PR itself does not change behavior for input files names <something>.json anyway.

output_filename += ".token"

print("Writing signed access token to {}".format(output_filename))
with open(output_filename, "w") as output:
Expand Down
Loading