1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
function getData ($data) {
if ($data == "avatar") {
$avatar = file_get_contents("https://unrealsoftware.de/getuserdata.php?id=" . $_COOKIE['usgn'] . "&data=avatar");
echo "<img src='https://unrealsoftware.de/" . $avatar . "' alt='Avatar' class=' rounded-full'/>";
} elseif ($data == "mode") {
$mode = file_get_contents("https://unrealsoftware.de/getuserdata.php?id=" . $_COOKIE['usgn'] . "&data=modetxt");
$pt_status = preg_replace('#(<[a-z ]*)(style=("|\')(.*?)("|\'))([a-z ]*>)#', '\\1\\6', $mode);
return $pt_status;
} elseif ($data == "country") {
$country = file_get_contents("https://unrealsoftware.de/getuserdata.php?id=" . $_COOKIE['usgn'] . "&data=country");
return $country;
} elseif ($data == "regdate") {
$regdate = file_get_contents("https://unrealsoftware.de/getuserdata.php?id=" . $_COOKIE['usgn'] . "&data=regdate");
$format_regdate = date("Y-m-d", $regdate);
return $format_regdate;
} else {
return false;
}
}
Overall the code quality is not so good, but this part kills me the most 
The way you add one string to another isn't optimised. You already use double brackets, so you can include variables right inside them, no need to use the .
operator. Or use single brackets instead of double.
You use if-else
instead of switch
.
You create too many variables, while it's possible to perform all you need on the output right inside the return
statement.
For the first case you use echo
instead of return
.
Improper formatting of the code.
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
function validateUSGN() {
$accessKey = null;
$usgn = null;
// UnrealSoftware Connect
$api = "https://unrealsoftware.de/connect.php";
$redirectURI = "OWNWEBSITE";
if($_SERVER['REQUEST_METHOD'] == 'POST') {
if (!is_numeric($_POST['usgn']) || $_POST['usgn']<1 || $_POST['usgn']>999999) {
// if usgn is not numeric and/or below 1 or more than 9999999
echo "Invalid USGN ID format: Must be numeric and above 0!<br>";
} else {
$accessKey = hashing(); // Generate an access key
// Connect with API
$setkey = $api .= "?setkey=" . $accessKey . "&autoclose=1&redirect=" . $redirectURI;
header("Location:" . $setkey);
// Store data in brower cookies, for now...
setcookie("accessKey", $accessKey, time() + (86400 * 30), "/"); // 86400 = 1 day
setcookie("usgn", $_POST['usgn'], time() + (86400 * 30), "/"); // 86400 = 1 day
}
}
}
No need to create variables of null
values that early. Better check that whatever handles these variables doesn't misbehave. And if it does, set their values to what you want or make the code throw Exceptions.
This function would try to continue working as normal in case the POST data contains a float number.
This function returns nothing.
This function does nothing if the request method is not POST. Also, it seems to be this function is called upon every request to the page??? Why is it like that? Better make your program to check if the method is POST, and only then perform whatever is needed. No need to create a function for it. Also it's being called late enough, on the step where PHP is about to return HTML output. This would generate a notice/warning (I don't remember exactly) if the error_reporting
wasn't disabled by you.
Usage of .=
instead of .
for $setkey = $api .= "?setkey=" . $accessKey . "&autoclose=1&redirect=" . $redirectURI;
performs changes on the $api
variable, which is not what we need. I'd also say there's need for the $api
variable at all.
Your program doesn't stop after it sets cookies and returns the Location header.
1
2
3
4
5
6
7
8
9
10
function isLoggedIn() {
$check = file_get_contents("https://unrealsoftware.de/connect.php?keyofid=" . $_COOKIE['usgn'] . "&iskey=" . $_COOKIE['accessKey']);
if ($check == 1 and isset($_COOKIE['accessKey'])) {
return true;
} elseif ($check == 0) {
return false;
} else {
return false;
}
}
elseif
part. Simply use else.
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
<?php
function logout() {
if (isset($_COOKIE['usgn']) && ($_COOKIE['accesskey'])) {
setcookie("usgn", "", time() - 3600, "/", "", 0);
setcookie("accesskey", "", time() - 3600, "/", "", 0);
session_destroy();
header("location: ./index.php");
} else {
echo "You already logged out.";
}
}
logout();
header("location: ./index.php");
?>
There's no need make logout()
a function.
There's no CSRF protection for the logging out part.
You use empty strings instead of null
.
This won't sign out a user if they've removed one of the cookies. You also forgot to place the second isset()
here.
The Location header is being set twice if the user signs out. If the user has already signed out, he'd not be able to see the message that says he has already signed out.
There are probably more problems, such as the getUSGN()
function not used anywhere in your example (it also returns a string ERROR
on failure instead of null
or false
for whatever reason). Also, please don't embed PHP inside HTML code that often. Simply use Heredoc, so it'd be more clean. I'm not sure if you still have plans on working on this project, so let's consider this a notice for those who has plans on using it, at least. 
Stuff 
[PHP]ConnectED - USGN Login 
1 likes it!
Offline
Features:
Built with TailwindCSS 3.0.24 (uses CDN) 
Skripter5000: 
Where possible USGN ID is fetched by cookies to save traffic 
Download
1 
