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

[DM-45303] Change authentication provider in QueryJobManager & fix issue with ownerID missing from uws #111

Merged
merged 1 commit into from
Jul 30, 2024

Conversation

stvoutsin
Copy link
Member

@stvoutsin stvoutsin commented Jul 24, 2024

Summary
Now that the previous Authenticator class and its implementation is deprecated in the new Cadc library, it's been replaced by the IdentityManager abc. During the switchover we started by using the StandardIdentityManager class found here:
https://github.com/opencadc/ac/blob/2d178c4291da715ce17fb3ed596169be204bc406/cadc-gms/src/main/java/org/opencadc/auth/StandardIdentityManager.java#L110
This seemed to work, however I've recently found a bug where the ownerID value of UWS jobs was empty.
This PR introduces a new implementer of the Abstract IdentityManager class, which uses the methods defined in our previous AuthenticatorImpl.java class, and for the new abc methods I use the implementations found in the StandardIdentityManager class.

This seems to work now, as authentication works fine, and uws jobs now have the ownerID correctly assigned to them.

Jira issue
https://rubinobs.atlassian.net/browse/DM-45303

Tests
Tested with new UI testing suite which I'll publish soon. This tests:

  • Test TAP Adql queries with Firefly
  • Test 4 of the Notebooks that involve catalog access (TAP), uploads & visualization
  • Run taplint and assert that it matches previous result
    Totals: Errors: 97; Warnings: 695; Infos: 158; Summaries: 20; Failures: 3

Related phalanx changes
For the new IdentityManager implementation to be used in phalanx, requires passing in the following property under the charts/cadc-tap/templates/tap-deployment.yaml:
-Dca.nrc.cadc.auth.IdentityManager=org.opencadc.tap.impl.RubinIdentityManagerImpl

@stvoutsin stvoutsin changed the title Add RubinIdentityManagerImpl as a Rubin implementation of IdentityManager [DM-45303] Add Rubin implementation of IdentityManager Jul 24, 2024
Copy link
Member

@rra rra left a comment

Choose a reason for hiding this comment

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

I'm not against this as a short-term fix, but we were really hoping to stop maintaining a custom authenticator, so it's unfortunate that we're moving backwards here. Is the missing username a bug in the upstream identity manager? Or do we have something configured incorrectly? I thought this was all working in conjunction with the custom Gafaelfawr route that we added for the default CADC authenticator.

@stvoutsin
Copy link
Member Author

I'm not against this as a short-term fix, but we were really hoping to stop maintaining a custom authenticator, so it's unfortunate that we're moving backwards here. Is the missing username a bug in the upstream identity manager? Or do we have something configured incorrectly? I thought this was all working in conjunction with the custom Gafaelfawr route that we added for the default CADC authenticator.

The login process works fine, the only issue I see is that we end up with an empty ownerID in the UWS table.
Not quite sure if it is a bug or missconfig, but I've posted a question to the cadc team so maybe they have a better idea. The subject validation/augment process looks different in the StandardIdentityManager so hopefully if it's a quick fix in there and we can just use theirs.

As for this PR I can wait to hear back before merging it in as a temp fix, and see what they suggest.

@stvoutsin stvoutsin changed the title [DM-45303] Add Rubin implementation of IdentityManager [DM-45303] Change authentication provider in QueryJobManager & fix issue with ownerID missing Jul 26, 2024
@stvoutsin stvoutsin changed the title [DM-45303] Change authentication provider in QueryJobManager & fix issue with ownerID missing [DM-45303] Change authentication provider in QueryJobManager & fix issue with ownerID missing from uws Jul 26, 2024
@stvoutsin
Copy link
Member Author

I've updated the PR after our discussions with cadc, as they also found a bug with the ownerID missing from UWS tables, and pushed a fix to cadc-uws-server-1.2.21. In the changes I've updated to that version, but also changed our QueryJobManager which was previously using an X509 IdentityManager, to now use AuthenticationUtil.getIdentityManager() which in our case gets resolved to an OpenID IM. With these changes we don't need to maintain a custom implementation of the IdentityManager and can just use the StandardIdentityManager from CADC.

One thing to note is that ownerID now appears as stvoutsin rather than cn=stvoutsin which was the case before. It doesn't seem that this breaks anything, as I've ran the following tests which all ran successfully:

  • Portal workflows (Done via playwright test suit, to be uploaded soon so I can reference it)
  • Nublado notebooks (Not all of them, I ran 4 queries that involve catalog access to DP02 & DP03)
  • Taplint:
    Totals: Errors: 97; Warnings: 695; Infos: 158; Summaries: 20; Failures: 3

Copy link
Member

@rra rra left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you for digging into this! I think it's fine to just delete the old AuthenticatorImpl after this has been merged, so it's there in Git if we ever want to dig it back out again but it's not there confusing people into thinking it's used. No one should run a new TAP server with old parameters, since they should only be doing this via Phalanx.

@stvoutsin stvoutsin merged commit 853a71c into master Jul 30, 2024
2 checks passed
@stvoutsin stvoutsin deleted the tickets/DM-45303 branch July 30, 2024 09:05
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.

2 participants