Skip to content

Conversation

stefanvanherwijnen
Copy link
Contributor

I moved the swagger views directory to the src folder because otherwise there would be two root dirs in the views Configuration.
I tested initializing a project with either swagger or oidc, and both and everything seems to work.

The src folder which is copied in the cli-plugin-oidc-provider is basically a copy of the test/app folder in the oidc plugin and thus matches the documentation 1:1.

"OpenID Connect provider" can be selected as a feature and everything should work out of the box (apart from the correct OIDC configuration). Generating OIDC configurations with the CLI could possibly be a feature in the future by rendering it into oidcConfig.hbs (e.g. defining clients on initialization).

@Romakita
Copy link
Contributor

Awesome :D

@Romakita
Copy link
Contributor

arf... but views directory must be outside /src to preserve path when you build application

@stefanvanherwijnen
Copy link
Contributor Author

Do you mean that views is not bundled upon yarn build? I think we should copy the views folder in that case as srcseems to be the correct location for views in my opinion.

@Romakita
Copy link
Contributor

Exactly the views isn't copied by the yarn buid / tsc. It's normal, this is why I prefer to set the views outside src to preserve the relative paths between src and build directories. I'll push a commit to fix that :)

@stefanvanherwijnen
Copy link
Contributor Author

It looks like NestJS also has views outside the src folder (https://docs.nestjs.com/techniques/mvc#model-view-controller). It's up to you, but to me it makes more sense that src compiles to dist and everything necessary is included. Otherwise you might end up hosting the dist folder and it won't work because you forgot to copy the views folder manually (or are required to keep the entire git repo checked out on your hosting server).

@Romakita
Copy link
Contributor

Romakita commented Aug 22, 2021

Yes sure but the build is performed by tsc and tsc doesn't make a copy of the views directory or other directories that haven't ts files. Basically, the project works without extra command if the views directory is outside the src ^^. After, if the developer wants to add extra command to copy the views, it can :)

@Romakita Romakita merged commit a57f9ec into tsedio:master Aug 22, 2021
@Romakita
Copy link
Contributor

🎉 This PR is included in version 3.10.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Romakita
Copy link
Contributor

@stefanvanherwijnen I released a new version with two fix:

  • one about the npm published package (missing files in archive)
  • one about the oidcbasePath ^^

the release is on going. You will be able to test your works with the v3.10.2

@stefanvanherwijnen
Copy link
Contributor Author

stefanvanherwijnen commented Aug 22, 2021


InteractionCtrl is not and should not be mounted at the oidc basePath 😉. Interactions are completely separate from oidc-provider.

https://github.com/panva/node-oidc-provider/blob/main/docs/README.md#user-flows

@stefanvanherwijnen
Copy link
Contributor Author

Actually, it seems to work fine. I expected oidc-provider to return a 404.
One issue, the renderAll function contains a few errors.
Should be:

          this.rootRenderer.renderAll(
            [
              "/src/config/oidc/index.ts.hbs",
              "/src/controllers/oidc/InteractionsCtrl.ts.hbs",
              "/src/interactions/ConsentInteraction.ts",
              "/src/interactions/CustomInteraction.ts",
              "/src/interactions/LoginInteraction.ts",
              "/src/models/Account.ts",
              "/src/services/Accounts.ts",
              "/views/forms/interaction-form.ejs.hbs",
              "/views/forms/login-form.ejs.hbs",
              "/views/forms/select-account-form.ejs.hbs",
              "/views/partials/footer.ejs",
              "/views/partials/header.ejs",
              "/views/partials/login-help.ejs.hbs",
              "/views/interaction.ejs",
              "/views/login.ejs",
              "/views/repost.ejs.hbs"
            ].filter(Boolean),
            ctx,
            {
              templateDir: `${TEMPLATE_DIR}/init`
            }
          )

@Romakita
Copy link
Contributor

InteractionCtrl is not and should not be mounted at the oidc basePath 😉. Interactions are completely separate from oidc-provider.

https://github.com/panva/node-oidc-provider/blob/main/docs/README.md#user-flows

Ok. I didn't know that but it doesn't make a lot of sense to me. There is no security reason behind that that would impose this kind of rule. ^^

I correct the cli suddenly.

@Romakita
Copy link
Contributor

#159

@stefanvanherwijnen
Copy link
Contributor Author

My guess was that because oidc-provider is its own koa instance, mounting /interactions under the oidc basePath would result in a 404 because the routes would not be registered in the koa instance. But I was wrong and it did seem to work (probably because of interactionsUrl).
I do think it is better to keep /oidc limited to just oidc-provider, and mount any user implementations (interactions) somewhere else (to make it more clear that they are not part of oidc-provider and to prevent any errors which might occur when mounting them under the basePath of the oidc-provider koa instance).
I'll give the recent changes a test, thanks 👍 .

@stefanvanherwijnen
Copy link
Contributor Author


Should be just "/interaction/:uid" now. With that change it all seems to work.

@Romakita
Copy link
Contributor

Romakita commented Aug 23, 2021

Fix is on going. Thanks for your help ;)

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants