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

Fix JS regex use to validate IRIS IAM strings #354

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

gregcorbett
Copy link
Member

No description provided.

@gregcorbett gregcorbett added the bug label Jun 9, 2022
@gregcorbett gregcorbett added this to the August 2022 milestone Jun 9, 2022
@gregcorbett gregcorbett self-assigned this Jun 9, 2022
@gregcorbett gregcorbett requested a review from a team as a code owner June 9, 2022 10:30
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

It feels like these should eventually not be hardcoded values.

htdocs/web_portal/javascript/linking.js Outdated Show resolved Hide resolved
@ElliottKasoar
Copy link
Contributor

Minor point but should // End with @iris.iam.ac.uk be updated to reflect the correct string?

It's probably obvious, but just to note that the commented out regExIdString above was planned to check the rest of the form of the ID string. It may be worth either implementing it for a more detailed check, or removing it if it's never going to be used (including it would "give away" the full form of the ID string, which may be a issue in itself).

@ghost
Copy link

ghost commented Jun 15, 2022

Minor point but should // End with @iris.iam.ac.uk be updated to reflect the correct string?

Seems like a good idea.

@ghost
Copy link

ghost commented Jun 15, 2022

It's probably obvious, but just to note that the commented out regExIdString above was planned to check the rest of the form of the ID string. It may be worth either implementing it for a more detailed check, or removing it if it's never going to be used (including it would "give away" the full form of the ID string, which may be a issue in itself).

Why was it not used in the first place, and what would be the implication of the 'give away'?

@ElliottKasoar
Copy link
Contributor

ElliottKasoar commented Jun 15, 2022

Why was it not used in the first place

From what I can recall, I think it was a combination of seeming unnecessary, and that I couldn't test it fully, as my instance of GOCDB didn't have IRIS IAM auth enabled (so I wasn't even registered myself).

I think the longer form was essentially a placeholder based on discussions with @gregcorbett about the expected full form of the ID string, but later we moved towards more minimal validation that was sufficient to differentiate the different auth types (e.g. // End with @egi.eu and // Start with slash only).

and what would be the implication of the 'give away'?

I meant in the sense of helping people too much in guessing what an ID string should be that they're not in possession of, but I don't think it's really an issue?

@gregcorbett
Copy link
Member Author

Minor point but should // End with @iris.iam.ac.uk be updated to reflect the correct string?

Ah yes, I've changed the code but not the comment 🤦

@ghost
Copy link

ghost commented Jun 16, 2022

I meant in the sense of helping people too much in guessing what an ID string should be that they're not in possession of, but I don't think it's really an issue?

No I don't think so. The form is (needs to be) published info. I guess this validation is aimed at trapping typos in a form input field.

@gregcorbett
Copy link
Member Author

Minor point but should // End with @iris.iam.ac.uk be updated to reflect the correct string?

Ah yes, I've changed the code but not the comment 🤦

I've fixed this and rebased (into a single commit) the branch on top of the latest dev.

@gregcorbett
Copy link
Member Author

This has been running on the IRIS instance since at least Jun 2022.

@gregcorbett gregcorbett requested a review from a team November 29, 2023 11:08
Copy link
Contributor

@Sae126V Sae126V left a comment

Choose a reason for hiding this comment

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

LGTM

@gregcorbett
Copy link
Member Author

rebasing on latest dev

Co-authored-by: ineilson <ian.neilson@stfc.ac.uk>
@gregcorbett gregcorbett merged commit 608e48a into GOCDB:dev Nov 29, 2023
7 checks passed
@gregcorbett gregcorbett deleted the iris_regex_fix branch November 29, 2023 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants