Credential insert by name

[XVincentX]

Connects #361 closes #361
Attempt 2 after #488 failure.

• Merge Credential Service — minor refactor #485 first

Review without whitespace will make the job easier: https://github.com/ExpressGateway/express-gateway/pull/490/files?w=1

What's up?

The following PR|Monster|Dragoon will change the way credentials are stored and retrieved with regards of the consumers.

In particular:

1. On the CLI, whether you're doing -c username or -c id, now a consumer lookup will be performed, therefore all the operations will be performed on the same entity. The id will always be the source of truth for all the subsequent operations.

2. For basic auth, the credential id IS the key id. This is not limiting anything as the gateway today is supporting 1 basic-auth key per consumer. We can change this behaviour in the future eventually.

3. Update all the tests to use user.id instead of the username when looking up for the credentials.

Warning

As this is fixing a wrong behavior, This might be a breaking change for existing users.

What what?

If an user created a credential in this way:

eg credentials create -c userName

It won't be found anymore by the system, since usernames are now "transformed" into their IDs first, and then the credential is retrieved/stored/bla bla.

As I do not know how many people are relying on this, we need to discuss whether this is a breaking change or a simple bugfix (hopefully the latter)

[XVincentX]

When tests are finally passing after 15 hours of work:

[altsang]

want to understand this more carefully - if the admin does eg credentials create -c username from now on - this will work, but you're saying if an admin had done this in the past it won't work?

[XVincentX]

@altsang Yes. That's why I wrote that it might be a breaking change.

Master branch: -c userId and -c username were treated as two different entities, therefore you would get one credential for the userid and one for the username (which is incorrect — they're the same entity).

With this PR, if you call the CLI with -c username, the system will lookup for the stored user, retrive its ID and use that one for all the successive operations.

Given this change, all the previous (and wrong) credentials stored with username won't be accessible anymore (because of this translation).

We could:

• Communicate the change and advice everybody to migrate the old credentials manually
• Provide a migration script
• Provide some code-layer that would detect the legacy condition and keep going
• Anything else that's not coming in my mind right now

I'm open to other possibilities of course — I just wanted to understand what's your intention first.

[codecov]

Codecov Report

Merging #490 into master will decrease coverage by 0.01%.
The diff coverage is 91.52%.

@@           Coverage Diff            @@
##           master   #490      +/-   ##
========================================
- Coverage   92.01%    92%   -0.02%     
========================================
  Files         219    219              
  Lines        9271   9280       +9     
========================================
+ Hits         8531   8538       +7     
- Misses        740    742       +2

Impacted Files	Coverage Δ
...olicy-seq-oauth2-expression-log-ratelimit-proxy.js	97.75% <ø> (ø)	⬆️
lib/services/credentials/credential.service.js	86.66% <ø> (ø)	⬆️
lib/policies/basic-auth/auth.js	91.66% <100%> (ø)	⬆️
bin/generators/users/create.js	96.1% <100%> (ø)	⬆️
test/cli/credentials/create.test.js	95.14% <100%> (ø)	⬆️
test/e2e/oauth2-authorization-code.js	99.13% <100%> (ø)	⬆️
test/services/auth.test.js	100% <100%> (ø)	⬆️
.../policies/oauth/consumer-and-token-headers.test.js	97.53% <100%> (ø)	⬆️
...st/services/credential-service/credentials.test.js	100% <100%> (ø)	⬆️
lib/policies/key-auth/keyauth.js	89.18% <100%> (ø)	⬆️
... and 11 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa9775c...ba2d7d1. Read the comment docs.

[XVincentX]

We agreed we'll create a migration command. I guess we can review/merge this PR in meantime.

That'll need #492 to be completed.

[DrMegavolt]

one small note about status code and then LGTM
I've tried in CLI, it works

[DrMegavolt]

422 is kind of form validation. it should be 404, since no consumer exists

[XVincentX]

That's absolutely correct. 422 means "not processable" entity.

[XVincentX]

I'll candidly admit I have no idea why the coverage is decreasing and how to put it back and make it pass.