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: flatten (de)serialization of custom user claims #1159

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

jorgehermo9
Copy link
Contributor

@jorgehermo9 jorgehermo9 commented Jan 8, 2025

Closes #1115

Left some doubts as // TODO: in the code, please take a look into that comments when reviewing.

CC @Django-Fakkeldij

@@ -1,6 +1,5 @@
---
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 snapshot file's diff is marked as "renamed file", but actually I deleted the old valid token and custom claims snapshot, as it was unreferenced, and added a new valid token and custom boolean claims snapshot, such as the other from this PR.

src/auth/jwt.rs Outdated Show resolved Hide resolved
@Django-Fakkeldij
Copy link

Think this looks great! Thank you for taking the time to fix this issue ❤️

@jorgehermo9
Copy link
Contributor Author

Fixed failed CI

Copy link
Contributor

@kaplanelad kaplanelad left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR.

src/auth/jwt.rs Outdated Show resolved Hide resolved
src/auth/jwt.rs Outdated Show resolved Hide resolved
starters/rest-api/src/models/users.rs Outdated Show resolved Hide resolved
@jorgehermo9
Copy link
Contributor Author

Hi @kaplanelad, thank you for your review. Addressed the changes you suggested.

@kaplanelad
Copy link
Contributor

@jorgehermo9 can you fix the CI, please?

@jorgehermo9
Copy link
Contributor Author

Hi @kaplanelad . The loco-gen-deploy action is failing (https://github.com/loco-rs/loco/actions/runs/12927511346/job/36052787934?pr=1159) because the app generated tries to use the loco version 0.14.0 instead of the local one. As we changed the public API of the JWT module (and updated the template in loco-new in this PR), the app generated should use the local loco crate instead of pulling 0.14.0 from the registry, where the JWT API is not updated with these changes.

To reproduce, in the root directory of this branch, run

cargo install --path ./loco-new
loco new -n myapp --db sqlite --bg async --assets serverside -a

(those are the commands being ran in https://github.com/loco-rs/loco/actions/runs/12927511346/job/36052787934?pr=1159)

and in myapp/Cargo.toml you can see this
image

In order to fix that, we should link to the local loco dependency, using the LOCO_DEV_MODE_PATH env variable as other CI workflows do

LOCO_DEV_MODE_PATH: ${{ github.workspace }}

with the following command in local development (being at the repo root directory) LOCO_DEV_MODE_PATH=.. loco new -n myapp --db sqlite --bg async --assets serverside -a the generated Cargo.toml file looks like this
image
and the app can be compiled succesfully.

I updated the CI workflow in cac9e5c to address this

@jorgehermo9
Copy link
Contributor Author

jorgehermo9 commented Jan 23, 2025

Now it is failing because the local loco-rs crate is not copied in the dockerfile https://github.com/loco-rs/loco/actions/runs/12928528426/job/36055996790?pr=1159

How should we proceed here? If the dockerfile build should pull the dependency from crates.io, we have to publish a new loco-rs version containing the public api changes.

The other option is to copy the local loco dependency into the docker build context. Tried that in 18de238 but I don't know if it's the best workaround... any suggestions?

- name:
run: cargo loco generate deployment --kind docker && docker build -t demo .
working-directory: ./myapp
- name: generate deployment
Copy link
Contributor Author

@jorgehermo9 jorgehermo9 Jan 23, 2025

Choose a reason for hiding this comment

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

we have to split this in two steps (generate deployment and build deployment) because cargo loco generate deployment --kind docker if we have the local loco dependency in the path
image

The docker build works because once we are copying the files in the container, we move the loco directory to ${{github.workspace}} from /tmp/myapp/loco so the app can compile with cargo build --release

(see the dockerfile template)

@@ -7,7 +7,10 @@ FROM rust:1.83.0-slim as builder
WORKDIR /usr/src/

COPY . .

{% if loco_dev_mode_path -%}
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 had to use this in order to build from a local loco dependency instead of pulling the crate from crates.io

@jorgehermo9 jorgehermo9 force-pushed the master branch 2 times, most recently from df2416f to f54a7b0 Compare January 23, 2025 13:18
@jorgehermo9
Copy link
Contributor Author

jorgehermo9 commented Jan 23, 2025

Fixed CI.

The main problem was that the loco-gen-deploy action was not able to build from a local loco-rs dependency as it was pulling the crate from crates.io. In order to test this PR, a local loco-rs dependency is needed instead, and that dependency must be copied to the docker build context.

0897e27 and 18de238 addressed this limitation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change (de)serialization of UserClaims (specifically claims field)
3 participants