Skip to content

Conversation

starksm64
Copy link
Contributor

No description provided.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I spotted a couple of typos and then there is this question of the configuration properties.

I suppose the mp.jwt ones are directly accessed by the SmallRye JWT library? Not sure if it will be easy to make them Quarkus ones and pass the property along.

|quarkus.jwt.enabled|true|Determine if the jwt extension is enabled. Enabled by default if you include the smallrye-jwt dependency, so this would be used to disable it.
|quarkus.jwt.realm-name|Quarkus-JWT|Name to use for security realm.
|quarkus.jwt.auth-mechanism|MP-JWT|Name to use for authentication mechanism
|mp.jwt.verify.publickey|none|The `mp.jwt.verify.publickey` config property allows the Public Key text itself to be supplied as a string. The Public Key will be parsed from the supplied string in the order defined in section <<Supported Public Key Formats>>.
Copy link
Member

Choose a reason for hiding this comment

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

Any chance, these properties could be made consistent with the others? The idea was really to expose a consistent set of Quarkus config properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really as these are the values defined by the MP-JWT specification. We can have duplicates, but then we are not really supporting the MicroProfile specification.

Copy link
Member

@gsmet gsmet Mar 4, 2019

Choose a reason for hiding this comment

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

That's what I suspected. For the record, we already have non-quarkus-prefixed properties with the REST client: we define the service URLs using the MP syntax.

@emmanuelbernard thoughts?

@starksm64
Copy link
Contributor Author

Apparently the latest pull with rebase was not done or not done correctly? Is there a way to fix the commits associated with this PR?

@starksm64
Copy link
Contributor Author

Another rebase with force push looks to have cleaned this up

@gsmet
Copy link
Member

gsmet commented Mar 4, 2019

@starksm64 you missed some of my comments (they might be hidden by GitHub as there are too many of them). The remaining ones are all here: https://github.com/jbossas/protean-shamrock/pull/1162/files .

@starksm64
Copy link
Contributor Author

Ok, the 8f61938 commit should have addressed everything but the mp.* property names.

@gsmet
Copy link
Member

gsmet commented Mar 5, 2019

OK, let's merge it, we can discuss the general issue of properties defined in the MP specs later.

@gsmet gsmet merged commit 221097c into quarkusio:master Mar 5, 2019
@stuartwdouglas stuartwdouglas added this to the 0.11.0 milestone Mar 6, 2019
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.

3 participants