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

Update the authentication process for remote storages #4241

Merged

Conversation

shinyhappydan
Copy link
Contributor

@shinyhappydan shinyhappydan commented Sep 3, 2023

Fixes #4063

Remaining tasks:

  • Config option for the existing approach (token)
  • Possible change to construction of KeyValueStore
  • Possible move of construction into sdk module

Comment on lines 36 to 43
Encoder.AsObject.instance[TokenError] {
case TokenHttpError(r) =>
JsonObject(keywords.tpe := "TokenHttpError", "reason" := r.reason)
case TokenNotFoundInResponse(r) =>
JsonObject(keywords.tpe -> "TokenNotFoundInResponse".asJson, "reason" := r.message)
case ExpiryNotFoundInResponse(r) =>
JsonObject(keywords.tpe -> "ExpiryNotFoundInResponse".asJson, "reason" := r.message)
}
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 tried semi-automatic derivation, but the error messages are bad and I couldn't see the problem

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean ?
Also if you only use the message/reason from DecodingFailure/ HttpClientError because it is what we are interested in here, you should not run into this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When using semi-automatic derivation for an Encoder[TokenError], there were errors which were unclear. I tried changing some obvious things, but could not fix the issue. I noticed that we had used this method elsewhere in the codebase, so I did that.

@@ -146,8 +147,12 @@ class StoragePluginModule(priority: Int) extends ModuleDef {

many[ResourceShift[_, _, _]].ref[Storage.Shift]

make[AuthTokenProvider].from { (cfg: StorageTypeConfig) =>
AuthTokenProvider(cfg)
make[KeycloakAuthService].from { (httpClient: HttpClient @Id("storage"), realms: Realms, clock: Clock[UIO]) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to go in sdk and use the same http client as Identities

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 don't see any modules inside of sdk? Also it's using the config from the storage part

@shinyhappydan shinyhappydan marked this pull request as ready for review September 10, 2023 11:49
case class Credentials(user: String, password: Secret[String], realm: Label) extends AuthMethod
object Credentials {
@nowarn("cat=unused")
implicit private val labelConfigReader: ConfigReader[Label] = ConfigReader.fromString(str =>
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't already have one for Label ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we do I don't know where it is

@shinyhappydan shinyhappydan changed the title authenticate using keycloak Update the authentication process for remote storages Sep 11, 2023
@@ -114,6 +114,41 @@ Storages can no longer be created with credentials that would get stored:

These should instead be defined in the Delta configuration.

#### Remote storages
Copy link
Contributor

Choose a reason for hiding this comment

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

To put in the storages page and a link here to the chapter in the storage page

@shinyhappydan
Copy link
Contributor Author

image image image

imsdu
imsdu previously approved these changes Sep 14, 2023
@shinyhappydan shinyhappydan merged commit 288460e into BlueBrain:master Sep 14, 2023
6 checks passed
@shinyhappydan shinyhappydan deleted the remote-storage-auth-changes branch September 14, 2023 11:42
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.

Update the authentication process for remote storages
2 participants