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

Integrate Keycloak in int and scr env #2609

Merged
merged 8 commits into from
Aug 28, 2023

Conversation

aneelac22
Copy link
Contributor

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2023

Codecov Report

Merging #2609 (a3dc995) into master (56f391c) will increase coverage by 0.05%.
Report is 2 commits behind head on master.
The diff coverage is 83.33%.

❗ Current head a3dc995 differs from pull request most recent head c33c522. Consider uploading reports for the commit c33c522 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2609      +/-   ##
==========================================
+ Coverage   57.57%   57.62%   +0.05%     
==========================================
  Files          92       91       -1     
  Lines        2786     2773      -13     
  Branches      711      656      -55     
==========================================
- Hits         1604     1598       -6     
- Misses       1066     1174     +108     
+ Partials      116        1     -115     
Files Changed Coverage Δ
src/cognito/auth.ts 2.22% <0.00%> (-0.06%) ⬇️
src/auth/index.ts 29.62% <40.00%> (-0.56%) ⬇️
src/jwt/offline.ts 94.54% <80.00%> (-3.29%) ⬇️
src/utils/common.ts 62.38% <83.33%> (+1.02%) ⬆️
src/jwt/jwt.ts 73.43% <91.66%> (-0.09%) ⬇️
src/chrome/create-chrome.ts 48.75% <100.00%> (+0.64%) ⬆️
src/components/AppFilter/useAppFilter.ts 94.38% <100.00%> (-0.25%) ⬇️
src/jwt/Priv.ts 83.33% <100.00%> (ø)
src/jwt/user.ts 78.78% <100.00%> (ø)
src/utils/consts.ts 72.50% <100.00%> (+0.70%) ⬆️
... and 1 more

... and 24 files with indirect coverage changes

portal: 'https://ephem.outsrights.cc/',
},
int: {
url: ['console.int.openshiftusgov.com'],
Copy link
Member

Choose a reason for hiding this comment

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

Would it not be better to allow these URLs to be configured when building the assets via env variables and avoid exposing URLs like these on GitHub?

Copy link
Contributor

@Hyperkid123 Hyperkid123 Aug 21, 2023

Choose a reason for hiding this comment

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

@bastilian we configure the SSOUrl for our environments in the frontend operator and the values are not public. However, the specific environment of this PR is not yet using the frontend operator. It's in the process of migration. The same applies to the qaprodauht. Until qaprodauth is discontinued and all envs are using the FEO, we need these values still.

@@ -134,7 +136,7 @@ export const doOffline = (key: string, val: string, configSsoUrl?: string) => {
scopes.push(partnerScope);
}

if (ssoScopes) {
if (ssoScopes && !itLessKeycloakEnv) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a double-check. The offline token is not supposed to have all SSO scopes of the current session?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are no scopes in fedramp and this step is not needed

Copy link
Contributor

@Hyperkid123 Hyperkid123 left a comment

Choose a reason for hiding this comment

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

LGTM, there is just a small conflict due to the pf migrations.

@Hyperkid123 Hyperkid123 merged commit e0252dc into RedHatInsights:master Aug 28, 2023
6 checks passed
@aneelac22 aneelac22 deleted the keycloak branch April 10, 2024 08:40
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.

4 participants